Closed Bug 1299483 Opened 8 years ago Closed 8 years ago

CSP: Implement 'strict-dynamic'

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [domsecurity-active])

Attachments

(7 files, 5 obsolete files)

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
freddy
: review+
ckerschb
: review+
Details | Diff | Splinter Review
8.25 KB, patch
freddy
: 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
      No description provided.
Blocks: csp-w3c-3
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
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: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Attachment #8793974 - 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+
There is also the following testing site: https://poc.miki.it/strict-dynamic-tests/
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)
Attachment #8798805 - Flags: review?(dveditz)
Attachment #8793974 - Attachment is obsolete: true
Attachment #8798806 - Flags: review+
Attachment #8798806 - Flags: review+
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)
Attachment #8793977 - Attachment is obsolete: true
Attachment #8793977 - Flags: review?(fbraun)
Attachment #8798808 - Flags: review?(fbraun)
Attachment #8798808 - Flags: review?(dveditz)
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+
Depends on: 1313937
(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.
(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)
(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."
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+
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+
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
Depends on: 1316826
You need to log in before you can comment on or make changes to this bug.