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
Use RequestContext::sanitizeLangCode() on lang parameter

Attached:

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

stephen.donner wrote:

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

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

(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

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

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

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:

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