Bug 1086999 (CVE-2015-4490)

CSP: Asterisk (*) wildcard should not allow blob:, data:, or filesystem: when matching source expressions

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete, sec-moderate, site-compat})

unspecified
mozilla40
dev-doc-complete, sec-moderate, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40+ fixed)

Details

(Whiteboard: [adv-main40+])

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

4 years ago
There is a discrepancy in our CSP implementation and the spec. The spec says [1] that blob:, data:, and filesystem: should be excluded in case of a wildcard (allow all) when matching source expressions.

Currently we allow all schemes in case of an asterisk wildcard, e.g. here: [2].

We should update our implementation to follow the spec.

[1] http://www.w3.org/TR/CSP11/#match-source-expression
[2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#291
(Assignee)

Updated

4 years ago
Blocks: 493857
(Assignee)

Comment 1

4 years ago
Francois, do you wanna take this one?
Flags: needinfo?(francois)
Sure.
Status: NEW → ASSIGNED
Flags: needinfo?(francois)
Assignee: nobody → francois
(Assignee)

Updated

4 years ago
Priority: -- → P1
(Assignee)

Comment 3

4 years ago
It's not a dependency, but I think Bug 1021669 is closely related to this one - Francois a CC'd you on Bug 1021669 as well.
Keywords: sec-moderate
(Assignee)

Comment 4

3 years ago
Created attachment 8560739 [details] [diff] [review]
bug_1086999.patch

That should fix the problem, but we still need a testcase for it.
Assignee: francois → mozilla
(Assignee)

Comment 5

3 years ago
Created attachment 8562376 [details] [diff] [review]
bug_1086999.patch
Attachment #8560739 - Attachment is obsolete: true
Attachment #8562376 - Flags: review?(sstamm)
(Assignee)

Comment 6

3 years ago
Created attachment 8562377 [details] [diff] [review]
bug_1086999_tests.patch

Please note that I choose the onload/onerror solution on purpose. I think we have enough tests that rely on observers. I'd rather have a variation of tests hence that choice.
Attachment #8562377 - Flags: review?(sstamm)
Comment on attachment 8562376 [details] [diff] [review]
bug_1086999.patch

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

r=me with filesystem->file changes here and in nsCSPService.cpp.

::: dom/security/nsCSPUtils.cpp
@@ +386,5 @@
> +      (NS_SUCCEEDED(aUri->SchemeIs("blob", &isBlobScheme)) && isBlobScheme);
> +    bool isDataScheme =
> +      (NS_SUCCEEDED(aUri->SchemeIs("data", &isDataScheme)) && isDataScheme);
> +    bool isFileScheme =
> +      (NS_SUCCEEDED(aUri->SchemeIs("filesystem", &isFileScheme)) && isFileScheme);

I think you want "file" here, not "filesystem".  The WG should probably fix that, or clarify where "filesystem" scheme is defined (I can't find it used anywhere).

When you change this to "file", please update nsCSPService.cpp too.
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#68
Attachment #8562376 - Flags: review?(sstamm) → review+
Comment on attachment 8562377 [details] [diff] [review]
bug_1086999_tests.patch

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

r=me if you apply the right CSP for each test (see below) and verify that the tests work before landing.  I'm happy to re-review too if you want.

It's nice to see the event-based testing.  Do you think anything could cause neither one of the "onload" or "onerror" events to fire?

::: dom/base/test/csp/test_blob_data_schemes.html
@@ +57,5 @@
> +  messageCounter = 0;
> +
> +  var src = "file_csp_testserver.sjs";
> +  // append the file that should be served
> +  src += "?file=" + escape("tests/dom/base/test/csp/file_blob_data_schemes.html");

Random question: why are the CSP tests still in dom/base?  Did we decide not to move them when we moved csp into dom/security?

@@ +59,5 @@
> +  var src = "file_csp_testserver.sjs";
> +  // append the file that should be served
> +  src += "?file=" + escape("tests/dom/base/test/csp/file_blob_data_schemes.html");
> +  // append the CSP that should be used to serve the file
> +  src += "&csp=" + escape("default-src 'unsafe-inline' * blob: data:");

Don't you want curTest.policy here instead of a CSP in a string literal?
Attachment #8562377 - Flags: review?(sstamm) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8575573 [details] [diff] [review]
bug_1086999.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> I think you want "file" here, not "filesystem".  The WG should probably fix
> that, or clarify where "filesystem" scheme is defined (I can't find it used
> anywhere).
> 
> When you change this to "file", please update nsCSPService.cpp too.
> http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.
> cpp#68

Thanks - that's a great catch - updated!
Attachment #8562376 - Attachment is obsolete: true
Attachment #8562377 - Attachment is obsolete: true
Attachment #8575573 - Flags: review+
(Assignee)

Comment 10

3 years ago
Created attachment 8575577 [details] [diff] [review]
bug_1086999_tests.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8)
> r=me if you apply the right CSP for each test (see below) and verify that
> the tests work before landing.  I'm happy to re-review too if you want.

Thanks Sid, addressed your concerns, the reason it was working is because I used ok() instead of is(). ok() just makes sure the first argument is non-null - updated those bits - still works.
 
> It's nice to see the event-based testing.  Do you think anything could cause
> neither one of the "onload" or "onerror" events to fire?

I don't think onload can cause problems, but I see onError overreporting, hence I slighlty updated to patch to wait for the callback that data and blob actually ran.

> Random question: why are the CSP tests still in dom/base?  Did we decide not
> to move them when we moved csp into dom/security?

It's blocked by some WebRTC problems - Nils is on it to get the problem fixed - once fixed we can finally move the tests into dom/security - I really want that to happen sooner than later.
 
> @@ +59,5 @@
> > +  var src = "file_csp_testserver.sjs";
> > +  // append the file that should be served
> > +  src += "?file=" + escape("tests/dom/base/test/csp/file_blob_data_schemes.html");
> > +  // append the CSP that should be used to serve the file
> > +  src += "&csp=" + escape("default-src 'unsafe-inline' * blob: data:");
> 
> Don't you want curTest.policy here instead of a CSP in a string literal?

Yes, explained above I was using ok() instead of is() - works correctly now.

I don't think there is need for a re-review - carrying over r+.
Attachment #8575577 - Flags: review+
(Assignee)

Comment 11

3 years ago
I had this patch on TRY [1] and apparently:
> ./mach web-platform-tests testing/web-platform/tests/content-security-policy/script-src/script-src-1_9.html
is not running anymore. Reason is that the policy used for the test is:
> default-src *; report-uri ...
which then blocks
> blob:http://web-platform.test:8000/d20996e0-961a-42a1-af82-244bfb1768fd

Since we are changing that blob and data URLs have to be whitelisted explicitly in the CSP policy in this bug we are blocking that request correctly, but now the test needs to be updated.

Interesting note, even without the patch the test does not pass, neither on Firefox, nor in Chrome, but the reason the mochitest-suite is complaining is the test is "NOT RUN" at all.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ad9ee25eed8
(Assignee)

Comment 12

3 years ago
James, I see you checked in the latest web-platform-tests [1]. I found a compatibility problem (see Comment 11). Can you tell me the workflow how we update our web-platform tests? Where do we pull them from? I suppose I am not going to fix the test problem in mc, right? Potentially I have to create a pull request so the tests use a updated CSP which whitelists 'blobs' explicitly.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1118722
Flags: needinfo?(james)
You actually can just do the fix in m-c and it will get upstreamed next time we do an update. Or you can do the fix in https://github.com/w3c/web-platform-tests and it will gt downstreamed next time we do an update.
Flags: needinfo?(james)
(Assignee)

Comment 14

3 years ago
Created attachment 8576334 [details] [diff] [review]
bug_1086999.patch

Dan brought to my attention that it indeed should be 'filesystem:' and not 'file:' - I added a comment to make sure we are spec compliant even though firefox does not support 'filesystem:' schemes at the moment.
Attachment #8575573 - Attachment is obsolete: true
Attachment #8576334 - Flags: review+
(Assignee)

Comment 15

3 years ago
Created attachment 8576337 [details] [diff] [review]
bug_1086999_web_platform_test_update.patch

> @Sid:
I had to change this web-platform test since the blob is now blocked correctly, hence the postMessage can't fire anymore.

In fact, we should also send a violation report, but at the moment we don't because we incorrectly identify the load as a preLoad [1].

We have already identified the problem of incorrectly classifying preloads once a while ago [2] - we have to fix that at some point.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#163
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1048048#c4

> @James:
Thanks for the speedy response. Does the change look reasonable to you? I would rather land on mc and then have you do the upstream instead of the other way round.
Attachment #8576337 - Flags: review?(sstamm)
Attachment #8576337 - Flags: review?(james)
Comment on attachment 8576337 [details] [diff] [review]
bug_1086999_web_platform_test_update.patch

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

::: testing/web-platform/tests/content-security-policy/script-src/buildInlineWorker.js
@@ +24,2 @@
>      test.step(function () {
> +       assert_true(true, 'inline script not ran');

Nit: grammar.  Maybe 'inline script was blocked'?
Attachment #8576337 - Flags: review?(sstamm) → review+
Comment on attachment 8576337 [details] [diff] [review]
bug_1086999_web_platform_test_update.patch

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

Just some style issues.

::: testing/web-platform/meta/content-security-policy/script-src/script-src-1_9.html.ini
@@ +1,4 @@
>  [script-src-1_9.html]
>    type: testharness
>    [test inline worker]
> +    expected: PASS

You don't need expected: PASS, that's the default. Just remove the whole subsection.

::: testing/web-platform/tests/content-security-policy/script-src/buildInlineWorker.js
@@ +7,5 @@
>  
>   // can I create a new script tag like this? ack...
>   var url = window.URL.createObjectURL(blob);
>  
> + try {

So I haven't looked at the spec here, I'm just judging the test as a whole. I think the intent isn't very clear; it's not obvious what can fail. So based on what I think I understand, the test should look more like:

var t = async_test("inline worker");

t.step(function() {
  var workerSource = document.getElementById('inlineWorker');
  var blob = new Blob([workerSource.textContent]);
  try {
    var worker = new Worker(url);
  catch(e) {
    // Would be nice to assert something about e here
    // But otehrwise the test passes if trying to construct the worker throws
    t.done();
  }
  // Not sure why this isn't assert_unreached() to fail the test…
  worker.addEventListener("message", t.step_func(function(e) {
    assert_unreached("script ran");
  })

  worker.postMessage("");
})

And actually, you can probably strip most of the boilerplate away thus:

var workerSource = document.getElementById('inlineWorker');
var blob = new Blob([workerSource.textContent]);
try {
  var worker = new Worker(url);
catch(e) {
  // Would be nice to assert something about e here
  // But otehrwise the test passes if trying to construct the worker throws
  done();
}
// Not sure why this isn't assert_unreached() to fail the test…
worker.addEventListener("message", function(e) {
    assert_unreached("script ran");
})

worker.postMessage("");


Which works because there's only one test in the file.
Attachment #8576337 - Flags: review?(james) → review-
(Assignee)

Comment 18

3 years ago
Created attachment 8580349 [details] [diff] [review]
bug_1086999_web_platform_test_update.patch

James, thanks for the info, that all makes sense to me. Unfortunately there is nothing we can assert before we call done() in the catch-block.

The only thing we can do is to fix the preload problem I described so that we actually send out a violation report so that both tests in that testfile pass. But that might take a while, it's not super high up in my priority list at the moment.
Attachment #8576337 - Attachment is obsolete: true
Attachment #8580349 - Flags: review?(james)
(Assignee)

Comment 19

3 years ago
Created attachment 8580352 [details] [diff] [review]
bug_1086999_web_platform_test_update.patch

Oops - forgot to qref before uploading the patch - here we go.
Attachment #8580349 - Attachment is obsolete: true
Attachment #8580349 - Flags: review?(james)
Attachment #8580352 - Flags: review?(james)
(Assignee)

Updated

3 years ago
Blocks: 878608
Attachment #8580352 - Flags: review?(james) → review+
(Assignee)

Comment 22

3 years ago
Potentially also causing intermittents for Bug 1138973 and Bug 1124091
(Assignee)

Comment 23

3 years ago
Created attachment 8583426 [details] [diff] [review]
bug_1086999_b2g_policy_updates.patch

Fabrice, the csp spec [1] states that the wildcard (*) should not match data:, blob: and filesystem:. We have incorporated that change into our CSP implementation, which is now breaking B2G, specifically it's breaking the following gaia integration test:
> apps/system/test/marionette/context_menu_test.js
which uses the CSP defined in modules/libpref/init/all.js.

I am not sure if I have updated all the right policies in the attached patch. I am also not sure whether this is the right change to perform, maybe the default policy for trusted, privileged or certified apps should not even allow blob: or data: schemes.

One thing to mention (and to be explicit):
> default-src *
is now equivalent to
> default-src * blob: data:

Please let me know how to proceed!


[1] http://www.w3.org/TR/CSP11/#match-source-expression
Flags: needinfo?(fabrice)
Attachment #8583426 - Flags: feedback?(fabrice)
I'm gonna redirect to Paul that knows that much better than I do.
Flags: needinfo?(fabrice) → needinfo?(ptheriault)
Attachment #8583426 - Flags: feedback?(fabrice) → feedback?(ptheriault)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #23)
> Created attachment 8583426 [details] [diff] [review]
> bug_1086999_b2g_policy_updates.patch
> 
> Fabrice, the csp spec [1] states that the wildcard (*) should not match
> data:, blob: and filesystem:. We have incorporated that change into our CSP
> implementation, which is now breaking B2G, specifically it's breaking the
> following gaia integration test:
> > apps/system/test/marionette/context_menu_test.js
> which uses the CSP defined in modules/libpref/init/all.js.
> 
> I am not sure if I have updated all the right policies in the attached
> patch. I am also not sure whether this is the right change to perform, maybe
> the default policy for trusted, privileged or certified apps should not even
> allow blob: or data: schemes.
> 
> One thing to mention (and to be explicit):
> > default-src *
> is now equivalent to
> > default-src * blob: data:
> 
> Please let me know how to proceed!
> 
> 
> [1] http://www.w3.org/TR/CSP11/#match-source-expression

Thanks Christoph - this looks good to me in terms of whats in the policy.  Certified apps use blob: uris all over the place (wallpaper, icons etc), and we mainly care about script (I care about inline styles too, but 968907 blocks using style-src 'self' )

I was going to suggest that we could also just not supply a default-src attribute, but I read from the spec that if default-src is NOT supplied, then it default's to * (i.e. blocking data: & blob:). 

I don't know why we set the privileged CSP preference in a different place to where we set the certified apps CSP, or if this is indeed the correct place though. (TBH I thought we set them in b2g.js, I'm not sure why the privileged one moved to modules/libpref/init/all.js)
Flags: needinfo?(ptheriault)
(In reply to Paul Theriault [:pauljt] from comment #25)

> I don't know why we set the privileged CSP preference in a different place
> to where we set the certified apps CSP, or if this is indeed the correct
> place though. (TBH I thought we set them in b2g.js, I'm not sure why the
> privileged one moved to modules/libpref/init/all.js)

We did that when android and desktop started to support privileged apps.
(Assignee)

Comment 27

3 years ago
Comment on attachment 8583426 [details] [diff] [review]
bug_1086999_b2g_policy_updates.patch

Paul, seems like you are fine with the change, right? If so, could you r+ the patch, otherwise please let me know how to update. Thanks!
Attachment #8583426 - Flags: feedback?(ptheriault) → review?(ptheriault)
Attachment #8583426 - Flags: review?(ptheriault) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)
> Comment on attachment 8583426 [details] [diff] [review]
> bug_1086999_b2g_policy_updates.patch
> 
> Paul, seems like you are fine with the change, right? If so, could you r+
> the patch, otherwise please let me know how to update. Thanks!

Yep - big thanks for this Christoph.

Updated

3 years ago
Depends on: 1155792
Does any browser implement this part of the spec?  We've now had at least two instances of site breakage reported in the two weeks since this patch landed...  It may just be that no one implemented the spec correctly and now the spec is not web-compatible.
Flags: needinfo?(mozilla)
(In reply to Not doing reviews right now from comment #31)
> Does any browser implement this part of the spec?

Our original implementation did (X-Content-Security-Policy header, written in JS). Apparently a casualty of the C++ rewrite.

> We've now had at least two instances of site breakage reported in the two weeks since
> this patch landed...  It may just be that no one implemented the spec correctly and now
> the spec is not web-compatible.

Probably, and the remaining options are really sucky. The downsides to supporting data: don't affect Chrome because they never supported the part of the HTML5 spec that says they should be same-origin with the context that created them. Because we _do_ it is absolutely unsafe to allow '*' to include data: for iframe-src, script-src, object-src, and to a lesser extent script-src. No one's likely to do "script-src *", but if they rely on the same-origin policy they might well leave out frame-src and let it fallback to a "default-src *" -- harmless in Chrome, deadly in Firefox.

Our options seem to be:
 a) insist on the spec interpretation, and tech evangelize the broken sites (how many sites use CSP?)
 b) go ahead and make data: no longer inherit the principal. Although it's the logical equivalent to document.write() it's been a huge footgun for us anyway. Will probably break some Firefox-only content though.
 c) have '*' include data: for image-src and media-src where it's not that dangerous since that's the case we keep breaking. But now the behavior of * is inconsistent depending on CSP directive.
 d) switch the gating permission for data: in the dangerous contexts on the user having set 'unsafe-inline' on the appropriate directive. bonus: we really ought to have done that for iframe srcdoc anyway.
 e) As (b) but only on sites where there is a CSP.  Might be confusing to people, though.

Updated

3 years ago
Depends on: 1157084
(Assignee)

Comment 34

3 years ago
Thanks Boris and Dan for the info/feedback. Unfortunately none of the options mentioned in Comment 32 and Comment 33 seem compelling at the moment.

So far we got three bug reports of web compatibility issues after landing the patches attached to this Bug (1157084, 1154704, 1155792).

After chatting with Dan in person, I think we realistically of two options:
a) We back out the change for now.
b) We file evangelism bugs for the three sites that break and hope they update their sites quickly. And further hope that Mike sees that the same way [1] so other browser's CSP implementations are going to match ours in regards to blocking blob:, data:, if not explicitly whitelisted in the CSP header.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1155792#c7
Flags: needinfo?(mozilla)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #34)
> So far we got three bug reports of web compatibility issues after landing
> the patches attached to this Bug (1157084, 1154704, 1155792).

Bug 1157724 is another one, though this can be easily fixed on the website. So I filed an issue there.[1]

Sebastian

[1] https://github.com/NielsLeenheer/html5test/issues/387
Tracking this for Firefox 40 since it doesn't seem to be completely resolved and we're seeing multiple reports of sites breaking.
tracking-firefox40: --- → +

Updated

3 years ago
Keywords: site-compat

Updated

3 years ago
Keywords: dev-doc-needed
Added a note to https://developer.mozilla.org/en-US/Firefox/Releases/40#Security and added the related schemes to https://developer.mozilla.org/en-US/docs/Web/Security/CSP/CSP_policy_directives.

Let me know if there's anything else to add.

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

3 years ago
Depends on: 1182476
(Assignee)

Updated

3 years ago
No longer blocks: 1186412
Depends on: 1186412
Whiteboard: [adv-main40+]
Alias: CVE-2015-4490

Updated

3 years ago
Depends on: 1193648
You need to log in before you can comment on or make changes to this bug.