Closed Bug 822568 Opened 12 years ago Closed 12 years ago

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

Categories

(Webtools Graveyard :: MXR, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: reed, Assigned: reed)

References

()

Details

(Keywords: reporter-external, wsec-xss)

Attachments

(1 file, 1 obsolete file)

Ahmed Hassan <ahmed.hassan@star-ware.com> 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)>
Attached patch patch - v1 (obsolete) — Splinter Review
Attachment #693275 - Flags: review?(justdave)
Flags: sec-bounty?
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.
Attachment #693275 - Flags: review?(justdave) → review-
s/feeds/feels/
Attached patch patch - v2Splinter Review
Attachment #693275 - Attachment is obsolete: true
Attachment #693278 - Flags: review?(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.
Attachment #693278 - Flags: review?(justdave) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Do we need to kick someone to get that deployed or does it auto-update these days?
(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.
Confirmed fixed on production.
Status: RESOLVED → VERIFIED
Group: webtools-security
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 !
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!
Flags: sec-bounty? → sec-bounty-
Adding keywords to bugs for metrics, no action required. Sorry about bugmail spam.
Keywords: wsec-xss
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: