Closed Bug 1273245 Opened 3 years ago Closed 3 years ago

[SECURITY] Backport upstream bug 1253263 to bmo/4.2 to fix XSS vulnerability in dependency graphs via bug summary

Categories

(bugzilla.mozilla.org :: General, defect, major)

Production
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: dkl, Assigned: dkl)

References

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1253263 +++

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 1273245_1.patchSplinter Review
Attachment #8753035 - Flags: review?(dylan)
Comment on attachment 8753035 [details] [diff] [review]
1273245_1.patch

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

r=dylan
Attachment #8753035 - Flags: review?(dylan) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   d63cbb5..3808736  master -> master
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
pushed live
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.