From 07efbeb8aedc1c04a3bc2a88cbb86b8d36bc15ec Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 17 Aug 2009 13:23:45 +0000 Subject: [PATCH] * Fixed XSS vulnerability introduced by r49833. Only pre-release versions of MediaWiki were affected. * Refactored the IE script entry point security check into WebRequest::isPathInfoBad(). Use the standard CGI variable PATH_INFO to do this check instead of the various potential non-standard solutions. Made the check fairly permissive to avoid a repeat of bug 13049 due to broken CGI setups especially with cgi.fix_pathinfo=0. This should theoretically be very portable and secure, but I have not tested it widely. * Removed Chris Wrinn from the credits since his patch was wrong and has been removed. * Made the error message more informative. --- CREDITS | 1 - RELEASE-NOTES | 2 ++ api.php | 12 +++--------- includes/RawPage.php | 15 +++------------ includes/WebRequest.php | 31 +++++++++++++++++++++++++++++++ mwScriptLoader.php | 5 +++-- 6 files changed, 42 insertions(+), 24 deletions(-) diff --git a/CREDITS b/CREDITS index bbdf82da25..3016f485e6 100644 --- a/CREDITS +++ b/CREDITS @@ -63,7 +63,6 @@ following names for their contribution to the product. * Brent G * Brianna Laugher * Carlin -* Chris Wrinn * church of emacs * Dan Nessett * Daniel Arnold diff --git a/RELEASE-NOTES b/RELEASE-NOTES index 6582dc1274..58ad369291 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -406,6 +406,8 @@ this. Was used when mwEmbed was going to be an extension. * (bug 20273) Fix broken output when no pages are found in the content namespaces * (bug 20265) Make AncientPages and UnusedFiles work on SQLite +* Fixed XSS vulnerability for Internet Explorer clients (only pre-release + versions of MediaWiki were affected). == API changes in 1.16 == diff --git a/api.php b/api.php index ec805fcceb..16657ca9de 100644 --- a/api.php +++ b/api.php @@ -49,16 +49,10 @@ wfProfileIn('api.php'); // which will end up triggering HTML detection and execution, hence // XSS injection and all that entails. // -// Ensure that all access is through the canonical entry point... -// -if( isset( $_SERVER['SCRIPT_NAME'] ) ) { - $url = $_SERVER['SCRIPT_NAME']; -} else { - $url = $_SERVER['URL']; -} -if( strcmp( "$wgScriptPath/api$wgScriptExtension", $url ) ) { +if( $wgRequest->isPathInfoBad() ) { wfHttpError( 403, 'Forbidden', - 'API must be accessed through the primary script entry point.' ); + 'Invalid file extension found in PATH_INFO. ' . + 'The API must be accessed through the primary script entry point.' ); return; } diff --git a/includes/RawPage.php b/includes/RawPage.php index ea698bce73..3e38144b16 100644 --- a/includes/RawPage.php +++ b/includes/RawPage.php @@ -109,19 +109,9 @@ class RawPage { } function view() { - global $wgOut, $wgScript; - - $url = wfGetScriptUrl(); - if( $url == '' ) { - # This will make the next check fail with a confusing error - # message, so we should mention it separately. - wfHttpError( 500, 'Internal Server Error', - "\$_SERVER['URL'] is not set. Perhaps you're using CGI" . - " and haven't set cgi.fix_pathinfo = 1 in php.ini?" ); - return; - } + global $wgOut, $wgScript, $wgRequest; - if( strcmp( $wgScript, $url ) ) { + 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 @@ -135,6 +125,7 @@ class RawPage { # # Just return a 403 Forbidden and get it over with. wfHttpError( 403, 'Forbidden', + 'Invalid file extension found in PATH_INFO. ' . 'Raw pages must be accessed through the primary script entry point.' ); return; } diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 784dc0fab1..157f09305d 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -670,6 +670,33 @@ class WebRequest { public function setSessionData( $key, $data ) { $_SESSION[$key] = $data; } + + /** + * 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 + * is not HTML but may be detected as such. + * + * Various past attempts to use the URL to make this check have generally + * run up against the fact that CGI does not provide a standard method to + * determine the URL. PATH_INFO may be mangled (e.g. if cgi.fix_pathinfo=0), + * but only by prefixing it with the script name and maybe some other stuff, + * the extension is not mangled. So this should be a reasonably portable + * way to perform this security check. + */ + public function isPathInfoBad() { + global $wgScriptExtension; + + 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' ) ); + } } /** @@ -740,4 +767,8 @@ class FauxRequest extends WebRequest { $this->notImplemented( __METHOD__ ); } + public function isPathInfoBad() { + return false; + } + } diff --git a/mwScriptLoader.php b/mwScriptLoader.php index 8637cc8cd4..c8841274a8 100644 --- a/mwScriptLoader.php +++ b/mwScriptLoader.php @@ -30,8 +30,9 @@ require_once('includes/WebStart.php'); wfProfileIn( 'mwScriptLoader.php' ); -if( strpos( wfGetScriptUrl(), "mwScriptLoader{$wgScriptExtension}" ) === false ){ +if( $wgRequest->isPathInfoBad() ){ wfHttpError( 403, 'Forbidden', + 'Invalid file extension found in PATH_INFO. ' . 'mwScriptLoader must be accessed through the primary script entry point.' ); return; } @@ -51,4 +52,4 @@ if ( !$wgEnableScriptLoader ) { $myScriptLoader = new jsScriptLoader(); $myScriptLoader->doScriptLoader(); -wfProfileOut( 'mwScriptLoader.php' ); \ No newline at end of file +wfProfileOut( 'mwScriptLoader.php' ); -- 2.20.1