1243178 - (CVE-2016-1954) CSP's report-uri (over-)writes files

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Nicolas Golubovic]

Firefox can be forced to (over-)write files on the user's host when prompted to report policy violations to a file:///, resource:// or chrome:// URL. While the last two are tricky to exploit since they most likely require vulnerable add-ons, the first is dead simple. The attacker does not have full control of the report, but can control multiple values inside of the report JSON structure.

For a demonstration of the issue you may visit the following link with a UNIX host and check your /tmp directory afterwards. It should contain a file called attack_works.html.

https://c.iceqll.eu/overwrite_resource.php

The attack sends the following CSP header and then violates it:
Content-Security-Policy: script-src 'none'; report-uri file:///tmp/attack_works.html

Works in:
- FF 42, 43, 44

The first tested version where it _does not work anymore_ is the Nightly Build (47.xx).

Why do I report the issue anyway? Two reasons:
- Maybe it was fixed by accident and the severity went unnoticed
- I am in the course of writing a thesis, so I'd love to have something to reference - and if this is a duplicate, I'd like to reference that. Sorry. I'm being very selfish :-/

Comment 1

[Daniel Veditz [:dveditz]]

I can't think of anything we changed recently that would "fix" this in Nightly, though we should limit report-uri to http(s) urls. Or at the very very least we need to run CheckMayLoad() on it so we're not loading URI_IS_DANGEROUS_TO_LOAD schemes!

Comment 2

[Daniel Veditz [:dveditz]]

Christoph says this is likely fixed by bug 1188028, which would correspond to the beta channel (though I don't think we've released a 45 beta yet... end of the week). Would also be fixed on Dev Edition (Firefox 46).

Still, even though the AsyncOpen2() changes do the basic may-load checks, I'd still be happy to stomp only any report-uri that tries to set a non-http(s) URL.

Comment 3

[Frederik Braun [:freddy]]

If this bug indeed allows overwriting files within extensions, this would allow code injection with high(er) privileges, right?
I'd rather err on the side of caution and call this higher than sec-moderate. Thoughts?

Comment 4

[Nicolas Golubovic]

(In reply to Frederik Braun [:freddyb] from comment #3)
> If this bug indeed allows overwriting files within extensions, this would
> allow code injection with high(er) privileges, right?

Right. As shown in the PoC above, it is possible to smuggle HTML (and JS) into the report. When visited using a privileged scheme like chrome://xxx/content/ this will have full code execution on the host system. I have not tested with built-in chrome and resource URIs, but one possible scenario is the following:


- Add-on with em:unpack=true (this may not be required in all cases) registers content package with contentaccessible=yes
- Attacker knows that the add-on is activated on the victim's browser due to fingerprinting (trivial when contentaccessible=yes is given)
- Attacker overwrites an existing add-on HTML or XHTML file with the report-uri technique (e.g. chrome://addon-name/content/file.xhtml)
- On the next load of the overwritten file, the attacker's payload executes
=> JavaScript code execution in a privileged chrome context


However, I think a signature check which you will enforce in future versions might also mitigate this threat?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Nicolas G from comment #4)
> - On the next load of the overwritten file, the attacker's payload executes
> => JavaScript code execution in a privileged chrome context

But the attacker can't control what will be written in the file. It will always be the CSP-report, no?

Comment 6

[Nicolas Golubovic]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> But the attacker can't control what will be written in the file. It will
> always be the CSP-report, no?

That's true. However, take a close look at the written report (also from the above PoC):

{"csp-report":{"blocked-uri":"self","document-uri":"https://c.iceqll.eu/overwrite_resource.php","line-number":3,"original-policy":"script-src 'none'; report-uri file:///tmp/attack_works.html","referrer":"","script-sample":"<svg onload=alert(1)>","source-file":"https://c.iceqll.eu/overwrite_resource.php","violated-directive":"script-src 'none'"}}%      qll@corellia /tmp % 

A HTML parser will skip all the JSON gibberish it does not understand and will find the first < right there at the <svg onload=alert(1)> part. It will just take that out of the context and render it, thus executing the JavaScript payload.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

I agree, this is a nifty idea. Exactly that's why we are introducing 'Security be Default' Network checks at the moment, by calling ascyncOpen2() on all channels. For CSP reports we are using asyncOpen2() since FF45 [1] which doesn't give you local file access.

The other thing that makes this kind of attack a little more difficult is, that the attack code has to fit within 40 chars [2] because we truncate the script sample. Not impossible to do, but definitely harder.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/acc983ca0dec#l5.285
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#418

Comment 8

[Daniel Veditz [:dveditz]]

Raising severity rating. It's a low-ish sec-high, but web-accessible file destruction (even if writing useful content seems unlikely, though not impossible) is at least as bad as bug 1241100. We need this fixed on ESR 38

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_csp_uri_reports.patch

Given that we need a fix for ESR 38, I think what we actually want is that small fix within the parser so that a report-uri can not point to any other schemes than http:// or https://.

Also, using that approach, we spit out a console warning at CSP parsing, potentially something we want on trunk at some point anyway.

What do you think Dan?

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8712899 [details] [diff] [review]
bug_1195172_csp_uri_reports.patch

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

Would like a new patch along the lines I mention, but if you feel strongly about keeping this an absolute minimum patch I could live with this.

::: dom/security/nsCSPParser.cpp
@@ +932,2 @@
>        const char16_t* params[] = { mCurToken.get() };
>        logWarningErrorToConsole(nsIScriptError::warningFlag, "couldNotParseReportURI",

this is OK as a quick fix. Might be good enough for ESR38 but will lead to a confusing error message in this case (could not parse a valid url?). It wouldn't be too much work to make this
 
  if (failed)
    current "couldNotParseReportURI code
  else if (!isHttp)
    same but use reportURInotHttpsOrHttp2

we already have that string and use it to log a warning in SendReports
https://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#875

That SendReports warning-only check would then be redundant of course, but for the sake of a smaller ESR change we can leave it.

::: dom/security/test/TestCSPParser.cpp
@@ +504,5 @@
>      { "connect-src http://www.example.com/foo%zz;",
>        "connect-src 'none'" },
>      { "script-src https://foo.com/%$",
>        "script-src 'none'" },
> +    { "report-uri file:///tmp/attack_works.html", "report-uri 'none'"},

The test kind of gives the game away, don't think we should check this in with the patch. Although the patch itself is pretty obvious. This will have to be a late-cycle check-in (in which case the test is OK). 

More interested in adding the test to trunk though, so it'll be tested for the future. The current trunk code doesn't have this security bug, but it would not pass this test since the AsyncOpen2() check that prevents the bug happens at SendReports time.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_csp_reports_skip_non_http.patch

As agreed over IRC, we should keep the change small. Since we are already logging a warning, we can simply add a 'continue' and skip the invalid URI, as we do for other errors that appear at that stage.

[1] Content Security Policy: The report URI (file:///tmp/attack_works.html) should be an HTTP or HTTPS URI.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 8712945 [details] [diff] [review]
bug_1195172_csp_reports_skip_non_http.patch

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

Nice and simple, and a more appropriate message. Thanks
r=dveditz

Comment 13

[Nicolas Golubovic]

Quick heads-up after some tests: You may already know this, but unpack=true is required and contentaccessible=yes is not. HTTPS Everywhere, for example, fits the description and can be disabled on recent Firefox versions by overwriting chrome://https-everywhere/content/about.xul, for example. This will make the signature check fail. Themes and other add-ons not subject to rigorous signature checks can be hijacked, as well as extensions when checks are disabled by a setting.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 8712945 [details] [diff] [review]
bug_1195172_csp_reports_skip_non_http.patch

sec-approval+

Don't need this on aurora/beta, but we do want it on ESR-38

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Christoph, could you please nominate a patch for uplift to esr38? Thanks! I am also wondering whether this fix is already on 45, 46 and 47 because I don't see committs from m-b, m-a, m-c in this bug. Please confirm.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ritu Kothari (:ritu) from comment #15)
> Hi Christoph, could you please nominate a patch for uplift to esr38? Thanks!
> I am also wondering whether this fix is already on 45, 46 and 47 because I
> don't see committs from m-b, m-a, m-c in this bug. Please confirm.

In fact, the proper fix from a Security point of view landed within Bug 1188028 which was for 45. So at the moment only ESR is affected. Dan I chatted in person and since the patch within this bug is real simple we should land this patch on 45,46,47 and ESR. I'll file the uplift request.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8712945 [details] [diff] [review]
bug_1195172_csp_reports_skip_non_http.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
Users of webpages might be exposed to local file overwrite attacks using the report-uri directive, where instead of reporting to a URI JS might be written into a local file.

[Feature/regressing bug #]:
Most likely that was the CSP rewrite (Bug 925004).

[User impact if declined]:
Attack from Comment 0 could be mounted.


[Describe test coverage new/current, TreeHerder]:
Manual verification.

[Risks and why]: 
Low, since it affects only pages that use CSP and use the report-uri directive.

[String/UUID change made/needed]:
no

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8712945 [details] [diff] [review]
bug_1195172_csp_reports_skip_non_http.patch

Sec-high and simple bug fix. Let's uplift to Beta45, Aurora46 and esr38.7

Comment 19

[Carsten Book [:Tomcat]]

wes could you land this ?

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a7c3d89e43b
https://hg.mozilla.org/releases/mozilla-beta/rev/5154bb929236
https://hg.mozilla.org/releases/mozilla-esr38/rev/a5c4c18849b4

Comment 21

[Carsten Book [:Tomcat]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/f72ada9941bb

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f72ada9941bb

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue in Fx44, verified fixed in Fx45.0.2.