Closed Bug 1253263 (CVE-2016-2803) Opened 4 years ago Closed 4 years ago
[SECURITY] XSS vulnerability in dependency graphs via bug summary
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.
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)
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).
(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...
(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.
(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...
(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!
Affects Bugzilla 2.16rc1 and newer, see bug 134571.
Severity: normal → major
Depends on: 134571
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
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
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+
dveditz: could we have a CVE number for this one?
assigned CVE-2016-2803 to this bug.
To ssh://email@example.com/bugzilla/bugzilla.git abfe21f..dd61903 master -> master To ssh://firstname.lastname@example.org/bugzilla/bugzilla.git 16dd96b..0cc1797 5.0 -> 5.0 To ssh://email@example.com/bugzilla/bugzilla.git 3cbbb6a..5d47f29 4.4 -> 4.4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
sec adv sent
You need to log in before you can comment on or make changes to this bug.