Last Comment Bug 1299483 - CSP: Implement 'strict-dynamic'
: CSP: Implement 'strict-dynamic'
Status: RESOLVED FIXED
[domsecurity-active]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Security (show other bugs)
: unspecified
: Unspecified Unspecified
P1 normal (vote)
: mozilla52
Assigned To: Christoph Kerschbaumer [:ckerschb]
:
: Christoph Kerschbaumer [:ckerschb]
Mentors:
Depends on: 1313937 1316826
Blocks: csp-w3c-3
  Show dependency treegraph
 
Reported: 2016-08-31 07:24 PDT by Christoph Kerschbaumer [:ckerschb]
Modified: 2017-01-26 04:25 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
bug_1299483_strict_dynamic_enforcement_changes.patch (26.14 KB, patch)
2016-09-22 15:18 PDT, Christoph Kerschbaumer [:ckerschb]
fbraun: review+
Details | Diff | Splinter Review
bug_1299483_strict_dynamic_parser_changes.patch (10.95 KB, patch)
2016-09-22 15:18 PDT, Christoph Kerschbaumer [:ckerschb]
fbraun: review+
Details | Diff | Splinter Review
bug_1299483_strict_dynamic_parser_tests.patch (3.04 KB, patch)
2016-09-22 15:18 PDT, Christoph Kerschbaumer [:ckerschb]
fbraun: review+
Details | Diff | Splinter Review
bug_1299483_implement_strict_dynamic_tests.patch (6.67 KB, patch)
2016-09-22 15:19 PDT, Christoph Kerschbaumer [:ckerschb]
no flags Details | Diff | Splinter Review
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch (6.45 KB, patch)
2016-09-22 15:19 PDT, Christoph Kerschbaumer [:ckerschb]
no flags Details | Diff | Splinter Review
bug_1299483_strict_dynamic_enforcement_changes.patch (43.72 KB, patch)
2016-10-07 03:25 PDT, Christoph Kerschbaumer [:ckerschb]
ckerschb: review+
dveditz: review+
Details | Diff | Splinter Review
bug_1299483_strict_dynamic_parser_changes.patch (11.05 KB, patch)
2016-10-07 03:26 PDT, Christoph Kerschbaumer [:ckerschb]
ckerschb: review+
dveditz: review+
Details | Diff | Splinter Review
bug_1299483_strict_dynamic_parser_tests.patch (2.73 KB, patch)
2016-10-07 03:26 PDT, Christoph Kerschbaumer [:ckerschb]
ckerschb: review+
dveditz: review+
Details | Diff | Splinter Review
bug_1299483_implement_strict_dynamic_tests.patch (6.70 KB, patch)
2016-10-07 03:28 PDT, Christoph Kerschbaumer [:ckerschb]
fbraun: review+
ckerschb: review+
Details | Diff | Splinter Review
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch (8.25 KB, patch)
2016-10-07 03:28 PDT, Christoph Kerschbaumer [:ckerschb]
fbraun: review+
dveditz: review+
Details | Diff | Splinter Review
bug_1299483_enforce_strict_dynamic_only_in_script_src.patch (4.51 KB, patch)
2016-10-30 14:56 PDT, Christoph Kerschbaumer [:ckerschb]
dveditz: review+
Details | Diff | Splinter Review
bug_1299483_implement_strict_dynamic_test_default_src.patch (7.38 KB, patch)
2016-10-30 14:56 PDT, Christoph Kerschbaumer [:ckerschb]
dveditz: review+
Details | Diff | Splinter Review

Description User image Christoph Kerschbaumer [:ckerschb] 2016-08-31 07:24:15 PDT

    
Comment 1 User image Christoph Kerschbaumer [:ckerschb] 2016-09-22 15:16:56 PDT
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 User image Christoph Kerschbaumer [:ckerschb] 2016-09-22 15:18:07 PDT
Created attachment 8793972 [details] [diff] [review]
bug_1299483_strict_dynamic_enforcement_changes.patch
Comment 3 User image Christoph Kerschbaumer [:ckerschb] 2016-09-22 15:18:33 PDT
Created attachment 8793973 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_changes.patch
Comment 4 User image Christoph Kerschbaumer [:ckerschb] 2016-09-22 15:18:55 PDT
Created attachment 8793974 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_tests.patch
Comment 5 User image Christoph Kerschbaumer [:ckerschb] 2016-09-22 15:19:24 PDT
Created attachment 8793976 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_tests.patch
Comment 6 User image Christoph Kerschbaumer [:ckerschb] 2016-09-22 15:19:46 PDT
Created attachment 8793977 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch
Comment 7 User image Frederik Braun [:freddyb] (off until March 6th) 2016-09-23 01:57:17 PDT
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 User image Frederik Braun [:freddyb] (off until March 6th) 2016-09-23 05:23:11 PDT
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 User image Frederik Braun [:freddyb] (off until March 6th) 2016-09-23 05:55:26 PDT
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 User image Christoph Kerschbaumer [:ckerschb] 2016-09-26 07:59:23 PDT
There is also the following testing site: https://poc.miki.it/strict-dynamic-tests/
Comment 11 User image Christoph Kerschbaumer [:ckerschb] 2016-10-07 03:25:06 PDT
Created attachment 8798804 [details] [diff] [review]
bug_1299483_strict_dynamic_enforcement_changes.patch
Comment 12 User image Christoph Kerschbaumer [:ckerschb] 2016-10-07 03:25:45 PDT
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 User image Christoph Kerschbaumer [:ckerschb] 2016-10-07 03:26:06 PDT
Created attachment 8798805 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_changes.patch
Comment 14 User image Christoph Kerschbaumer [:ckerschb] 2016-10-07 03:26:42 PDT
Created attachment 8798806 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_tests.patch
Comment 15 User image Christoph Kerschbaumer [:ckerschb] 2016-10-07 03:28:10 PDT
Created attachment 8798807 [details] [diff] [review]
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 User image Christoph Kerschbaumer [:ckerschb] 2016-10-07 03:28:42 PDT
Created attachment 8798808 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch
Comment 17 User image Christoph Kerschbaumer [:ckerschb] 2016-10-07 03:29:25 PDT
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 User image Frederik Braun [:freddyb] (off until March 6th) 2016-10-10 05:27:56 PDT
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 User image Frederik Braun [:freddyb] (off until March 6th) 2016-10-10 05:31:05 PDT
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 User image Daniel Veditz [:dveditz] 2016-10-23 23:37:29 PDT
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 User image Daniel Veditz [:dveditz] 2016-10-23 23:42:29 PDT
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 User image Daniel Veditz [:dveditz] 2016-10-23 23:58:43 PDT
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 User image Christoph Kerschbaumer [:ckerschb] 2016-10-30 14:43:55 PDT
(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 User image Christoph Kerschbaumer [:ckerschb] 2016-10-30 14:56:15 PDT
Created attachment 8805869 [details] [diff] [review]
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 User image Christoph Kerschbaumer [:ckerschb] 2016-10-30 14:56:59 PDT
Created attachment 8805870 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_test_default_src.patch
Comment 26 User image Daniel Veditz [:dveditz] 2016-11-01 10:03:44 PDT
(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 User image Michele Spagnuolo 2016-11-07 17:10:33 PST
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 User image Daniel Veditz [:dveditz] 2016-11-08 00:00:23 PST
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 User image Daniel Veditz [:dveditz] 2016-11-08 00:01:07 PST
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 User image Christoph Kerschbaumer [:ckerschb] 2016-11-08 05:01:20 PST
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 32 User image Pulsebot 2016-11-08 07:52:39 PST
Pushed by mozilla@christophkerschbaumer.com:
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

Note You need to log in before you can comment on or make changes to this bug.