Closed Bug 911547 (CVE-2014-1504) Opened 11 years ago Closed 10 years ago

data-URI + Firefox restart = CSP bypass

Categories

(Core :: Security, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox27 --- wontfix
firefox28 + verified
firefox29 + verified
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: nicolas.golubovic+bugzilla, Assigned: geekboy)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-other, sec-low, Whiteboard: [adv-main28+])

Attachments

(4 files, 6 obsolete files)

If an attacker injects a data-URI into a web application, the Content Security Policy of the linking site gets inherited. Example:

<a href="data:text/html,<script>alert(document.cookie)</script>">Click me</a>

This attack would be mitigated with a reasonable CSP (or default-src 'none'), but the Browser will still visit the data-URI. If an attacker now manages to kill the Browser (due to bugs/exploits triggered by another tab or Addon installation or ...) Firefox will try to restore the browsing session when it is restarted. Now the data-URI will be opened in the context of the site protected with CSP but without enforcing a CSP.


Steps to reproduce:

1. XSS a site with CSP enabled:
http://poc.iceqll.eu/cspdata/?xss=<a+href="data:text/html,<script>alert(document.cookie)</script>">Click+me</a>

2. Click the data-URI (or lure your victim into doing so).

3. Kill the Browser (after waiting a second):
killall -9 firefox-bin

4. Start the Browser again. Oops. CSP bypassed?



Not sure if this is really that much security critical because there are huge obstacles an attacker has to overcome (particularly killing the Browser).

Tested on Nightly build 26.0a1 (2013-08-31).
If I understand the bug right, it's not just limited to the data: URI, but actually any loaded-from-cache page.  Does the same problem exist with inline script?   For example, if session restore re-opens a page that had CSP-blocked inline script, will it continue to block it?
Flags: needinfo?(mozilla.org)
I've tested the issue on my POC-page [1] and it does not work when restoring the session. I'm not sure why but FF does not take the page from the cache and sends a new request (maybe I've set the wrong headers?). 

Tested with:
http://poc.iceqll.eu/cspdata/?xss=%3Cscript%3Ealert%281%29%3C/script%3E
Flags: needinfo?(mozilla.org)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #1)
> If I understand the bug right, it's not just limited to the data: URI, but
> actually any loaded-from-cache page.

We save http headers in the cache, why wouldn't the CSP header be there for a normal http page? For a data uri, yes, I see the problem. The data URI isn't really in the "cache" at all, it's in the session-restore list. When it gets restored the browser doesn't have to look in the cache because the URL itself is all the data it needs.

But wait, the domain of a data: document is context dependent, and if you've crashed the browser I'm not sure we save that context. When you restart the script may run but it's probably in a domainless null principal that can't attack the original site. At least I hope so--obviously need to test to make sure (it's possible session-restore tries to be clever and save the domain context, but I'd hope not).

If the data: uri is restored with a "null principal" then this isn't a CSP bypass at all.

[Just checked, we do store the CSP headers in the cache. You can check individual entries through about:cache?device=disk ]
Damn, we are somehow saving (part of) the context for data: urls in sessionrestore somehow. We either need to stop doing that or find a way to save CSP as part of that context. The former is probably easier.
An attack would probably look like:
 * attacker page (no CSP) opens victim page with injection
   (preferably in a frame, but new tab works too)
 * As data: is blocked by default somehow you have to get the
   user to click on your link. Depending on where the injection
   is this could range from very hard to get noticed, to a page
   that allows inline-style ("safe", right?) so that you can
   float an image link over the entire contents.
 * If the attack is in a tab the rest is guessing, but in a frame
   you can detect navigation, wait a few seconds for session-restore
   to capture the state, and then use any of the known crashing
   bugs in the attacker page to crash the browser.
 * let session restore do the dirty work for you

The CSP spec is unlikely to tackle navigation any time soon so our choices are
 1) save CSP as part of session-restore
 2) tell session-restore to stop saving data: uri domain state

I don't believe this attack can generalize to all sites so I'm provisionally calling it sec-low.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: csec-other, sec-low
Product: Firefox → Core
Session restore serializes the principal.

When mCSP was added to nsPrincipal, the serialization was _not_ changed to include it.  So now serializing and deserializing a principal doesn't produce the same permissions as it used to have, which seems pretty clearly bad to me.
I think we should make the implementation of nsIContentSecurityPolicy serializable and then save it with the session stored by nsPrincipal.cpp.  I've begun an initial whack at doing this, and will post a patch when it is somewhat complete.  I'll probably just serialize the CSP object as a set of policy strings and then create new objects from those strings (formerly headers) when the session is restored.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
While trying to serialize the CSP object, I noticed that the CSP object holds a reference to the requesting principal for sending reports (and asking content policies for permission).  Problem is re-connecting that principal on deserialization since serializing it would have generated a copy.  The 

nsDocument::InitCSP() is the thing that originally hooks this up, but I can't see where to hook into the deserialization/Read() procedure to reconnect it.

I see there are three options:
1. Don't reconnect _requestingPrincipal and risk dropping violation reports for restored pages (that is what will happen).
2. Have nsIContentSecurityPolicy implement nsISerializable, then find where the document is deserialized and call back into nsIContentSecurityPolicy.scanRequestData somewhere in the document.
3. Don't serialize the CSP, but re-compute it by calling nsDocument::InitCSP() again once the document has been revived.

1 is easy.  I can't quite figure out how to finish 2 (not sure what is the right point to re-connect that reference, but serializing everything else inside an nsIContentSecurityPolicy is easy).  3 would perhaps be elegant, but we would lose any runtime changes to the CSP (add-on supplied, etc).

So I really wanna do 2, but can't figure out where I can obtain the channel corresponding to the document that's deserializing this principal.  Any help?  I'll soon post my WIP that I just tossed together and have not yet tested.
Flags: needinfo?
Attached patch wip -- not done (obsolete) — Splinter Review
Flags: needinfo?
Accidentally cleared the needinfo flag.  Anyone have thoughts on how to do #2?  My WIP is a start at this.
Flags: needinfo?
hg blame suggested I ask you, bz.  Any thoughts on how/where to re-attach the channel (nsIContentSecurityPolicy::ScanRequestData()) to the deserialized CSP object?
Flags: needinfo? → needinfo?(bzbarsky)
> Problem is re-connecting that principal on deserialization since serializing it would
> have generated a copy.

Hmm.  Our serialization/deserialization code has some general provisions for reusing existing objects, but that may just be for fastload bits.

In this case the document is not being deserialized at all; just the SHEntry.  In particular, InitCSP is in fact getting called in this case... but for a data: document it does nothing, of course, since there is no CSP to be had there.

At the point when the SHEntry is being deserialized there is no channel, nor any document.  So doing #2 might be hard if it actually needs a channel.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #13)
> In particular, InitCSP is in fact getting called in this case...
> but for a data: document it does nothing, of course, since there is no CSP
> to be had there.

Oh, hm.  Maybe there's a way to attach the CSP to a data: document before InitCSP gets called so InitCSP can find it and pull it out.  Are there any flags or whatever on the data: document that indicate it's being restored?

> At the point when the SHEntry is being deserialized there is no channel, nor
> any document.  So doing #2 might be hard if it actually needs a channel.

Well, what it really needs is a reference back to its owning principal (which ScanRequestData gets from the channel).  I suppose we could implement another function on nsIContentSecurityPolicy that lets us hook up the principal, but that seems a bit forced and after a quick scan of SessionRestore.jsm, I'm not sure it would be very easy.

InitCSP pulls the principal from nsDocument::NodePrincipal(), so if we can somehow pull a string representation of the CSP out of the serialized object and make it available during InitCSP, this becomes pretty easy.  Do you know off the top of your head if there are any easy ways to make a http-header-like thing like CSP tag along for a data: document's path through StartDocumentLoad() -- that we could pull out during InitCSP?
Flags: needinfo?(bzbarsky)
> Are there any flags or whatever on the data: document that indicate it's being restored?

There might be on the docshell...

> so if we can somehow pull a string representation of the CSP out of the serialized
> object and make it available during InitCSP, this becomes pretty easy

Can you not store that in the serialization/deserialization of the principal?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #15)
> > so if we can somehow pull a string representation of the CSP out of the serialized
> > object and make it available during InitCSP, this becomes pretty easy
> 
> Can you not store that in the serialization/deserialization of the principal?

Yeah, I guess we can, but the CSP interface currently does not provide for setting the principal ref directly (requires a channel).  We probably need to change nsIContentSecurityPolicy to have take a reference to the princpal and not a channel, so I'll give that a shot.  I was hoping for a simpler/more elegant solution, but I think this is the right way.

Thanks for the help, bz.
Attached patch tests (obsolete) — Splinter Review
I can't reproduce the reported behavior on nightly.  I can see based on the fact that nsPrincipal doesn't serialize the CSP how this might happen, but I can't actually recreate it.

Attached are some mochitests that attempt to recreate data: URI and inline scripts that CSP should block or allow according to the policy.  The tests pass on nightly without any additional patches.  

Nicolas: have you been able to reproduce this issue recently?  When I run your PoC, I don't see the alert (before or after session restoring).
Attachment #803372 - Flags: review?(ttaubert)
Flags: needinfo?(mozilla.org)
To be clear, I can't reproduce the "alert comes back" behavior you're looking for in comment 2.

The data: URI that's loaded after click does indeed have a suppressed alert before kill and an active alert after restore.  The same behavior should come in session restore of an HTTP page, right?
Attached patch wip (obsolete) — Splinter Review
This should take care of storing CSP with the principal.  The other attachment (tests) don't fail without this patch, so I'm not exactly sure what's going on or how to test for whether this is stored or not when the session-restore revived tab via tests in attachment 803372 [details] [diff] [review] seems to keep its CSP without this patch.
Attachment #800878 - Attachment is obsolete: true
Comment on attachment 803389 [details] [diff] [review]
wip

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

::: browser/components/sessionstore/test/Makefile.in
@@ +139,5 @@
>  	$(filter disabled-for-intermittent-failures--bug-766044, browser_459906_sample.html) \
>  	$(filter disabled-for-intermittent-failures--bug-765389, browser_461743_sample.html) \
> +	browser_911547.js \
> +	browser_911547_sample.html \
> +	browser_911547_sample.html^headers^ \

I forgot to update this patch fully after pulling the tests out into another patch, so ignore these stray entries in Makefile.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #18)
> To be clear, I can't reproduce the "alert comes back" behavior you're
> looking for in comment 2.

Me neither ;-)
Seems as if FF steered clear of that due to other things (bit of luck?).


> The data: URI that's loaded after click does indeed have a suppressed alert
> before kill and an active alert after restore.

That's what I initially reported.


> The same behavior should come in session restore of an HTTP page, right?

That's what Mozillians here suggested but I was never able to trigger that behaviour.
Flags: needinfo?(mozilla.org)
Comment on attachment 803372 [details] [diff] [review]
tests

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

::: browser/components/sessionstore/test/browser_911547.js
@@ +27,5 @@
> +
> +  // attempt to inject and run a script via data URI (pre-restore)
> +  injectDataURIScript(browser, SCRIPT_INJECT);
> +  is(browser.contentDocument.getElementById("test_id").value, "ok",
> +     "CSP didn't block the script that modifies test_id");

I was confused for a minute whether the message here was wrong or not. This message is shown when the test passes, too. So I think this should express our expecations, or what we're testing here.
Attachment #803372 - Flags: review?(ttaubert) → review+
Thanks, Tim.  I'll need to rewrite this test to simulate more accurately the STR in this bug's initial comment.  I'll rewrite those strings when I do that work.
Hmm, I also noticed that the test isn't failing for me locally. Maybe that's what you mean by simulating more accurately? Anyway we should make sure the test fails without the patch applied.
Yeah, the test simulates only session-restore of the HTTP page with a CSP.  It needs to be modified to simulate the data: link click and restore.  That should fail without the patch, but is more complicated to create than the currently posted test.
Attached patch tests (obsolete) — Splinter Review
Ok, this should test the right behavior.

The WIP attached to this bug doesn't work, but these tests should reproduce the desired situation (link click to data: document, close and restore the document, want CSP to block the script).  Tim, please take a look and let me know if this looks reasonable.
Attachment #803372 - Attachment is obsolete: true
Attachment #814194 - Flags: review?(ttaubert)
Attached patch wip (obsolete) — Splinter Review
This patch works better.  I had to generate a classinfo record for ContentSecurityPolicy implementation.

But it's not quite done.  I'm still trying to figure out why the serialized CSP can't be read back in. Getting this error:

 "(NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_deserializeHistoryEntry :: line 2895"

I'm confused because the last line in contentSecurityPolicy.js generates NSGetFactory.  I must be missing a step.
Attachment #803389 - Attachment is obsolete: true
Comment on attachment 814194 [details] [diff] [review]
tests

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

The test looks good and it fails when run locally. Seems like it covers the issue, thanks!
Attachment #814194 - Flags: review?(ttaubert) → review+
Still trying to debug my WIP.  The NS_ERROR_FACTORY_NOT_REGISTERED comes from the wrong CID being written out during serialization of mCSP in nsPrincipal.  Not yet sure why that wrong CID gets written out, but when it's read back in the object input stream balks.

CID being written/read is: 
{fffedae4-7fff-0000-102c-dec7ff7f0000}

At nsBinaryStream.cpp:264 (in nsBinaryOutputStream::WriteCompoundObject)
(gdb) p/x cid->m3
$15 = {0x10, 0x2c, 0xde, 0xc7, 0xff, 0x7f, 0x0, 0x0}
(gdb) p/x cid->m2
$16 = 0x0
(gdb) p/x cid->m1
$17 = 0x7fff
(gdb) p/x cid->m0
$18 = 0xfffedae4

Should be (in contentSecurityPolicy.manifest):
{d1680bb4-1ac0-4772-9437-1188375e44f2}
Depends on: 960694
Yeah, turns out bug 960694 is the answer.  Running a cleaned up patch through the sessionstore and CSP tests, will post shortly.
Attached patch testsSplinter Review
Had a little bitrot, minor makefile/manifest changes.  Carrying over r=ttaubert
Attachment #814194 - Attachment is obsolete: true
Attachment #8361314 - Flags: review+
Attached patch proposed patch (obsolete) — Splinter Review
This seems to fix it.  

jst: can I get a review here since you've seen the code already?

grobinson: flagging you for review on the changes to CSP bits.
Attachment #814252 - Attachment is obsolete: true
Attachment #8361315 - Flags: review?(jst)
Attachment #8361315 - Flags: review?(grobinson)
Hm, browser/components/sessionstore/test/browser_819510_perwindowpb.js seems to break locally on this, still digging into it.

 0:54.64 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_819510_perwindowpb.js | leaked until shutdown [nsGlobalWindow #108 about:blank]
 0:54.64 TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_819510_perwindowpb.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xul]
 0:54.64 TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_819510_perwindowpb.js | leaked 1 window(s) until shutdown [url = about:blank]
I should probably also add that if you want to apply this to your own tree, apply the fix for bug 960694 first, then the patches attached to this bug.
Comment on attachment 8361315 [details] [diff] [review]
proposed patch

Looks good, a few minor comments you should look into below before landing:

- In nsIContentSecurityPolicy.idl

   /**
    * Called after the CSP object is created to fill in the appropriate request
    * and request header information needed in case a report needs to be sent.
    */
-  void scanRequestData(in nsIHttpChannel aChannel);
+  //void scanRequestData(in nsIHttpChannel aChannel);

Remove this instead of just commenting it out.

- In contentSecurityPolicy.js:

   /**
    * Given an nsIHttpChannel, fill out the appropriate data.
    */
   scanRequestData:
   function(aChannel) {
-    if (!aChannel)
+    setRequestContext(null, null, null, aChannel);
+  },

This can go too, cause it's no longer in the idl. No?

- In read():

+      // shouldn't need self info because when the policy is turned back into a
+      // string, 'self' is replaced with the explicit source expression.

We should make up our minds on this, we either need self info or we don't, and update the comment to reflect that :)

- In write():

+    // we can't serialize a reference to the principal that triggered this
+    // instance to serialize, so when this is deserialized by the principal the
+    // caller should hook it up manually by calling scanRequestData on this

Fix the reference to the removed scanRequestData method her.
Attachment #8361315 - Flags: review?(jst) → review+
Comment on attachment 8361315 [details] [diff] [review]
proposed patch

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

r=me with nits addressed.

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "nsISerializable.idl"

Is this leftover?

@@ +156,5 @@
>    /**
>     * Called after the CSP object is created to fill in the appropriate request
>     * and request header information needed in case a report needs to be sent.
>     */
> +  //void scanRequestData(in nsIHttpChannel aChannel);

Remove this if it's no longer being used.

::: content/base/src/contentSecurityPolicy.js
@@ +124,5 @@
>    // nsIContentPolicy type as XHR (which is fixed)
>  }
>  
>  ContentSecurityPolicy.prototype = {
> +  classID:          CSP_CLASS_ID, 

trailing whitespace
Attachment #8361315 - Flags: review?(grobinson) → review+
(In reply to Garrett Robinson [:grobinson] from comment #36)
> > +#include "nsISerializable.idl"
> 
> Is this leftover?

Nope, nsIContentSecurityPolicy implements the methods for nsISerializable, so we need to include the base interface.  Thanks for the reviews.
Attached patch proposed patchSplinter Review
Updated as per review comments.  Carrying over r=jst,grobinson
Attachment #8361315 - Attachment is obsolete: true
Attachment #8364734 - Flags: review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #33)
> Hm, browser/components/sessionstore/test/browser_819510_perwindowpb.js seems
> to break locally on this, still digging into it.

Looks like this broken test was disabled due to intermittent failures on linux (my local environment) in bug 894063.
Looking good locally, I'm testing the changes on try (without the new tests):
 
https://tbpl.mozilla.org/?tree=Try&rev=027ab91ce402
Wow, okay, that try run was all sorts of orange.  Digging into it now.
Comment on attachment 8364734 [details] [diff] [review]
proposed patch

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

::: content/base/src/contentSecurityPolicy.js
@@ +389,2 @@
>  
>      if (aChannel instanceof Ci.nsIHttpChannel && aChannel.referrer) {

That's embarassing.  This if statement is leftover from rebasing the patch and should have been removed... it caused a syntax error in contentSecurityPolicy.js, making all the CSP tests go down in a ball of fire.
One more try, this time only debug and mochi/xpcshell tests since that's what burned:
https://tbpl.mozilla.org/?tree=Try&rev=c5596a3f5145
Try looks good.  Just waiting on bug 960694 to land.
960694 landed, looked good on inbound.  Landed these patches on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/80c7fa20601d
https://hg.mozilla.org/integration/mozilla-inbound/rev/461927a6e020
Blocks: CSP
Target Milestone: --- → mozilla29
sec-lows aren't being uplifted to b2g26. My guess is that esr24 is a wontfix as well, but will leave that to RelMan.

Sid, can you please nominate for Aurora please? :)
Comment on attachment 8364734 [details] [diff] [review]
proposed patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): session restore did not save CSP
User impact if declined: Session-restored documents will not have CSP restrictions applied
Testing completed (on m-c, etc.): on m-c, has unit tests (See other patch) 
Risk to taking this patch (and alternatives if risky): minor risk of interference with other session restore capabilities, but test coverage there is pretty good so risk is low.
String or IDL/UUID changes made by this patch: nsContentSecurityPolicy.idl, no strings.
Attachment #8364734 - Flags: approval-mozilla-aurora?
Comment on attachment 8361314 [details] [diff] [review]
tests

[Approval Request Comment]
This goes with the other patch.  Same info.
Attachment #8361314 - Flags: approval-mozilla-aurora?
I should add that if we uplift to aurora, we must uplift 960694 too.  I'll nominate the patch in that bug.
Since this is sec-low it doesn't meet ESR landing criteria, marking wontfix.
Attachment #8364734 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8361314 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #48)

> String or IDL/UUID changes made by this patch: nsContentSecurityPolicy.idl,
> no strings.

Since this is only going up to Aurora, the IDL change is fine.
This doesn't apply cleanly to Aurora. Please provide a branch patch or request approval for any other dependencies this has.
Flags: needinfo?(sstamm)
ryanvm: yeah, I noticed and rebased the patches.  It was not completely trivial so I'd like to run some tests locally before pushing to aurora.  I'll try to do this tomorrow (or at the very least post the rebased patches that will build on aurora).
Attached patch csp-serializeSplinter Review
patch rebased for mozilla-aurora
Attachment #8367520 - Flags: review+
the two patches I just attached apply cleanly to mozilla-aurora and the CSP xpcshell-tests and content/base/test/* mochitests pass locally.
Flags: needinfo?(sstamm)
Matt, can you please verify this is fixed?
Flags: needinfo?(mwobensmith)
Flags: needinfo?(mwobensmith)
Flags: in-testsuite?
I'm working with contributor Nagasahas Dasa on this bug. We'll update it shortly with our verification results.
With the help of steps to reproduce in the comment-0 and using a similar example - an alert generated from a page opened via data: protocol - I was able to reproduce this on affected builds Fx 26 and Fx 27.  I can confirm that the issue is resolved on Fx 28 and Fx 29.0a1 from 2014-02-26.
Whiteboard: [adv-main28+]
Alias: CVE-2014-1504
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: