1251647 - XSS vulnerability in the remo-form-payment page

Status: RESOLVED FIXED

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

Description

[Wladimir Palant]

Steps to reproduce:

1. Look through https://bugzilla-dev.allizom.org/buglist.cgi?product=Mozilla Reps&component=Budget Requests and select an old bug that nobody will care about (I decided to use https://bugzilla-dev.allizom.org/show_bug.cgi?id=885675).
2. Edit the title of the bug and add <img src=d onerror=alert("xss")> to it (bonus points for adding some text to mask this manipulation).
3. Send an email with a fake sender to a MoCo employee and trick them into entering 885675 under "Budget request bug" on https://bugzilla-dev.allizom.org/page.cgi?id=remo-form-payment.html.

Result:

As soon as the focus moves away from that field the page will load the bug info and - BOOM, alert triggered by the code in bug title. Real malicious code would take over that Bugzilla account.

The problematic code can be found under https://github.com/mozilla/webtools-bmo-bugzilla/blob/7c39ca2/extensions/REMO/template/en/default/pages/remo-form-payment.html.tmpl#L112:

> div.innerHTML = bug_message;

Variable bug_message is being set a few lines above:

> bug_message = "[% terms.Bug %] " + bug_id + " - " + data.result.bugs[0].status +
>                " - " + data.result.bugs[0].summary;

The bug summary is being taken over for the message, no HTML escaping.

Comment 1

[Wladimir Palant]

Note that entering "<img src=d onerror=alert("xss")>" into that field will produce the same result (no HTML escaping in the error message either) but tricking people to enter that will be significantly harder :)

Comment 2

[Dylan Hardison [:dylan] (he/him)]

There seems to be no reason to be using innerHTML here -- no formatting should be in those messages...
Also some jquery would be be nice.

$(div).text(bug_message) should do the trick.

Looking over similar code, (basically other innerHTML uses in forms and other extensions) I don't see any other cases where we're not escaping.

Comment 3

[Daniel Veditz [:dveditz]]

innerHTML is Satan's candy. Please remove it where you can because it's a giant footgun. Yes, it can be used safely in some cases, but it's also possible for later "improvements" to accidentally turn those safe cases into unsafe ones. Better to build on better bones when you can.

Comment 4

[Daniel Veditz [:dveditz]]

Calling this sec-high for now because XSS on BMO, but because of the social engineering component required for an actual attack sec-moderate might be more appropriate.
 * small number of potential victims (who use that form)
 * attacker would have to appear to be someone we wanted to pay
   (if it were really the person they appeared to be then we probably have too much
   information about them--e.g. bank account numbers--to make this a safe attack.
   too many ways to be detected after the fact)
 * victim would have to not notice the attack in the bug title when they were reviewing the bug
   before going to the payment form
   - or else the attacker would have to be changing the summary at just the right time, between
     review and going to the form. And then removing it after a successful attack although
     that wouldn't hide it from the bug history

Comment 5

[Wladimir Palant]

Social engineering (particularly targeted one which is what we are talking about here) doesn't have to be straightforward. In this particular case a more promising approach would be: "Hey, the payment form won't accept bug 885675 for me, do you see that as well? Please tell me, I'll contact the Bugzilla guys then."

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1251647_1.patch

One step closer to CSP and no YUI.

Comment 7

[Wladimir Palant]

Comment on attachment 8724166 [details] [diff] [review]
1251647_1.patch

jQuery makes me sad but this clearly looks better than the original. Good luck with your CSP chase, you'll need it :)

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8724166 [details] [diff] [review]
1251647_1.patch

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

::: extensions/REMO/web/js/payment.js
@@ +26,5 @@
> +}
> +
> +function togglePaymentInfo (e) {
> +    var div = document.getElementById('paymentinfo');
> +    if (e.checked == false) {

if (!$('#receivedpayment').is(':checked'))

@@ +27,5 @@
> +
> +function togglePaymentInfo (e) {
> +    var div = document.getElementById('paymentinfo');
> +    if (e.checked == false) {
> +        div.style.display = 'block';

use $('#paymentinfo').hide() and $('#paymentinfo').show() if possible.

@@ +46,5 @@
> +    div.show();
> +
> +    if (bug_cache[bug_id]) {
> +        div.text(bug_cache[bug_id]);
> +        return true;    

Remove all trailing whitespace.

@@ +53,5 @@
> +    div.text('Getting bug info...');
> +
> +    var url = ("rest/bug/" + bug_id +
> +               "?include_fields=product,component,status,summary&Bugzilla_api_token=" + BUGZILLA.api_token);
> +    $.get(url).done(function(data) {

better to use $.getJSON as it interprets the inbound response, converts it into a JavaScript Object, and passes it into the callback (so you don't have to mess with eval or other conversion mechanism).

@@ +56,5 @@
> +               "?include_fields=product,component,status,summary&Bugzilla_api_token=" + BUGZILLA.api_token);
> +    $.get(url).done(function(data) {
> +        var bug_message = "";
> +        if (data.error) {
> +            bug_message = "Get bug failed: " + data.error.message;

data.message

@@ +58,5 @@
> +        var bug_message = "";
> +        if (data.error) {
> +            bug_message = "Get bug failed: " + data.error.message;
> +        }
> +        else if (data.result) {

else if (data.bugs && data.bugs[0]) {

@@ +68,5 @@
> +                    "'Mozilla Reps' and component 'Budget Requests'.";
> +            }
> +            else {
> +                bug_message = "Bug " + bug_id + " - " + data.result.bugs[0].status +
> +                    " - " + data.result.bugs[0].summary;

replace data.result with just data in all places.

@@ +72,5 @@
> +                    " - " + data.result.bugs[0].summary;
> +            }
> +        }
> +        else {
> +            bug_message = "Get bug failed: " + res.responseText; 

bug_message = "Bug " + bug_id + " was not found or you do not have access to it.";

@@ +78,5 @@
> +        div.text(bug_message);
> +        bug_cache[bug_id] = bug_message;
> +    }).fail(function(res) {
> +        if (res.responseText) {
> +            div.text("Get bug failed: " + res.responseText);

This will most likely be a JSON data struct if return status != 200 so we should check if JSON and print data.message if possible instead of dumping the JSON to the user.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1251647_2.patch

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8724260 [details] [diff] [review]
1251647_2.patch

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

r=dkl

Comment 11

[David Lawrence [:dkl]]

glob can check this in right before pushing (or me if we do it monday morning) along with bug 1251731 at the same time.

dkl

Comment 12

[Dylan Hardison [:dylan] (he/him)]

NI'ing glob just in case. It would also be no problem for me to do the commits and the push at ~7:00am US/Eastern.

Comment 13

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #12)
> It would also be no problem for me to do the commits and the push at ~7:00am US/Eastern.

thanks - it's probably best for you to drive the push.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   be2d5f9..e9b54b1  master -> master