822568 - XSS in the 'hitlimit' parameter of MXR's source

Status: VERIFIED FIXED

(Webtools Graveyard :: MXR, defect)

Description

[Reed Loden [:reed]]

Ahmed Hassan <[e-mail]> reported an XSS in MXR to security@:
==============================================================================

i would like to report a vulnerability that affect one of mozilla domains

the vulnerability is a reflected cross site scripting which could lead 
to run A malicious JavaScript or steal user cookies

Full detail for the bug :

Vulnerability affects the domain  : http://mxr.mozilla.org

Vulnerability exists in link : 
http://mxr.mozilla.org/labs-central/search?hitlimit=

Affected parameter : hitlimit

Example demonstrating the vulnerability : 
http://mxr.mozilla.org/labs-central/search?hitlimit=%22%3E%0A%3Cimg%20src=x%20onerror=alert%28document.cookie%29%3E

Payload used : ">%0A<img src=x onerror=alert(document.cookie)>

Comment 1

[Reed Loden [:reed]]

Attachment: patch - v1

Comment 2

[Dave Miller [:justdave]]

Comment on attachment 693275 [details] [diff] [review]
patch - v1

Review of attachment 693275 [details] [diff] [review]:
-----------------------------------------------------------------

::: search
@@ +608,5 @@
>  $filter =~ s/%(\w\w)/chr(hex $1)/ge;
>  
>  sub cleanHitlimit {
>    my $hitLimit = shift;
> +  $hitLimit =~ s/\D//g;

This will successfully remove the payload, but it still leaves the embedded linefeed intact.  I guess that's good as far as fixing the security issue, but it still feeds imcomplete.  a /s modifier (so make it /gs instead) would make it remove the linefeed as well.

Comment 3

[Dave Miller [:justdave]]

s/feeds/feels/

Comment 4

[Reed Loden [:reed]]

Attachment: patch - v2

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 693278 [details] [diff] [review]
patch - v2

Review of attachment 693278 [details] [diff] [review]:
-----------------------------------------------------------------

This works.

Just to point out, the original would have taken the first set of consecutive digits if you just added the /gs to it.  Your new one will take all available digits and smush them together.  So "ab2cd4ef6gr" would become "246" with the new code, the old code with a /gs added would have made it "2".  But this all sucks anyway, and it fixes the security issue and ensures the value is an integer at least.

Comment 6

[Reed Loden [:reed]]

https://hg.mozilla.org/webtools/mxr/rev/ec10dd9e8075

Comment 7

[Dave Miller [:justdave]]

Do we need to kick someone to get that deployed or does it auto-update these days?

Comment 8

[Reed Loden [:reed]]

(In reply to Dave Miller [:justdave] from comment #7)
> Do we need to kick someone to get that deployed or does it auto-update these
> days?

I filed bug 822576 to get it updated.

Comment 9

[Reed Loden [:reed]]

Confirmed fixed on production.

Comment 10

[Ahmed Hassan]

seems i came late in this discussion , but it was a great work from your part and the vulnerability got fixed now :)

just want to ask is the vulnerability eligible for a bug bounty ? or it's out from the scope !

Comment 11

[Raymond Forbes[:rforbes]]

this site is not on the eligible site list nor is it severe enough to warrant a bounty.  thank you for submitting though, and keep them coming!

Comment 12

[Yvan Boily [:ygjb][:yvan]]

Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.