Closed Bug 1305713 Opened 8 years ago Closed 8 years ago

BMO: Persistent XSS via Git commit messages in comments

Categories

(bugzilla.mozilla.org :: Extensions, defect)

Production
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jwkbugzilla, Assigned: dkl)

References

()

Details

(Keywords: sec-critical, wsec-xss, Whiteboard: [reporter-external] [web-bounty-form] [verif?])

Attachments

(1 file, 1 obsolete file)

BMO comments linkify Git commit messages like the following:

To https://github.com/foobar.git
   1234..4321  master -> master

This is implemented as a format hook in extensions/BMO/Extension.pm. Format hooks run before HTML entities are escaped in the text and are responsible for escaping their results themselves - this particular hook fails to do it. So the following text in a comment will run JavaScript code:

> To https://github.com/foobar.git
>    1234..4321  master<iframe/onload=alert(document.domain)> -> master

Note that I quoted the text here to make sure the hook isn't triggered. Without quoting it would display an alert saying "bugzilla.mozilla.org". You don't need to create a new bug in order to test this, entering the text into a comment and switching to the preview tab will already display the alert.

Note that there are two branches in the code processing Git commit messages (github.com and git.mozilla.org). Both of them are vulnerable and the following text will trigger the bug in the other branch:

> To git@git.mozilla.org/foobar.git
>    1234..4321  master<iframe/onload=alert(document.domain)> -> master
Flags: sec-bounty?
Wladimir -- I'm going to post the text of bug 1305717 into here.  Although they are distinct bugs, they are close enough that they can probably be considered all part of the same problem.
I'm also going to use bug 1305717 as the PoC bug, since I can't seem to get into our test site at the moment.
From bug 1305717:

--

BMO comments rewrite links to BMO repositories on git.mozilla.org to point to GitHub instead:

  https://git.mozilla.org/?p=webtools/bmo/bugzilla.git;a=tree [github] [github]

While the text stays unchanged here, target of the link has been modified. This is implemented as a format hook in extensions/BMO/Extension.pm. Format hooks run before HTML entities are escaped in the text and are responsible for escaping their results themselves - this particular hook fails to do it. So the following link will run JavaScript code if you remove the space from it:

  https://git.mozilla.org/?p=webtools/bmo/bugzilla "><iframe/onload=alert(document.domain)>.git;a=tree

Without the space this link would display an alert saying "bugzilla.mozilla.org". You don't need to create a new bug in order to test this, entering the text into a comment and switching to the preview tab will already display the alert.
Group: websites-security → bugzilla-security
Component: Other → General
Product: Websites → bugzilla.mozilla.org
Version: unspecified → Production
Component: General → Extensions: Other
Assignee: nobody → dkl
Status: NEW → ASSIGNED
Component: Extensions: Other → General
Component: General → Extensions: BMO
I note this is not exploitable on bugzilla-dev under the modal UI. If only I had gotten that the CSP changes out a little faster.
I believe this bug has existed since bug 982788
Attached patch 1305713_1.patch (obsolete) — Splinter Review
Found some other places that need to be escaped at well :(

dkl
Attachment #8795356 - Flags: review?(dylan)
I can confirm this was possible as of bug 982788 which was committed without review.

It should have been caught in by me in either the review for bug 1281479 or bug 1286960.
Comment on attachment 8795356 [details] [diff] [review]
1305713_1.patch

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

still vulnerable to "https://git.mozilla.org/?p=webtools/bmo/bugzilla" ><iframe/onload=alert(document.domain)>.git;a=tree
Attachment #8795356 - Flags: review?(dylan) → review-
Attached patch 1305713_2.patchSplinter Review
Attachment #8795356 - Attachment is obsolete: true
Attachment #8795378 - Flags: review?(dylan)
Comment on attachment 8795378 [details] [diff] [review]
1305713_2.patch

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

r=dylan
Attachment #8795378 - Flags: review?(dylan) → review+
(In reply to Dylan Hardison [:dylan] from comment #8)
> I can confirm this was possible as of bug 982788 which was committed without
> review.

The other issue which was marked as a duplicate landed in bug 1282931 - proper review and everything.

(In reply to David Lawrence [:dkl] from comment #7)
> Found some other places that need to be escaped at well :(

From what I can tell these aren't exploitable. Not that I'm against proper escaping everywhere.
To https://github.com/mozilla-bteam/bmo.git
   8affa3e..4ac7c1e  master -> master
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Pushed live.
Group: bugzilla-security
(In reply to Wladimir Palant from comment #12)
> (In reply to Dylan Hardison [:dylan] from comment #8)
> > I can confirm this was possible as of bug 982788 which was committed without
> > review.
> 
> The other issue which was marked as a duplicate landed in bug 1282931 -
> proper review and everything.
> 
Both bug 1281479 or bug 1286960 were proper reviews, but I was blind to the lack of html_quote().
I think dkl's patch on this of adding that everywhere is good defensive programming. 

An even better defense is https://bugzilla.mozilla.org/show_bug.cgi?id=1286290,
which I will be expanding to cover both versions of show_bug.cgi.
Flags: sec-bounty? → sec-bounty+
Component: Extensions: BMO → Extensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: