BMO: Persistent XSS via Git commit messages in comments

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: BMO
RESOLVED FIXED
8 months ago
2 months ago

People

(Reporter: Wladimir Palant, Assigned: dkl)

Tracking

({sec-critical, wsec-xss})

Production
sec-critical, wsec-xss
Bug Flags:
sec-bounty +

Details

(Whiteboard: [reporter-external] [web-bounty-form] [verif?], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 months ago
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?

Comment 1

8 months ago
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.

Comment 2

8 months ago
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.

Comment 3

8 months ago
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.

Updated

8 months ago
Duplicate of this bug: 1305717

Updated

8 months ago
Group: websites-security → bugzilla-security
Component: Other → General
Keywords: sec-critical, wsec-xss
Product: Websites → bugzilla.mozilla.org
Version: unspecified → Production
Component: General → Extensions: Other
(Assignee)

Updated

8 months ago
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
(Assignee)

Comment 7

8 months ago
Created attachment 8795356 [details] [diff] [review]
1305713_1.patch

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-
(Assignee)

Comment 10

8 months ago
Created attachment 8795378 [details] [diff] [review]
1305713_2.patch
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+
(Reporter)

Comment 12

8 months ago
(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.
(Assignee)

Comment 13

8 months ago
To https://github.com/mozilla-bteam/bmo.git
   8affa3e..4ac7c1e  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
(Assignee)

Comment 14

8 months ago
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+
You need to log in before you can comment on or make changes to this bug.