r81576 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r81575‎ | r81576 | r81577 >
Date:22:55, 5 February 2011
Author:hashar
Status:reverted (Comments)
Tags:
Comment:
Avoid code duplication for Language::isValidCode

r81335 changed the way we validate language code by introducing:
strcspn( $code, "/\\\000" ) !== strlen( $code )

That code was later made a function in r81340 but some conditional tests were
not updated to reflect this change.
Modified paths:
  • /trunk/phase3/languages/Language.php (modified) (history)

Diff [purge]

Index: trunk/phase3/languages/Language.php
——@@ -156,9 +156,7 @@
157157 static $recursionLevel = 0;
158158
159159 // Protect against path traversal below
160 - if ( !Language::isValidCode( $code )
161 - || strcspn( $code, "/\\\000" ) !== strlen( $code ) )
162 - {
 160+ if ( !Language::isValidCode( $code ) ) {
163161 throw new MWException( "Invalid language code \"$code\"" );
164162 }
165163
——@@ -2829,9 +2827,7 @@
28302828 */
28312829 static function getFileName( $prefix = 'Language', $code, $suffix = '.php' ) {
28322830 // Protect against path traversal
2833 - if ( !Language::isValidCode( $code )
2834 - || strcspn( $code, "/\\\000" ) !== strlen( $code ) )
2835 - {
 2831+ if ( !Language::isValidCode( $code ) ) {
28362832 throw new MWException( "Invalid language code \"$code\"" );
28372833 }
28382834

Follow-up revisions

RevisionCommit summaryAuthorDate
r82925Revert r81576: The fact that there are two checks, one close to the inclusion...tstarling02:35, 28 February 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r81335(bug 27094) fix path traversal vulnerabilitytstarling22:43, 1 February 2011
r81340Relax Language::isValidCode() to avoid breaking uselang hackststarling22:54, 1 February 2011

Comments

#Comment by Tim Starling (talk | contribs)   02:34, 28 February 2011

The fact that there are two checks, one close to the inclusion and one exposed to the user, was a deliberate security measure. Instead, Language::isValidCode() should be made stricter, so that it will be different to the other check, avoiding duplication. Also, it would be nice if a stricter regex were applied to class file inclusion than to language object creation, to support uselang hacks.

I'm reverting this.

Status & tagging log