The default bug view has changed. See this FAQ.

CSP: Implement 'strict-dynamic'

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla52
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(7 attachments, 5 obsolete attachments)

43.72 KB, patch
ckerschb
: review+
dveditz
: review+
Details | Diff | Splinter Review
11.05 KB, patch
ckerschb
: review+
dveditz
: review+
Details | Diff | Splinter Review
2.73 KB, patch
ckerschb
: review+
dveditz
: review+
Details | Diff | Splinter Review
6.70 KB, patch
freddyb
: review+
ckerschb
: review+
Details | Diff | Splinter Review
8.25 KB, patch
freddyb
: review+
dveditz
: review+
Details | Diff | Splinter Review
4.51 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
7.38 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

7 months ago
Blocks: 1231788
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Keywords: dev-doc-needed
(Assignee)

Comment 1

6 months ago
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/
(Assignee)

Updated

6 months ago
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
(Assignee)

Comment 2

6 months ago
Created attachment 8793972 [details] [diff] [review]
bug_1299483_strict_dynamic_enforcement_changes.patch
Attachment #8793972 - Flags: review?(fbraun)
(Assignee)

Comment 3

6 months ago
Created attachment 8793973 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_changes.patch
Attachment #8793973 - Flags: review?(fbraun)
(Assignee)

Comment 4

6 months ago
Created attachment 8793974 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_tests.patch
Attachment #8793974 - Flags: review?(fbraun)
(Assignee)

Comment 5

6 months ago
Created attachment 8793976 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_tests.patch
Attachment #8793976 - Flags: review?(fbraun)
(Assignee)

Comment 6

6 months ago
Created attachment 8793977 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch
Attachment #8793977 - Flags: review?(fbraun)
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.
Attachment #8793972 - Flags: review?(fbraun) → review+
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
Attachment #8793973 - Flags: review?(fbraun) → review+
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.
Attachment #8793974 - Flags: review?(fbraun) → review+
(Assignee)

Comment 10

6 months ago
There is also the following testing site: https://poc.miki.it/strict-dynamic-tests/
(Assignee)

Comment 11

6 months ago
Created attachment 8798804 [details] [diff] [review]
bug_1299483_strict_dynamic_enforcement_changes.patch
Attachment #8793972 - Attachment is obsolete: true
Attachment #8798804 - Flags: review+
(Assignee)

Comment 12

6 months ago
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.
Attachment #8798804 - Flags: review?(dveditz)
(Assignee)

Comment 13

6 months ago
Created attachment 8798805 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_changes.patch
Attachment #8793973 - Attachment is obsolete: true
Attachment #8798805 - Flags: review+
(Assignee)

Updated

6 months ago
Attachment #8798805 - Flags: review?(dveditz)
(Assignee)

Comment 14

6 months ago
Created attachment 8798806 [details] [diff] [review]
bug_1299483_strict_dynamic_parser_tests.patch
Attachment #8793974 - Attachment is obsolete: true
Attachment #8798806 - Flags: review+
(Assignee)

Updated

6 months ago
Attachment #8798806 - Flags: review+
(Assignee)

Comment 15

6 months ago
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.
Attachment #8793976 - Attachment is obsolete: true
Attachment #8793976 - Flags: review?(fbraun)
Attachment #8798807 - Flags: review?(fbraun)
Attachment #8798807 - Flags: review?(dveditz)
(Assignee)

Comment 16

6 months ago
Created attachment 8798808 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_parser_inserted_tests.patch
Attachment #8793977 - Attachment is obsolete: true
Attachment #8793977 - Flags: review?(fbraun)
Attachment #8798808 - Flags: review?(fbraun)
Attachment #8798808 - Flags: review?(dveditz)
(Assignee)

Comment 17

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

Ups, wrong flag, this patch obviously needs review from Dan.
Attachment #8798806 - Flags: review+ → review?(dveditz)
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.
Attachment #8798808 - Flags: review?(fbraun) → review+
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)
Attachment #8798807 - Flags: review?(fbraun) → review+
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.
Attachment #8798807 - Flags: review?(dveditz) → review+
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
Attachment #8798808 - Flags: review?(dveditz) → review+
Attachment #8798806 - Flags: review?(dveditz) → review+
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.
Attachment #8798807 - Flags: review+ → review-
Attachment #8798805 - Flags: review?(dveditz) → review+
Attachment #8798804 - Flags: review?(dveditz) → review+
(Assignee)

Updated

5 months ago
Depends on: 1313937
(Assignee)

Comment 23

5 months ago
(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.
(Assignee)

Comment 24

5 months ago
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.
Attachment #8805869 - Flags: review?(dveditz)
(Assignee)

Comment 25

5 months ago
Created attachment 8805870 [details] [diff] [review]
bug_1299483_implement_strict_dynamic_test_default_src.patch
Attachment #8805870 - Flags: review?(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

5 months ago
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 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
Attachment #8805869 - Flags: review?(dveditz) → review+
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
Attachment #8805870 - Flags: review?(dveditz) → review+
(Assignee)

Comment 30

5 months ago
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.
Attachment #8798807 - Flags: review- → review+
(Assignee)

Comment 31

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

Comment 32

5 months ago
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

Comment 33

5 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1316826
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.