1332829 - XSS (Reflected) in aus5.mozilla.org

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (frontend), defect, P1)

Description

[Hassham]

Attachment: Reflected cross-site scripting vulnerabilities arise when data is copied from a request and echoed into the application's immediate response in an unsafe way. An attacker can use the vulnerability to construct a request that, if issued by another applicat

While going thorught the application this url was discovered, further it was tested with Burp Suite and it was discovered that it is possible to set the value of the URL path folder 4 as it  is copied into the HTML document as plain text between tags. The payload zibj5<img src=a onerror=alert(doucment.domain)>q6lli was submitted in the URL path folder 4. 

This input was echoed unmodified in the application's response.  This proof-of-concept attack demonstrates that it is possible to inject arbitrary JavaScript into the application's response. The proof-of-concept attack demonstrated uses an event handler to introduce arbitrary JavaScript into the document.

POC link: https://aus5.mozilla.org/update/3/GMP/50.1.0zibj5%3Cimg%20src%3da%20onerror%3dalert(document.domain)%3Eq6lli/20161208153507/WINNT_x86-msvc-x64/en-US/release/Windows_NT%206.3.0.0%20(x64)/default/default/update.xml

Comment 1

[Julien Vehent]

Thanks for the report.
Ben: could you take a look at make sure Balrog escapes query parameters before reflecting them?
I think the application should also return a very basic CSP:
> Content-Security-Policy: default-src https:

Comment 2

[Benson Wong [:mostlygeek]]

Also setting the content-type header to 'text/plain' would prevent the browser from parsing it as HTML.

Comment 3

[[e-mail] (:bhearsum)]

OK, so there's three things here:
1) Properly escape anything going into responses, to make sure arbitrary Javascript cannot be run. This was made possible by bug 1281459, where we started including the %VERSION% portion of the URL in certain error responses. I'm not sure if this enables any real attack - the client side updater in Firefox parses all responses from Balrog as text/xml.
2) Set a CSP.
3) Make sure we set error responses' Content-Type to text/plain.

I'll have a look this week.

Comment 4

[April King [:April]]

Why this:

> Content-Security-Policy: default-src https:

And not this:

> Content-Security-Policy: default-src 'none'; frame-ancestors 'none'

Are we ever viewing content directly on aus5?  This is what our websec guidelines recommend for "API" style websites where you're not viewing things directly.

Comment 5

[April King [:April]]

> Also setting the content-type header to 'text/plain' would prevent the browser from parsing it as HTML.

This is only *sometimes* the case; many browsers can and will attempt to guess the MIME type. Not sure if that will happen in this case, as its MIME type sniffing behavior seems, at times, to be somewhat non-deterministic. To be safe, you can add this header:

X-Content-Type-Options: nosniff

Which will disable that behavior.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: escaping, CSP, and mimetype

April, do you mind having a look at this to make sure I've done the right things with the sec issues. Specifically, I'm:
- Escaping the entire response body for error responses. Based on my own testing, cgi.escape seems like the right thing to use - it escapes &, <, and > per https://docs.python.org/2/library/cgi.html#cgi.escape.
- Always setting the Content-Security-Policy header
- Setting mimetype to text/plain for error responses. (Normal responses are text/xml, and never contain any part of the request URI.)

Johan, flagging you to make sure I'm not implemented any of this in a silly way.

Since this is a security bug, I'll attach the patch here. The next production push isn't planned until next week, so I'll hold off pushing this to https://github.com/mozilla/balrog until we're ready to stage that. If it's important to get this fixed in production sooner, we can probably do one later this week.

Comment 7

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8829993 [details] [diff] [review]
escaping, CSP, and mimetype

This patch looks good to me. For the record, I agree with the implementation details, which are:
* Using cgi to escape HTML characters in python 2 code is the way to go[1].
* @app.after_request appends the CSP header on every response we send (error or regular responses) which is great.
* Only error responses get their content-type to "text/plain".

In regard to comment 5, I'd like to call out: Firefox might sniff the content type even with `X-Content-Type-Options: nosniff` set. This feature is present in Firefox only since 50 (see bug 471020). I read some of the comments there: bug 471020 comment 78 highlight these specs[2] which read:
> The following MIME types (with or without parameters) must not be interpreted as scripting languages:
>    "text/plain"
>    [...]

Hence, even though this feature is not present prior Firefox 50, it seems we won't execute JS on the client side anymore, thanks to `text/plain`. That said, I'm not an expert, I just crawled some docs to get more context :)

[1] https://wiki.python.org/moin/EscapingHtml 
[2] https://www.w3.org/TR/html5/scripting-1.html#scriptingLanguages

Comment 8

[April King [:April]]

I generally prefer to be overly safe, but it appears you are right about text/plain -- learn something new every day!  If we want to skip it to safe a few bytes, I can totally understand.  That said, if we are planning on moving to HTTP/2 soon, it might not hurt to implement as header compression should ensure that overhead remains the same regardless of the header's presence, after the initial transmission.

Comment 9

[Daniel Veditz [:dveditz]]

Firefox does not follow the spec and block text/plain, unless the X-Content-Type-Options: nosniff header is sent. Release users see about 1% of scripts as text/plain and 2% of scripts as text/html. Looks like 0% is xml but we haven't blocked it yet -- the telemetry to see what we could safely block is relatively new.

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-01-19&keys=__none__!__none__!__none__&max_channel_version=release%252F51&measure=SCRIPT_BLOCK_INCORRECT_MIME&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-01-16&table=0&trim=1&use_submission_date=0

See bug 1288361 and bug 1299267. Needs a follow-up now that we have data: safe to block the two XML types, text/csv, and maybe application/octet-stream (who are those 0.14% and how loud will they scream?). We can try blocking text/plain on nightly as an experiment and see what happens, but 1% usage is usually too much to successfully kill something.

Comment 10

[Daniel Veditz [:dveditz]]

Moral: when it comes to browsers don't believe the spec. Sometimes specs reflect what "most" browsers do (but you're using one that's not one of the "most"), and other times specs say things that the editors desperately wish were true but no browser actually does (yet?). This is especially true for a large spec trying to describe historical accretion like HTML.

Comment 11

[April King [:April]]

Thank you for your input, Dan.  Always appreciate your insight into these areas.  :)

Overall then, I think the plan is to update AUS to have:

> Content-Security-Policy: default-src 'none'; frame-ancestors 'none'
> X-Content-Type-Options: nosniff

Unless anybody has any strong objections?

Comment 12

[April King [:April]]

To add to Dan's comments, often times the spec describes something such as "extensions and bookmarklets should be exempt from CSP" which sound super easy but are actually almost entirely infeasible to implement technically given the design of browser and javascript engines.

Comment 13

[April King [:April]]

Comment on attachment 8829993 [details] [diff] [review]
escaping, CSP, and mimetype

r+, pending the addition of XCTO  :)

Comment 14

[[e-mail] (:bhearsum)]

Attachment: updated patch that also sets X-Content-Type-Options

Comment 15

[Daniel Veditz [:dveditz]]

I kind of wonder why we're echoing anything back at all for this error. Other errors are either ignored (e.g. doesn't seem to care what date you give) or result in an empty <updates></updates>. But given that we do, the proposed CSP looks good.

Comment 16

[Daniel Veditz [:dveditz]]

Filed bug 1333995 about blocking XML types at least, possibly more.

Comment 17

[[e-mail] (:bhearsum)]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> I kind of wonder why we're echoing anything back at all for this error.
> Other errors are either ignored (e.g. doesn't seem to care what date you
> give) or result in an empty <updates></updates>. But given that we do, the
> proposed CSP looks good.

Eating errors and returing <updates></updates> has made debugging errors a lot more difficult in the past (https://bugzilla.mozilla.org/show_bug.cgi?id=1281459 has background). You'll still get <updates></updates> when you make a valid request that doesn't map to an update, but invalid input (eg: failing to a parse a version) will result in a more useful error message.

Comment 18

[[e-mail] (:bhearsum)]

This is in production now. The new headers look correct to me - can someone else verify as well?

Comment 19

[April King [:April]]

Looks great to me!  https://observatory.mozilla.org/analyze.html?host=aus5.mozilla.org

Comment 20

[[e-mail] (:bhearsum)]

(In reply to April King [:April] from comment #19)
> Looks great to me! 
> https://observatory.mozilla.org/analyze.html?host=aus5.mozilla.org

Thanks! This fixes bug 1302164 too, in that case.