From 97ff30ddb42219931ebeac2ecbc2a940acf356b8 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Fri, 3 Jun 2011 05:32:51 +0000 Subject: [PATCH] (bug 28840) If the query string hits bug 28235, redirect to a safer URL instead of showing an unhelpful error message. IE 6 will only use the extension of the final destination for its cache filename. --- api.php | 13 +----- includes/RawPage.php | 18 +------- includes/WebRequest.php | 100 +++++++++++++++++++++++++++++++++++++++- load.php | 12 +---- 4 files changed, 103 insertions(+), 40 deletions(-) diff --git a/api.php b/api.php index 978e8d4e94..7fff04803a 100644 --- a/api.php +++ b/api.php @@ -61,18 +61,7 @@ wfProfileIn( 'api.php' ); $starttime = microtime( true ); // URL safety checks -// -// See RawPage.php for details; summary is that MSIE can override the -// Content-Type if it sees a recognized extension on the URL, such as -// might be appended via PATH_INFO after 'api.php'. -// -// Some data formats can end up containing unfiltered user-provided data -// which will end up triggering HTML detection and execution, hence -// XSS injection and all that entails. -// -if ( $wgRequest->isPathInfoBad() ) { - wfHttpError( 403, 'Forbidden', - 'Invalid file extension found in PATH_INFO or QUERY_STRING.' ); +if ( !$wgRequest->checkUrlExtension() ) { return; } diff --git a/includes/RawPage.php b/includes/RawPage.php index 5804bf9bcb..c6c4a66f7f 100644 --- a/includes/RawPage.php +++ b/includes/RawPage.php @@ -118,22 +118,8 @@ class RawPage { function view() { global $wgOut, $wgRequest; - if( $wgRequest->isPathInfoBad() ) { - # Internet Explorer will ignore the Content-Type header if it - # thinks it sees a file extension it recognizes. Make sure that - # all raw requests are done through the script node, which will - # have eg '.php' and should remain safe. - # - # We used to redirect to a canonical-form URL as a general - # backwards-compatibility / good-citizen nice thing. However - # a lot of servers are set up in buggy ways, resulting in - # redirect loops which hang the browser until the CSS load - # times out. - # - # Just return a 403 Forbidden and get it over with. - wfHttpError( 403, 'Forbidden', - 'Invalid file extension found in PATH_INFO or QUERY_STRING. ' . - 'Raw pages must be accessed through the primary script entry point.' ); + if( !$wgRequest->checkUrlExtension() ) { + $wgOut->disable(); return; } diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 68cb73e5a0..9ede547dd9 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -766,6 +766,69 @@ class WebRequest { $_SESSION[$key] = $data; } + /** + * Check if Internet Explorer will detect an incorrect cache extension in + * PATH_INFO or QUERY_STRING. If the request can't be allowed, show an error + * 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 ) ) { + if ( !$this->wasPosted() ) { + if ( $this->attemptExtensionSecurityRedirect() ) { + return false; + } + } + wfHttpError( 403, 'Forbidden', + 'Invalid file extension found in the query string.' ); + + return false; + } + + if ( $this->isPathInfoBad() ) { + wfHttpError( 403, 'Forbidden', + 'Invalid file extension found in PATH_INFO.' ); + return false; + } + return true; + } + + /** + * 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; + } + + header( 'Location: ' . $url ); + header( 'Content-Type: text/html' ); + $encUrl = htmlspecialchars( $url ); + echo << + +Security redirect + + +

Security redirect

+

+We can't serve non-HTML content from the URL you have requested, because +Internet Explorer would interpret it as an incorrect and potentially dangerous +content type.

+

Instead, please use this URL, which is the same as the URL you have requested, except that +"&*" is appended. This prevents Internet Explorer from seeing a bogus file +extension. +

+ + +HTML; + echo "\n"; + return true; + } + /** * Returns true if the PATH_INFO ends with an extension other than a script * extension. This could confuse IE for scripts that send arbitrary data which @@ -884,8 +947,18 @@ class WebRequest { 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( $_SERVER['QUERY_STRING'] ); + $extension = self::findIE6Extension( $query ); if ( strval( $extension ) === '' ) { /* No extension or empty extension (false/'') */ return false; @@ -901,6 +974,31 @@ class WebRequest { 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; + } + /** * Parse the Accept-Language header sent by the client into an array * @return array( languageCode => q-value ) sorted by q-value in descending order diff --git a/load.php b/load.php index 2fc48fc950..74af68f3e9 100644 --- a/load.php +++ b/load.php @@ -45,17 +45,7 @@ if ( isset( $_SERVER['MW_COMPILED'] ) ) { wfProfileIn( 'load.php' ); // URL safety checks -// -// See RawPage.php for details; summary is that MSIE can override the -// Content-Type if it sees a recognized extension on the URL, such as -// might be appended via PATH_INFO after 'load.php'. -// -// Some resources can contain HTML-like strings (e.g. in messages) -// which will end up triggering HTML detection and execution. -// -if ( $wgRequest->isPathInfoBad() ) { - wfHttpError( 403, 'Forbidden', - 'Invalid file extension found in PATH_INFO or QUERY_STRING.' ); +if ( !$wgRequest->checkUrlExtension() ) { return; } -- 2.20.1