using "<" in as a value in the lang query string throws exception
Closed, ResolvedPublic

Description

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.


Version: unspecified
Severity: normal
URL: https://bugzilla.mozilla.org/show_bug.cgi?id=852611

bzimport added a subscriber: wikibugs-l.
bzimport set Reference to bz46332.
MarkAHershberger created this task.Via LegacyMar 19 2013, 7:02 PM
csteipp added a comment.Via ConduitMar 19 2013, 11:29 PM

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()?

csteipp added a comment.Via ConduitMar 19 2013, 11:38 PM

Created attachment 11954
Use RequestContext::sanitizeLangCode() on lang parameter

Attached: bug46332.patch

MarkAHershberger added a comment.Via ConduitMar 20 2013, 1:10 PM

(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.

csteipp added a comment.Via ConduitMar 23 2013, 12:21 AM

(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.

bzimport added a comment.Via ConduitMar 23 2013, 12:28 AM

stephen.donner wrote:

(In reply to comment #4)

I just did (csteipp@wikimedia.org). Please cc me if you can.

Done!

csteipp added a comment.Via ConduitMar 25 2013, 5:45 PM

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.

csteipp added a comment.Via ConduitMar 26 2013, 10:14 PM

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?

tstarling added a comment.Via ConduitMar 26 2013, 10:29 PM

(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.

csteipp added a comment.Via ConduitMay 30 2013, 11:01 PM

Created attachment 12429
Only display exception if $wgShowExceptionDetails is true

Fixes the path disclosure portion of this bug when wgShowExceptionDetails is false.

attachment bug46332.patch ignored as obsolete

csteipp added a comment.Via ConduitMay 30 2013, 11:16 PM

Created attachment 12430
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.

attachment bug46332.patch ignored as obsolete

tstarling added a comment.Via ConduitAug 15 2013, 3:28 AM

(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?

attachment bug46332.patch ignored as obsolete

csteipp added a comment.Via ConduitAug 16 2013, 11:27 PM

Created attachment 13112
Only display exception if $wgShowExceptionDetails is true

Only show generic message when $wgShowExceptionDetails is false. Updated to patch against master.

attachment bug46332-b.patch ignored as obsolete

csteipp added a comment.Via ConduitAug 16 2013, 11:38 PM

Created attachment 13113
Only display exception if $wgShowExceptionDetails is true

Updated to public static method, in case anyone is using makeComment to show errors (since Ibe45256eddee2ad30d19adcb2f1c0b0d5578e650)

attachment bug46332-b.patch ignored as obsolete

csteipp added a comment.Via ConduitAug 16 2013, 11:58 PM

Created attachment 13114
Only display exception if $wgShowExceptionDetails is true

... wrong version of the old patch. Sorry about that. This one should work better.

Attached: bug46332-b.patch

csteipp added a comment.Via ConduitAug 28 2013, 7:59 PM

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

csteipp added a comment.Via ConduitSep 5 2013, 4:59 PM

This issues was assigned CVE-2013-4301

csteipp added a project: Security.Via WebMar 26 2015, 8:39 PM

Add Comment