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)

2.16
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: jwkbugzilla, Assigned: LpSolit)

References

Details

(Keywords: sec-high, wsec-xss)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch, v1 (obsolete) — Splinter Review
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)
Attached patch patch, v1.1Splinter Review
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)
Flags: sec-bounty?
(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
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
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
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 on attachment 8726313 [details] [diff] [review]
patch, v1.1

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

r=dkl
Attachment #8726313 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval5.0?
Flags: approval4.4?
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
dveditz: could we have a CVE number for this one?
Flags: needinfo?(dveditz)
assigned CVE-2016-2803 to this bug.
Alias: CVE-2016-2803
Flags: needinfo?(dveditz)
Blocks: 1258124
Flags: sec-bounty? → sec-bounty+
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
Blocks: 1273245
sec adv sent
Group: bugzilla-security
Group: bugzilla-security
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: