1299483 - CSP: Implement 'strict-dynamic'

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

strict-dynamic [1] causes certain srcs within script-src or also default-src to be invalidated. Instead of having nsCSPKeywordSrc host the invalidate() method, which was used to invalidate 'unsafe-inline' we are now moving the invalidate() functionality into the base class which allows to invalidate all srcs where nsCSPHashSrc as well as nsCSPNonceSrc() overwrite that method to make sure they can't be invalidated by strict-dynamic.

Further, strict-dynamic only applies to parser inserted (created) scripts, hence we are extending the permits() method family with an additional argument so we can distinguish what scripts are actually created by the HTML parser.

Finally, within the parser we actually have to call the invalidate() function on all those srcs in case a policy includes strict-dynamic. Please note, since our compiler flags don't allow dynamic-cast so we could distinguish between regular srcs and hash, nonce and strict-dynamic srcs, we actually have to use a little hack, call the toString() function and don't log to the console in case we are calling invalidate() on any of those srcs. Open to suggestions if there is a better solution, but I doubt there is without massive refactoring involved.

We host our own parser tests, and mochitests for simple strict-dynamic cases but also for parser and non parser inserted scripts. Finally, there is also [2] to make sure our implementation matches Chrome's implementation.

[1] https://www.w3.org/TR/CSP3/#strict-dynamic-usage
[2] https://poc.miki.it/strict-dynamic-tests/

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_strict_dynamic_enforcement_changes.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_strict_dynamic_parser_changes.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_strict_dynamic_parser_tests.patch

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_implement_strict_dynamic_tests.patch

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch

Comment 7

[Frederik Braun [:freddy]]

Comment on attachment 8793972 [details] [diff] [review]
bug_1299483_strict_dynamic_enforcement_changes.patch

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

Enforcement looks correct.

::: dom/security/nsCSPUtils.h
@@ +211,5 @@
>      virtual void toString(nsAString& outStr) const = 0;
> +
> +    void invalidate()
> +      { mInvalidated = true; }
> + 

Nit: trailing whitespace.

Comment 8

[Frederik Braun [:freddy]]

Comment on attachment 8793973 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_changes.patch

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

::: dom/security/nsCSPParser.cpp
@@ +1239,5 @@
> +    if (!mHasHashOrNonce) {
> +      const char16_t* params[] = { mCurDir[0].get() };
> +      logWarningErrorToConsole(nsIScriptError::warningFlag, "strictDynamicButNoHashOrNonce",
> +                               params, ArrayLength(params));
> +    } 

Nit: Whitespace

Comment 9

[Frederik Braun [:freddy]]

Comment on attachment 8793974 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_tests.patch

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

::: dom/security/test/TestCSPParser.cpp
@@ +163,5 @@
>  }
>  
> +// what we need to test:
> +// script-src 'unsafe-inline' 'unsafe-inline'
> +// script-src 'nonce-foo' 'unsafe-inline'

The latter is already in this patchset, so please omit.
And why not add the former in this patch.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

There is also the following testing site: https://poc.miki.it/strict-dynamic-tests/

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_strict_dynamic_enforcement_changes.patch

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8798804 [details] [diff] [review]
bug_1299483_strict_dynamic_enforcement_changes.patch

Dan, please have a look at comment 1 for a brief description of the patch.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_strict_dynamic_parser_changes.patch

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_strict_dynamic_parser_tests.patch

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_implement_strict_dynamic_tests.patch

Dan, Freddy, I chatted with Google folks and extended our mochitests, hence putting up new patches.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8798806 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_tests.patch

Ups, wrong flag, this patch obviously needs review from Dan.

Comment 18

[Frederik Braun [:freddy]]

Comment on attachment 8798808 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch

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

Looking good! Sorry for the long wait.

::: dom/security/test/csp/file_strict_dynamic_unsafe_eval.html
@@ +5,5 @@
> +</head>
> +<body>
> +<div id="testdiv">blocked</div>
> +
> +<script nonce="foo">

Nit: Add a comment that explains what the test is doing and what should happen, like in the other files.
e.g., // 'strict-dynamic' should not invalidate 'unsafe-eval', this innerHTML should change.

Comment 19

[Frederik Braun [:freddy]]

Comment on attachment 8798807 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_tests.patch

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

Good stuff

::: dom/security/test/csp/test_strict_dynamic.html
@@ +84,5 @@
> +    return;
> +  }
> +
> +  curTest = tests[counter++];
> +  var src = "file_testserver.sjs?file=";

(By the way: Server-Side Scripts like these make the tiny devil on my shoulder snicker)

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 8798807 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_tests.patch

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

Looks good, and I'll assume you're going to update that comment.

::: dom/security/test/csp/test_strict_dynamic.html
@@ +27,5 @@
> +  {
> +    desc: "strict-dynamic with valid nonce should be allowed",
> +    result: "allowed",
> +    file: "file_strict_dynamic_script_extern.html",
> +    policy: "script-src 'strict-dynamic' 'nonce-foo' https: 'none' 'self'"

Apart from strict-dynamic (where the whitelist is ignored), what do we do when we get a nonsense policy with 'none' and something else?

'none' would seem to conflict with nonce and hash, too. "'none' means 'none'!"

@@ +39,5 @@
> +  {
> +    desc: "strict-dynamic, unsafe-inline and invalid nonce should be blocked",
> +    result: "blocked",
> +    file: "file_strict_dynamic_script_extern.html",
> +    policy: "default-src 'strict-dynamic' 'nonce-bar' 'unsafe-inline' http: http://example.com"

should this be file_strict_dynamic_script_inline.html ? Why would 'unsafe-inline' do anything with the script_extern test? Or else change the comment and say that you're really testing the site whitelist being ignored.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 8798808 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch

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

r=dveditz

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 8798807 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_tests.patch

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

Sorry to pick on this one. I see in the code that we check default-src too, but there are no tests for default-src 'strict-dynamic'. I'm also concerned that the code that invalidates the whitelist might be doing it in the default-src case (might be misunderstanding the code, still looking) so I'd like to see a test like
  csp: default-src 'strict-dynamic' nonce-foo http://example.com;
and make sure you can still e.g. load an image from example.com and that the whitelist zapping only applies to script loads.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #20)
> Apart from strict-dynamic (where the whitelist is ignored), what do we do
> when we get a nonsense policy with 'none' and something else?
> 
> 'none' would seem to conflict with nonce and hash, too. "'none' means
> 'none'!"

Back when we implemented CSP there was a part in the spec that 'none' should be ignored in case there are any other srcs available within a directive. We even have a comment in the code [1]. Now, looking through all the specs of CSP, I can't find that part anymore. I still think it's fine to ignore 'none' if there are other srcs available. If you feel different, I am happy to file a follow up so we can investigate and start a discussion there.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#817

 
> should this be file_strict_dynamic_script_inline.html ? Why would
> 'unsafe-inline' do anything with the script_extern test? Or else change the
> comment and say that you're really testing the site whitelist being ignored.

Thanks for pointing that out - I updated that part for the final patch.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_enforce_strict_dynamic_only_in_script_src.patch

(In reply to Daniel Veditz [:dveditz] from comment #22)
> Sorry to pick on this one. I see in the code that we check default-src too,
> but there are no tests for default-src 'strict-dynamic'. I'm also concerned
> that the code that invalidates the whitelist might be doing it in the
> default-src case (might be misunderstanding the code, still looking) so I'd
> like to see a test like
>   csp: default-src 'strict-dynamic' nonce-foo http://example.com;
> and make sure you can still e.g. load an image from example.com and that the
> whitelist zapping only applies to script loads.

Dan, so this is a very interesting point and you are absolutely right. Current implementation would invalidate *all* srcs within default-src hence also blocking images from loading (if no img-src defined and images would fall back to default-src).

So here is what I propose:
* Within this patch I slightly updated the parser patch to allow strict-dynamic only to appear within script-src. (Before landing I would merge that patch with bug_1299483_strict_dynamic_parser_changes.patch). I further started a discussion on the mailing list because I think it makes no sense to allow strict-dynamic within default-src. Reason being is that strict-dynamic requires a valid nonce anyway. Nonces are only allowed to be defined within script-src. I further filed Bug 1313937 in case we are going to allow strict-dynamic to be defined within default-src. For this first version, I would like to only allow strict-dynamic within script-src. The updated patch logs a warning to the console mentioning that strict-dynamic is only supported within script-src in Firefox.

* I would like to keep bug_1299483_implement_strict_dynamic_tests.patch the way the are, so please re-consider your r-. But to follow up on your suggestions I assembled yet another test (test_strict_dynamic_default_src.html) which also includes some 'todo_is()' tests to make sure that the whitelist will not be dropped for images within default-src but scripts would still be blocked correctly.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1299483_implement_strict_dynamic_test_default_src.patch

Comment 26

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb (limited availability)] from comment #23)
> Back when we implemented CSP there was a part in the spec that 'none' should
> be ignored in case there are any other srcs available within a directive. We
> even have a comment in the code [1]. Now, looking through all the specs of
> CSP, I can't find that part anymore.

I found https://w3c.github.io/webappsec-csp/2/#source-list-parsing which I think says the above. If the source list matches just 'none' then it's none, otherwise you split on spaces and parse tokens. No mention of what happens when you do that and then find 'none' anyway--guess it's just ignored. It's clearly an error on the developer's part. Being strict (obeying 'none' anyway) would encourage them to fix it, but if not we should think about giving an error message on the console.

(In reply to Christoph Kerschbaumer [:ckerschb (limited availability)] from comment #24)
> Dan, so this is a very interesting point and you are absolutely right.
> Current implementation would invalidate *all* srcs within default-src hence
> also blocking images from loading (if no img-src defined and images would
> fall back to default-src).

We should probably internally keep a "script-src" slot, and if it's not literally present then duplicate default-src into each missing directive (probably with a flag that it came from default-src for error messaging). That way it can be parsed/managed individually for cases like this.

> So here is what I propose:
> * Within this patch I slightly updated the parser patch to allow
> strict-dynamic only to appear within script-src.

I predict that the group is likely to say 'strict-dynamic' should be allowed in default-src in the end. This is OK for now though.

> sense to allow strict-dynamic within default-src. Reason being is that
> strict-dynamic requires a valid nonce anyway. Nonces are only allowed to be
> defined within script-src.

I believe that is incorrect. nonces are in the "allowed script sources", which the spec defines as "The term allowed script sources refers to the result of parsing the script-src directive’s value as a source list if the policy contains an explicit script-src, or otherwise to the default sources."

Comment 27

[Michele Spagnuolo]

FYI, I have committed 'strict-dynamic' Web Platform Tests under https://github.com/w3c/web-platform-tests/tree/master/content-security-policy/script-src. It would be cool if you could verify your implementation using them and let me know if you have any question.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 8805869 [details] [diff] [review]
bug_1299483_enforce_strict_dynamic_only_in_script_src.patch

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

r=dveditz

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 8805870 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_test_default_src.patch

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

r=dveditz

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8798807 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_tests.patch

Given the IRC conversation with Dan, he forgot to r+ this patch.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c9d6da9c430a28c5ee0c56ae6dd031498f54a89

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f5124ac4b6
CSP: Implement 'strict-dynamic', enforcement changes. r=dveditz,freddyb
https://hg.mozilla.org/integration/mozilla-inbound/rev/908b79c37888
CSP: Implement 'strict-dynamic', parser changes. r=dveditz,freddyb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d16e2f01a122
CSP: Implement 'strict-dynamic', parser tests. r=dveditz,freddyb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a802f55a2d95
CSP: Implement 'strict-dynamic', mochitests. r=dveditz,freddyb
https://hg.mozilla.org/integration/mozilla-inbound/rev/201b2637eac6
CSP: Implement 'strict-dynamic', parser inserted mochitests. r=dveditz,freddyb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c84b104987
CSP: Implement 'strict-dynamic', test default-src. r=dveditz

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22f5124ac4b6
https://hg.mozilla.org/mozilla-central/rev/908b79c37888
https://hg.mozilla.org/mozilla-central/rev/d16e2f01a122
https://hg.mozilla.org/mozilla-central/rev/a802f55a2d95
https://hg.mozilla.org/mozilla-central/rev/201b2637eac6
https://hg.mozilla.org/mozilla-central/rev/f3c84b104987

Comment 34

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/52#HTTP
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#strict-dynamic