r81340 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r81339‎ | r81340 | r81341 >
Date:22:54, 1 February 2011
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Relax Language::isValidCode() to avoid breaking uselang hacks
Modified paths:
  • /trunk/phase3/languages/Language.php (modified) (history)

Diff [purge]

Index: trunk/phase3/languages/Language.php
——@@ -196,7 +196,7 @@
197197 * not it exists.
198198 */
199199 public static function isValidCode( $code ) {
200 - return (bool)preg_match( '/^[a-z-]+$/', $code );
 200+ return strcspn( $code, "/\\\000" ) === strlen( $code );
201201 }
202202
203203 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r81342MFT r81340: Language::isValidCode() fixtstarling22:57, 1 February 2011
r81343MFT r81340: relax Language::isValidCode() restrictionststarling23:00, 1 February 2011
r81344MFT r81340: relax Language::isValidCode() restrictionststarling23:00, 1 February 2011
r81345MFT r81340: relax Language::isValidCode() restrictionststarling23:00, 1 February 2011
r81576Avoid code duplication for Language::isValidCode...hashar22:55, 5 February 2011
r82927Followup for r81340:...tstarling03:15, 28 February 2011

Comments

#Comment by Hashar (talk | contribs)   10:46, 2 February 2011

In previous r81335 you have used conditional tests such as: 

if ( !Language::isValidCode( $code ) 
 || strcspn( $code, "/\\\000" ) !== strlen( $code )
) ...

Since this revision makes Language::isValidCode() to return: 

strcspn( $code, "/\\\000" ) === strlen( $code );

Both statements looks strictly equivalents unless I am missing something.

#Comment by Tim Starling (talk | contribs)   22:58, 2 February 2011

Yes it's equivalent. Unfortunately I found out that my original plan for Language::isValidCode() was flawed after the vulnerability had already been made public, so I had to push out a quick and safe alternative which was compatible with the documentation I had already published.

#Comment by Hashar (talk | contribs)   22:55, 5 February 2011

I have cleaned code duplication with r81576

#Comment by Platonides (talk | contribs)   18:26, 2 February 2011

What about adding : to the list of forbidden chars?

We could also add DIRECTORY_SEPARATOR, for the sake of being paranoid.

#Comment by Tim Starling (talk | contribs)   23:54, 2 February 2011

I tested colons on Windows and they don't do any harm.

DIRECTORY_SEPARATOR is a pointless constant, except perhaps as a way to generate filenames that look appropriate for the server platform, to provide a less jarring UI for system administrators. For most internal paths you can just use "/". For security, you need to know exactly what characters are used as directory separators, on every platform that the software runs on.

There are varying levels of paranoia. I am worried that someone will screw up Language::isValidCode(), not unreasonably I think, since a very similar screwup caused the release of 1.5.3. That's why I added duplicate path traversal checks. I'm not worried that someone will invent a new operating system where some random punctuation character is used as a directory separator, and that that operating system will become wildly popular for running MediaWiki ;)

#Comment by Platonides (talk | contribs)   08:46, 3 February 2011

> I tested colons on Windows and they don't do any harm.

They were directory separators at least in classic Mac OS. Newer Macs may or may not have some backwards support for them. Given that no language code is expected to contain colons, and would be a fix for a not-too-odd platform (even if old), seems sensible to add.

> DIRECTORY_SEPARATOR...

Fair enough.


> I am worried that someone will screw up...

Thanks for pointing out: Needs tests. :)

#Comment by Platonides (talk | contribs)   09:23, 3 February 2011

I tested colons on Windows and they don't do any harm.

They do provide access to Alternate Data Streams. The appending of .php means you can't access default contents by the ::$DATA trick, and even if you could, it's useless when confined into language folder. But it could be used as a way to regain access into a previously compromised server.

#Comment by Tim Starling (talk | contribs)   03:20, 28 February 2011

Should be all fixed in r82927.

Status & tagging log