1349899 - Clean up invalid mozreview urls on attachment page

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, defect)

Description

[Wladimir Palant]

See https://bugzilla-dev.allizom.org/attachment.cgi?id=8592183&action=edit - an alert message saying XSS will appear. Code has been injected here by adding a text/plain attachment with the following contents:

> https://reviewboard.mozilla.org/r/12345/?"><script>alert("XSS")</script><"

The bug can be seen in https://github.com/mozilla-bteam/bmo/blob/e655f5133b4dd5c5d6f253069767db75c64eb349/extensions/MozReview/template/en/default/hook/attachment/edit-view.html.tmpl#L15 - while the review URL is being filtered in the title attribute, for the href attribute no escaping is being performed. So malicious data will be able to inject arbitrary HTML code here as long as it still matches the regular expression used to validate review URLs.

Comment 1

[Wladimir Palant]

For reference, I consider it a bad pattern throughout the entire Bugzilla and BMO codebase to insert values into HTML/JavaScript code without escaping. The developers seem to go to great lengths in order to determine which values are "safe" to insert without escaping them - at the risk of being wrong. Even if a value is "safe" now, an entirely unrelated change might turn it into something that an attacker can manipulate. People won't always remember updating templates because code locality isn't given in this case. This whole class of issues can be avoided entirely by escaping everything as a rule and only use "FILTER none" if the code would break otherwise (e.g. due to double escaping). The few extra CPU cycles added don't really matter these days. In fact, I have always been recommending template engines that perform the escaping by default and require you to take action in order to *skip* escaping - something that will be flagged in a review and hopefully lead to a thorough security evaluation. Modern template engines also avoid double escaping in most cases automatically by wrapping results returned by subtemplates in a special class that marks them as "already escaped, don't escape again."

Comment 2

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

(In reply to Wladimir Palant from comment #1)
> For reference, I consider it a bad pattern throughout the entire Bugzilla
> and BMO codebase to insert values into HTML/JavaScript code without
> escaping. The developers seem to go to great lengths in order to determine
> which values are "safe" to insert without escaping them - at the risk of
> being wrong. Even if a value is "safe" now, an entirely unrelated change
> might turn it into something that an attacker can manipulate. People won't
> always remember updating templates because code locality isn't given in this
> case. This whole class of issues can be avoided entirely by escaping
> everything as a rule and only use "FILTER none" if the code would break
> otherwise (e.g. due to double escaping). The few extra CPU cycles added
> don't really matter these days. In fact, I have always been recommending
> template engines that perform the escaping by default and require you to
> take action in order to *skip* escaping - something that will be flagged in
> a review and hopefully lead to a thorough security evaluation. Modern
> template engines also avoid double escaping in most cases automatically by
> wrapping results returned by subtemplates in a special class that marks them
> as "already escaped, don't escape again."

Agreed. If we ported the templates to Text::Xslate, this would be the case.
Values have to be marked raw.

For that matter, if performance was an issue we'd use https://metacpan.org/pod/HTML::Escape anyway.

Additionally, this will be the next place where I tune a strict CSP. :-)

Comment 3

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

There is a second minor vulnerability implied in this.

1) create attachment with content javascript:alert("batman")
2) set content type to text/x-review-board-request

This is possible because we don't even check the url with is_safe_url().

my patch will address both issues.

Comment 4

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

Attachment: Testing this

Comment 5

[Wladimir Palant]

(In reply to Dylan Hardison [:dylan] from comment #3)
> There is a second minor vulnerability implied in this.
> 
> 1) create attachment with content javascript:alert("batman")
> 2) set content type to text/x-review-board-request

Does this do anything? text/x-review-board-request is only treated specially if it matches the regexp and this URL won't.

Comment 6

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

Attachment: 1349899_1.patch

Fixes this in a few ways:

1) html escaping (the primary oversight; I think the fact that the same value is used twice on the same line, one place FILTER url, the other not, was how I missed it in review)
2) The regex for mozreview links is now more strict. This would have prevented exploitation too. Github links already are terminated by $, and I clarified with mcote what mozreview links should look like. 
3) Finally, we're going to reject urls that contain non-url-encoded junk at the end. If we add more autodetect urls in the future.

The larger issue of escaping we can address later; I'm very curious about porting our templates to Text::Xslate or maybe using https://metacpan.org/pod/Template::Stash::AutoEscaping.

Comment 7

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

Attachment: 1349899_2.patch

Minor improvement. Replacing the check for \s with a check for all characters that should probably be percent-encoded.

Comment 8

[:glob ✱]

Comment on attachment 8850555 [details] [diff] [review]
1349899_2.patch

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

r=glob

::: extensions/BMO/Extension.pm
@@ +1152,5 @@
>      return unless defined $url;
>      return if length($url) > 256;
>      $url = trim($url);
> +    # ignore urls that contain unescaped characters outside of the range mentioned in RFC 3986 section 2
> +    return if $url =~ m<[^A-Za-z0-9._~:/?#\[\]@!\$&'()*+,;=`.%-]>;

. is in the regex twice.

Comment 9

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

I think this will go out tonight, considering the severity.

Comment 10

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

There is no evidence of this being exploited in the production database, aside from my attachment to this bug.

checked all urls returned via:
SELECT thedata FROM attachments JOIN attach_data ON attach_data.id = attachments.attach_id WHERE mimetype = "text/x-review-board-request"

Comment 12

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

This bug is fixed now.