Last modified: 2013-09-17 00:30:08 UTC
Filing under security here b/c it was filed under security upstream. Upstream bug in URL. https://test.wikipedia.org/w/load.php?lang=%3C&modules=startup https://test.wikipedia.org/w/load.php?lang=%3C&modules=startup&only=scripts https://en.wikipedia.org/w/load.php?lang=%3C&modules=startup Above URLs give exceptions like the following: /* exception 'MWException' with message 'Invalid language code "<"' in /home/wikipedia/common/php-1.21wmf12/languages/Language.php:210 Stack trace: #0 /home/wikipedia/common/php-1.21wmf12/languages/Language.php(189): Language::newFromCode('<') #1 /home/wikipedia/common/php-1.21wmf12/includes/resourceloader/ResourceLoaderContext.php(164): Language::factory('<') #2 /home/wikipedia/common/php-1.21wmf12/includes/resourceloader/ResourceLoaderContext.php(239): ResourceLoaderContext->getDirection() #3 /home/wikipedia/common/php-1.21wmf12/includes/resourceloader/ResourceLoaderStartUpModule.php(251): ResourceLoaderContext->getHash() #4 /home/wikipedia/common/php-1.21wmf12/includes/resourceloader/ResourceLoader.php(479): ResourceLoaderStartUpModule->getModifiedTime(Object(ResourceLoaderContext)) #5 /home/wikipedia/common/php-1.21wmf12/load.php(47): ResourceLoader->respond(Object(ResourceLoaderContext)) #6 /usr/local/apache/common-local/live-1.5/load.php(3): require('/home/wikipedia...') #7 {main} */ Adding only=scripts results in two stack traces being printed.
Thanks for the report Mark. I don't have access to the Mozilla bug to see if they found a way, but it doesn't seem like this is exploitable. It looks like we don't do the sanitization on that parameter that we use for languages in other places. Maybe we should run it through RequestContext::sanitizeLangCode()?
Created attachment 11954 [details] Use RequestContext::sanitizeLangCode() on lang parameter
(In reply to comment #1) > Thanks for the report Mark. I don't have access to the Mozilla bug to see if > they found a way, but it doesn't seem like this is exploitable. I think this is mostly an information leak. If you register on that bugzilla, I can add you to the CC list for the bug. > It looks like we don't do the sanitization on that parameter that we use for > languages in other places. Maybe we should run it through > RequestContext::sanitizeLangCode()? The interesting thing is that setting the lang= parameter to anything without an angle bracket doesn't result in an exception.
(In reply to comment #3) > I think this is mostly an information leak. If you register on that > bugzilla, > I can add you to the CC list for the bug. I just did (csteipp@wikimedia.org). Please cc me if you can.
(In reply to comment #4) > I just did (csteipp@wikimedia.org). Please cc me if you can. Done!
I tested this with IE6 (probably the worst content sniffer), and wasn't able to get it to trigger javascript execution. That said, with the unescaped code reflected onto the page, there may be a browser somewhere that tries to interpret it as html, which would cause an xss. The fix should be pretty easy. I'll try and get that together soon.
Tim, it looks like you wrote that bit of ResourceLoaderContext.php. Could you look at the patch and see if that seems like the right way to handle it?
(In reply to comment #7) > Tim, it looks like you wrote that bit of ResourceLoaderContext.php. Could you > look at the patch and see if that seems like the right way to handle it? It doesn't seem right to me to report every exception as a separate security vulnerability, and to fix each by not throwing an exception. If there is an XSS vulnerability, it should be fixed in ResourceLoader::makeComment(). But I doubt there is one, since we have heavily-studied instances of HTML tags being output in JavaScript which have been deemed to be safe, in bug 34257. As for the path disclosure, it seems to me that that should be fixed by replacing all of the $this->makeComment( $exception->__toString() ) in ResourceLoader.php with: $this->formatException( $exception ) where ResourceLoader::formatException() would respect $wgShowExceptionDetails.
Created attachment 12429 [details] Only display exception if $wgShowExceptionDetails is true Fixes the path disclosure portion of this bug when wgShowExceptionDetails is false.
Created attachment 12430 [details] Only display exception if $wgShowExceptionDetails is true Actually, looking at the exceptions that can be thrown here, I think it should be ok to show some details of the error.
(In reply to comment #10) > Created attachment 12430 [details] > Only display exception if $wgShowExceptionDetails is true > > Actually, looking at the exceptions that can be thrown here, I think it > should be ok to show some details of the error. Generally, we have interpreted $wgShowExceptionDetails=false to mean not even the message should be shown. The policy was introduced by Brion -- I'm not personally attached to it, but I've added him to the CC list in case he wants to defend it. I'm happy with either patch. But maybe in a public followup change on Gerrit, getTraceAsString() could be added to the output in the $wgShowExceptionDetails=true case, to be consistent with the doc comment of $wgShowExceptionDetails?
Created attachment 13112 [details] Only display exception if $wgShowExceptionDetails is true Only show generic message when $wgShowExceptionDetails is false. Updated to patch against master.
Created attachment 13113 [details] Only display exception if $wgShowExceptionDetails is true Updated to public static method, in case anyone is using makeComment to show errors (since Ibe45256eddee2ad30d19adcb2f1c0b0d5578e650)
Created attachment 13114 [details] Only display exception if $wgShowExceptionDetails is true ... wrong version of the old patch. Sorry about that. This one should work better.
Brion looked at the current patch and said, <brion> csteipp: patch looks reasonable, but i haven't tested it He also commented about the policy Tim brought up in comment #11 (below). I'll go ahead and deploy this, and we'll release it with 1.21.2. (In reply to comment #11) > Generally, we have interpreted $wgShowExceptionDetails=false to mean not even > the message should be shown. The policy was introduced by Brion -- I'm not > personally attached to it, but I've added him to the CC list in case he wants > to defend it. <brion> csteipp: main intention is 'don't show details that may include file paths, usernames, passwords, or other exciting internal strings' <brion> wouldn't hurt to show the exception *type* i think <brion> but we also don't want the message to be *too* scary/technical
This issues was assigned CVE-2013-4301
