From f3f1fcdc2c6b034d832c960301dc8abece5c767f Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 6 Jun 2011 11:59:20 +0000 Subject: [PATCH] * Added a REQUEST_URI check to the bug 28235 handling. * Moved most of the bug 28235 code out to a separate library class, since I was running out of distinct function names. * Merged the QUERY_STRING and PATH_INFO security checks, since they are dealing with the exact same problem. Removed WebRequest::isQueryStringBad(). * Deal with img_auth.php by having it specify what extension it expects to be streaming out. This extension can then be compared with the extension that IE might detect. --- img_auth.php | 16 +- includes/AutoLoader.php | 1 + includes/WebRequest.php | 189 ++------------ includes/libs/IEUrlExtension.php | 247 ++++++++++++++++++ .../phpunit/includes/FindIE6ExtensionTest.php | 32 +-- 5 files changed, 295 insertions(+), 190 deletions(-) create mode 100644 includes/libs/IEUrlExtension.php diff --git a/img_auth.php b/img_auth.php index ac3f50e93c..8ea7b01a7b 100644 --- a/img_auth.php +++ b/img_auth.php @@ -42,14 +42,20 @@ if ( $wgImgAuthPublicTest wfForbidden('img-auth-accessdenied','img-auth-public'); } +$matches = WebRequest::getPathInfo(); +$path = $matches['title']; + // Check for bug 28235: QUERY_STRING overriding the correct extension -if ( $wgRequest->isQueryStringBad() ) +$dotPos = strpos( $path, '.' ); +$whitelist = array(); +if ( $dotPos !== false ) { + $whitelist[] = substr( $path, $dotPos + 1 ); +} +if ( !$wgRequest->checkUrlExtension( $whitelist ) ) { - wfForbidden( 'img-auth-accessdenied', 'img-auth-bad-query-string' ); -} + return; +} -$matches = WebRequest::getPathInfo(); -$path = $matches['title']; $filename = realpath( $wgUploadDirectory . $path ); $realUpload = realpath( $wgUploadDirectory ); diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php index edf918f5ca..5116d35e52 100644 --- a/includes/AutoLoader.php +++ b/includes/AutoLoader.php @@ -506,6 +506,7 @@ $wgAutoloadLocalClasses = array( 'CSSJanus' => 'includes/libs/CSSJanus.php', 'CSSMin' => 'includes/libs/CSSMin.php', 'IEContentAnalyzer' => 'includes/libs/IEContentAnalyzer.php', + 'IEUrlExtension' => 'includes/libs/IEUrlExtension.php', 'JavaScriptMinifier' => 'includes/libs/JavaScriptMinifier.php', # includes/media diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 53881efc37..b80ebeb7f5 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -772,25 +772,23 @@ class WebRequest { * message or redirect to a safer URL. Returns true if the URL is OK, and * false if an error message has been shown and the request should be aborted. */ - public function checkUrlExtension() { - $query = isset( $_SERVER['QUERY_STRING'] ) ? $_SERVER['QUERY_STRING'] : ''; - if ( self::isUrlExtensionBad( $query ) ) { + public function checkUrlExtension( $extWhitelist = array() ) { + global $wgScriptExtension; + $extWhitelist[] = ltrim( $wgScriptExtension, '.' ); + if ( IEUrlExtension::areServerVarsBad( $_SERVER, $extWhitelist ) ) { if ( !$this->wasPosted() ) { - if ( $this->attemptExtensionSecurityRedirect() ) { + $newUrl = IEUrlExtension::fixUrlForIE6( + $this->getFullRequestURL(), $extWhitelist ); + if ( $newUrl !== false ) { + $this->doSecurityRedirect( $newUrl ); return false; } } wfHttpError( 403, 'Forbidden', - 'Invalid file extension found in the query string.' ); + 'Invalid file extension found in the path info or query string.' ); return false; } - - if ( $this->isPathInfoBad() ) { - wfHttpError( 403, 'Forbidden', - 'Invalid file extension found in PATH_INFO.' ); - return false; - } return true; } @@ -798,12 +796,7 @@ class WebRequest { * Attempt to redirect to a URL with a QUERY_STRING that's not dangerous in * IE 6. Returns true if it was successful, false otherwise. */ - protected function attemptExtensionSecurityRedirect() { - $url = self::fixUrlForIE6( $this->getFullRequestURL() ); - if ( $url === false ) { - return false; - } - + protected function doSecurityRedirect( $url ) { header( 'Location: ' . $url ); header( 'Content-Type: text/html' ); $encUrl = htmlspecialchars( $url ); @@ -844,159 +837,13 @@ HTML; * Also checks for anything that looks like a file extension at the end of * QUERY_STRING, since IE 6 and earlier will use this to get the file type * if there was no dot before the question mark (bug 28235). - */ - public function isPathInfoBad() { - global $wgScriptExtension; - - if ( $this->isQueryStringBad() ) { - return true; - } - - if ( !isset( $_SERVER['PATH_INFO'] ) ) { - return false; - } - $pi = $_SERVER['PATH_INFO']; - $dotPos = strrpos( $pi, '.' ); - if ( $dotPos === false ) { - return false; - } - $ext = substr( $pi, $dotPos ); - return !in_array( $ext, array( $wgScriptExtension, '.php', '.php5' ) ); - } - - /** - * Determine what extension IE6 will infer from a certain query string. - * If the URL has an extension before the question mark, IE6 will use - * that and ignore the query string, but per the comment at - * isPathInfoBad() we don't have a reliable way to determine the URL, - * so isPathInfoBad() just passes in the query string for $url. - * All entry points have safe extensions (php, php5) anyway, so - * checking the query string is possibly overly paranoid but never - * insecure. * - * The criteria for finding an extension are as follows: - * - a possible extension is a dot followed by one or more characters not - * in <>\"/:|?.# - * - if we find a possible extension followed by the end of the string or - * a #, that's our extension - * - if we find a possible extension followed by a ?, that's our extension - * - UNLESS it's exe, dll or cgi, in which case we ignore it and continue - * searching for another possible extension - * - if we find a possible extension followed by a dot or another illegal - * character, we ignore it and continue searching - * - * @param $url string URL - * @return mixed Detected extension (string), or false if none found - */ - public static function findIE6Extension( $url ) { - $pos = 0; - $hashPos = strpos( $url, '#' ); - if ( $hashPos !== false ) { - $urlLength = $hashPos; - } else { - $urlLength = strlen( $url ); - } - $remainingLength = $urlLength; - while ( $remainingLength > 0 ) { - // Skip ahead to the next dot - $pos += strcspn( $url, '.', $pos, $remainingLength ); - if ( $pos >= $urlLength ) { - // End of string, we're done - return false; - } - - // We found a dot. Skip past it - $pos++; - $remainingLength = $urlLength - $pos; - - // Check for illegal characters in our prospective extension, - // or for another dot - $nextPos = $pos + strcspn( $url, "<>\\\"/:|?*.", $pos, $remainingLength ); - if ( $nextPos >= $urlLength ) { - // No illegal character or next dot - // We have our extension - return substr( $url, $pos, $urlLength - $pos ); - } - if ( $url[$nextPos] === '?' ) { - // We've found a legal extension followed by a question mark - // If the extension is NOT exe, dll or cgi, return it - $extension = substr( $url, $pos, $nextPos - $pos ); - if ( strcasecmp( $extension, 'exe' ) && strcasecmp( $extension, 'dll' ) && - strcasecmp( $extension, 'cgi' ) ) - { - return $extension; - } - // Else continue looking - } - // We found an illegal character or another dot - // Skip to that character and continue the loop - $pos = $nextPos + 1; - $remainingLength = $urlLength - $pos; - } - return false; - } - - /** - * Check for a bad query string, which IE 6 will use as a potentially - * insecure cache file extension. See bug 28235. Returns true if the - * request should be disallowed. - * - * @return Boolean + * @deprecated Use checkUrlExtension(). */ - public function isQueryStringBad() { - if ( !isset( $_SERVER['QUERY_STRING'] ) ) { - return false; - } - return self::isUrlExtensionBad( $_SERVER['QUERY_STRING'] ); - } - - /** - * The same as WebRequest::isQueryStringBad() except as a static function. - */ - public static function isUrlExtensionBad( $query ) { - if ( strval( $query ) === '' ) { - return false; - } - - $extension = self::findIE6Extension( $query ); - if ( strval( $extension ) === '' ) { - /* No extension or empty extension (false/'') */ - return false; - } - - /* Only consider the extension understood by IE to be potentially - * dangerous if it is made of normal characters (so it is more - * likely to be registered with an application) - * Compromise with api.php convenience. Considers for instance - * that no sane application will register a dangerous file type - * in an extension containing an ampersand. - */ - return (bool)preg_match( '/^[a-zA-Z0-9_-]+$/', $extension ); - } - - /** - * Returns a variant of $url which will pass isUrlExtensionBad() but has the - * same GET parameters, or false if it can't figure one out. - */ - public static function fixUrlForIE6( $url ) { - $questionPos = strpos( $url, '?' ); - if ( $questionPos === false || $questionPos === strlen( $url ) - 1 ) { - return $url; - } - - $beforeQuery = substr( $url, 0, $questionPos + 1 ); - $query = substr( $url, $questionPos + 1 ); - // Multiple question marks cause problems. Encode the second and - // subsequent question mark. - $query = str_replace( '?', '%3E', $query ); - // Append an invalid path character so that IE6 won't see the end of the - // query string as an extension - $query .= '&*'; - if ( self::isUrlExtensionBad( $query ) ) { - // Avoid a redirect loop - return false; - } - return $beforeQuery . $query; + public function isPathInfoBad( $extWhitelist = array() ) { + global $wgScriptExtension; + $extWhitelist[] = ltrim( $wgScriptExtension, '.' ); + return IEUrlExtension::areServerVarsBad( $_SERVER, $extWhitelist ); } /** @@ -1235,7 +1082,11 @@ class FauxRequest extends WebRequest { return $this->session; } - public function isPathInfoBad() { + public function isPathInfoBad( $extWhitelist = array() ) { return false; } + + public function checkUrlExtension( $extWhitelist = array() ) { + return true; + } } diff --git a/includes/libs/IEUrlExtension.php b/includes/libs/IEUrlExtension.php new file mode 100644 index 0000000000..100454d450 --- /dev/null +++ b/includes/libs/IEUrlExtension.php @@ -0,0 +1,247 @@ +\"/:|?.# + * - if we find a possible extension followed by the end of the string or + * a #, that's our extension + * - if we find a possible extension followed by a ?, that's our extension + * - UNLESS it's exe, dll or cgi, in which case we ignore it and continue + * searching for another possible extension + * - if we find a possible extension followed by a dot or another illegal + * character, we ignore it and continue searching + * + * @param $url string URL + * @return mixed Detected extension (string), or false if none found + */ + public static function findIE6Extension( $url ) { + $pos = 0; + $hashPos = strpos( $url, '#' ); + if ( $hashPos !== false ) { + $urlLength = $hashPos; + } else { + $urlLength = strlen( $url ); + } + $remainingLength = $urlLength; + while ( $remainingLength > 0 ) { + // Skip ahead to the next dot + $pos += strcspn( $url, '.', $pos, $remainingLength ); + if ( $pos >= $urlLength ) { + // End of string, we're done + return false; + } + + // We found a dot. Skip past it + $pos++; + $remainingLength = $urlLength - $pos; + + // Check for illegal characters in our prospective extension, + // or for another dot + $nextPos = $pos + strcspn( $url, "<>\\\"/:|?*.", $pos, $remainingLength ); + if ( $nextPos >= $urlLength ) { + // No illegal character or next dot + // We have our extension + return substr( $url, $pos, $urlLength - $pos ); + } + if ( $url[$nextPos] === '?' ) { + // We've found a legal extension followed by a question mark + // If the extension is NOT exe, dll or cgi, return it + $extension = substr( $url, $pos, $nextPos - $pos ); + if ( strcasecmp( $extension, 'exe' ) && strcasecmp( $extension, 'dll' ) && + strcasecmp( $extension, 'cgi' ) ) + { + return $extension; + } + // Else continue looking + } + // We found an illegal character or another dot + // Skip to that character and continue the loop + $pos = $nextPos + 1; + $remainingLength = $urlLength - $pos; + } + return false; + } + + /** + * When passed the value of $_SERVER['SERVER_SOFTWARE'], this function + * returns true if that server is known to have a REQUEST_URI variable + * with %2E not decoded to ".". On such a server, it is possible to detect + * whether the script filename has been obscured. + * + * The function returns false if the server is not known to have this + * behaviour. Microsoft IIS in particular is known to decode escaped script + * filenames. + * + * SERVER_SOFTWARE typically contains either a plain string such as "Zeus", + * or a specification in the style of a User-Agent header, such as + * "Apache/1.3.34 (Unix) mod_ssl/2.8.25 OpenSSL/0.9.8a PHP/4.4.2" + * + * @param $serverSoftware + * @return bool + * + */ + public static function haveUndecodedRequestUri( $serverSoftware ) { + static $whitelist = array( + 'Apache', + 'Zeus', + 'LiteSpeed' ); + if ( preg_match( '/^(.*?)($|\/| )/', $serverSoftware, $m ) ) { + return in_array( $m[1], $whitelist ); + } else { + return false; + } + } + +} diff --git a/tests/phpunit/includes/FindIE6ExtensionTest.php b/tests/phpunit/includes/FindIE6ExtensionTest.php index 534f0980dd..c6270e907d 100644 --- a/tests/phpunit/includes/FindIE6ExtensionTest.php +++ b/tests/phpunit/includes/FindIE6ExtensionTest.php @@ -1,13 +1,13 @@ assertEquals( 'y', - WebRequest::findIE6Extension( 'x.y' ), + IEUrlExtension::findIE6Extension( 'x.y' ), 'Simple extension' ); } @@ -15,7 +15,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testSimpleNoExt() { $this->assertEquals( '', - WebRequest::findIE6Extension( 'x' ), + IEUrlExtension::findIE6Extension( 'x' ), 'No extension' ); } @@ -23,7 +23,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testEmpty() { $this->assertEquals( '', - WebRequest::findIE6Extension( '' ), + IEUrlExtension::findIE6Extension( '' ), 'Empty string' ); } @@ -31,7 +31,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testQuestionMark() { $this->assertEquals( '', - WebRequest::findIE6Extension( '?' ), + IEUrlExtension::findIE6Extension( '?' ), 'Question mark only' ); } @@ -39,7 +39,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testExtQuestionMark() { $this->assertEquals( 'x', - WebRequest::findIE6Extension( '.x?' ), + IEUrlExtension::findIE6Extension( '.x?' ), 'Extension then question mark' ); } @@ -47,7 +47,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testQuestionMarkExt() { $this->assertEquals( 'x', - WebRequest::findIE6Extension( '?.x' ), + IEUrlExtension::findIE6Extension( '?.x' ), 'Question mark then extension' ); } @@ -55,7 +55,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testInvalidChar() { $this->assertEquals( '', - WebRequest::findIE6Extension( '.x*' ), + IEUrlExtension::findIE6Extension( '.x*' ), 'Extension with invalid character' ); } @@ -63,7 +63,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testInvalidCharThenExtension() { $this->assertEquals( 'x', - WebRequest::findIE6Extension( '*.x' ), + IEUrlExtension::findIE6Extension( '*.x' ), 'Invalid character followed by an extension' ); } @@ -71,7 +71,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testMultipleQuestionMarks() { $this->assertEquals( 'c', - WebRequest::findIE6Extension( 'a?b?.c?.d?e?f' ), + IEUrlExtension::findIE6Extension( 'a?b?.c?.d?e?f' ), 'Multiple question marks' ); } @@ -79,7 +79,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testExeException() { $this->assertEquals( 'd', - WebRequest::findIE6Extension( 'a?b?.exe?.d?.e' ), + IEUrlExtension::findIE6Extension( 'a?b?.exe?.d?.e' ), '.exe exception' ); } @@ -87,7 +87,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testExeException2() { $this->assertEquals( 'exe', - WebRequest::findIE6Extension( 'a?b?.exe' ), + IEUrlExtension::findIE6Extension( 'a?b?.exe' ), '.exe exception 2' ); } @@ -95,7 +95,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testHash() { $this->assertEquals( '', - WebRequest::findIE6Extension( 'a#b.c' ), + IEUrlExtension::findIE6Extension( 'a#b.c' ), 'Hash character preceding extension' ); } @@ -103,7 +103,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testHash2() { $this->assertEquals( '', - WebRequest::findIE6Extension( 'a?#b.c' ), + IEUrlExtension::findIE6Extension( 'a?#b.c' ), 'Hash character preceding extension 2' ); } @@ -111,7 +111,7 @@ class FindIE6ExtensionTest extends MediaWikiTestCase { function testDotAtEnd() { $this->assertEquals( '', - WebRequest::findIE6Extension( '.' ), + IEUrlExtension::findIE6Extension( '.' ), 'Dot at end of string' ); } -- 2.20.1