Christoph Kerschbaumer [:ckerschb] Assignee
	Description • 10 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

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 1 • 10 years ago

Francois, do you wanna take this one?

	François Marier [:francois]
	Comment 2 • 10 years ago

Sure.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 3 • 10 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.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 4 • 9 years ago

That should fix the problem, but we still need a testcase for it.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 6 • 9 years ago

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.

	Sid Stamm [:geekboy or :sstamm]
	Comment 7 • 9 years ago

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

	Sid Stamm [:geekboy or :sstamm]
	Comment 8 • 9 years ago

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?

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 9 • 9 years ago

(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!

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 10 • 9 years ago

(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+.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 11 • 9 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

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 12 • 9 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

	James Graham [:jgraham]
	Comment 13 • 9 years ago

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.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 14 • 9 years ago

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.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 15 • 9 years ago

> @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.

	Sid Stamm [:geekboy or :sstamm]
	Comment 16 • 9 years ago

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'?

	James Graham [:jgraham]
	Comment 17 • 9 years ago

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.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 18 • 9 years ago

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.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 19 • 9 years ago

Oops - forgot to qref before uploading the patch - here we go.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f23080673ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb83b6efa9ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/772945b1130d

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 9 years ago

Backed out for Gaia context_menu_test.js integration test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/84f7fa96af4f

https://treeherder.mozilla.org/logviewer.html#?job_id=7938256&repo=mozilla-inbound

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 22 • 9 years ago

Potentially also causing intermittents for Bug 1138973 and Bug 1124091

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 23 • 9 years ago

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

	[:fabrice] Fabrice Desré
	Comment 24 • 9 years ago

I'm gonna redirect to Paul that knows that much better than I do.

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 25 • 9 years ago

(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)

	[:fabrice] Fabrice Desré
	Comment 26 • 9 years ago

(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.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 27 • 9 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!

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 28 • 9 years ago

(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.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 29 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaf3cccabac
https://hg.mozilla.org/integration/mozilla-inbound/rev/c683f8d5426d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8aa222f1d01
https://hg.mozilla.org/integration/mozilla-inbound/rev/183190289b9c

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3aaf3cccabac
https://hg.mozilla.org/mozilla-central/rev/c683f8d5426d
https://hg.mozilla.org/mozilla-central/rev/a8aa222f1d01
https://hg.mozilla.org/mozilla-central/rev/183190289b9c

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 9 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 32 • 9 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 9 years ago

 e) As (b) but only on sites where there is a CSP.  Might be confusing to people, though.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 34 • 9 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

	Sebastian Zartner [:sebo]
	Comment 35 • 9 years ago

(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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 36 • 9 years ago

Tracking this for Firefox 40 since it doesn't seem to be completely resolved and we're seeing multiple reports of sites breaking.

	Sebastian Zartner [:sebo]
	Comment 37 • 9 years ago

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