Closed
Bug 1253263
(CVE-2016-2803)
Opened 8 years ago
Closed 8 years ago
[SECURITY] XSS vulnerability in dependency graphs via bug summary
Categories
(Bugzilla :: Dependency Views, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.4
People
(Reporter: jwkbugzilla, Assigned: LpSolit)
References
Details
(Keywords: sec-high, wsec-xss)
Attachments
(1 file, 1 obsolete file)
1.30 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
You can see this vulnerability in action on https://landfill.bugzilla.org/bugzilla-5.0-branch/showdependencygraph.cgi?id=32147&showsummary=1 or https://landfill.bugzilla.org/bugzilla-tip/showdependencygraph.cgi?id=28546&showsummary=1, clicking these links will currently display an alert saying "xss". This is because of the bug summary which has been set to the following string: > rectangle (1,2) (3,4) http"><script>alert(/xss/)</script> 1 The issue is the code which read DOT output here: https://github.com/bugzilla/bugzilla/blob/86fb87f/showdependencygraph.cgi#L52. It assumes that each node will produce only one line, this isn't true however if the node label contains line breaks - dot will output the label at the end of the line and leave line breaks as they are. This is an issue when bug summary is being displayed because the label is something like "12345\nsummary" then - and "summary" becomes a line of its own in the resulting map file. Now all one has to do is making sure that the summary is accepted by the regexp on line 58 and add some malicious payload to the URL part (which will be inserted into HTML code without escaping). In fact, adding malicious code to the coordinates would have been possible as well. Seeing bug 1221518, this code seems generally fragile - no escaping being performed for the values inserted into HTML output. Not sure why the the patch for bug 1221518 decided to escape a single field in an entirely different place, it would have been more logical to escape everything that's being inserted there (ideally by using proper templates). Of course, the other part of the issue is Bugzilla using a deprecated map output format which cannot be parsed reliably.
Assignee | ||
Comment 1•8 years ago
|
||
The problem is not HTML-escaping. The escaping is fine. The problem is that the regexp used to parse each line is way to permissive. (.*) accepts everything. It must of course use (\d+) to get coordinates.
Assignee: dependency.views → LpSolit
Status: NEW → ASSIGNED
Attachment #8726304 -
Flags: review?(dkl)
Assignee | ||
Comment 2•8 years ago
|
||
Just in case someone manages to inject code in the future, I also escape the URL, despite it should be safe (because it's built manually).
Attachment #8726304 -
Attachment is obsolete: true
Attachment #8726304 -
Flags: review?(dkl)
Attachment #8726313 -
Flags: review?(dkl)
Updated•8 years ago
|
Flags: sec-bounty?
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Frédéric Buclin from comment #2) > Just in case someone manages to inject code in the future, I also escape the > URL, despite it should be safe (because it's built manually). Except that my proof-of-concept here manipulated the URL field...
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Wladimir Palant from comment #3) > Except that my proof-of-concept here manipulated the URL field... You abused the regular expression, which was too permissive. The URL built in the .map file is fine.
Reporter | ||
Comment 5•8 years ago
|
||
(In reply to Frédéric Buclin from comment #4) > You abused the regular expression, which was too permissive. The URL built > in the .map file is fine. You didn't change that part of the regular expression. The URL built in the .map file didn't matter because I managed to inject my own. I have a strong suspicion that you don't really understand the issue here...
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Wladimir Palant from comment #5) > You didn't change that part of the regular expression. The URL built in the > .map file didn't matter because I managed to inject my own. I have a strong > suspicion that you don't really understand the issue here... I perfectly understand the issue. Thank you!
Assignee | ||
Comment 7•8 years ago
|
||
Affects Bugzilla 2.16rc1 and newer, see bug 134571.
Severity: normal → major
Depends on: 134571
Flags: blocking5.0.3+
Flags: blocking4.4.12+
Summary: Cross-site scripting vulnerability in dependency graphs via bug summary → [SECURITY] XSS vulnerability in dependency graphs via bug summary
Target Milestone: --- → Bugzilla 4.4
Version: 5.1 → 2.16
Comment 8•8 years ago
|
||
The reason the regexp change fixes this is because in the original code, the summary matches the general pattern we are looking for, and matching is greedy, so the value of $leftx (.*) swallows the entire real line, leaving the bogus summary part to match the other pieces. So you can manipulate $url. By fixing the regexp to only match digits, this doesn't happen, and the real stuff matches the other bits properly. So the patch works, but I agree, this is not the way this should be done today. Better, prettier and safer results would be got by using some sort of JS charting library. Gerv
Reporter | ||
Comment 9•8 years ago
|
||
Yes, my issue analysis isn't correct - I was testing this together with bug 1253267 originally and there were indeed two lines in the output. I then realized that a shorter description will do as well but didn't test this thoroughly any more. The issue in this particular case is indeed a greedy regexp whereas in combination with bug 1253267 it is trusting webdot output in general - the output would be accepted even by the modified regexp then, the line would look perfectly correct. And while this patch escapes the URL on one occasion, the default URL is left unescaped (not even HTTP/HTTPS enforcement there, javascript: is fine) - without the change in bug 1253267 this would still be vulnerable.
Comment 10•8 years ago
|
||
Comment on attachment 8726313 [details] [diff] [review] patch, v1.1 Review of attachment 8726313 [details] [diff] [review]: ----------------------------------------------------------------- r=dkl
Attachment #8726313 -
Flags: review?(dkl) → review+
Assignee | ||
Updated•8 years ago
|
Flags: approval?
Flags: approval5.0?
Flags: approval4.4?
Updated•8 years ago
|
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
Assignee | ||
Comment 11•8 years ago
|
||
dveditz: could we have a CVE number for this one?
Flags: needinfo?(dveditz)
Comment 12•8 years ago
|
||
assigned CVE-2016-2803 to this bug.
Alias: CVE-2016-2803
Flags: needinfo?(dveditz)
Updated•8 years ago
|
Updated•8 years ago
|
Flags: sec-bounty? → sec-bounty+
Assignee | ||
Comment 13•8 years ago
|
||
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git abfe21f..dd61903 master -> master To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 16dd96b..0cc1797 5.0 -> 5.0 To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 3cbbb6a..5d47f29 4.4 -> 4.4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Group: bugzilla-security
Updated•8 years ago
|
Group: bugzilla-security
You need to log in
before you can comment on or make changes to this bug.
Description
•