Bug 184031 - CSP: Implement 'strict-dynamic' source expression
Summary: CSP: Implement 'strict-dynamic' source expression
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL: See https://w3c.github.io/webappsec-c...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-26 20:57 PDT by sideshowbarker
Modified: 2021-09-29 13:39 PDT (History)
27 users (show)

See Also:


Attachments
Patch (77.80 KB, patch)
2021-09-21 17:09 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (47.60 KB, patch)
2021-09-21 17:52 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (58.16 KB, patch)
2021-09-22 17:44 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (55.87 KB, patch)
2021-09-23 10:20 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (54.61 KB, patch)
2021-09-27 10:34 PDT, Kate Cheney
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (52.86 KB, patch)
2021-09-27 16:19 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (54.60 KB, patch)
2021-09-28 08:42 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sideshowbarker 2018-03-26 20:57:53 PDT
See https://w3c.github.io/webappsec-csp/#strict-dynamic-usage

The CSP 'strict-dynamic' source expression is a way for CSP policies to (1) specify that if a CSP-trusted script loads other scripts, the UA must propagate its trustedness to any other scripts it loads, while also (2) specifying that the UA must ignore any host-source and scheme-source expressions which might also be provided in the policy — as well as ignoring the "'unsafe-inline'" and "'self' keyword-sources if they are provided in the policy.


Gecko and Blink/Chrome already have 'strict-dynamic' support (not sure if Edge does or not yet).
Comment 1 Radar WebKit Bug Importer 2018-03-27 00:10:23 PDT
<rdar://problem/38900632>
Comment 2 Mike West 2019-09-17 19:54:56 PDT
+wilander, as we discussed this at TPAC.
Comment 3 lwe 2020-02-11 10:01:27 PST
A recent Twitter thread [1] touched upon the value of providing some more evidence of the utility of 'strict-dynamic'. As someone who benefited a lot from having this feature available (it allowed us to roll out secure CSPs across most of Google), I figured I would chime in with some context.

The 'strict-dynamic' keyword enables simpler CSPs for many websites because it allows to propagate trust from already trusted scripts to dynamically created ones. This allows sites with external dependencies or with a large JavaScript codebase to adopt a nonce-based CSP for mitigating XSS without requiring large refactorings [2]. This is especially important as CSPs based on URL allowlists have been proven to be easily bypassable in the general case [3].

Currently 'strict-dynamic' is supported in all major browsers except Safari which forces every application using it to perform user-agent sniffing to detect users on Safari and disable this security protection for these users.

Some other major web applications using 'strict-dynamic' include:
Cloudflare, Atlassian, Square, Uber, Dropbox, Optimizely, Postmates, Bugcrowd, Instapaper, Pinterest, some Microsoft sites (Visual Studio Marketplace, etc.).

Google is setting a 'strict-dynamic' CSP using script nonces on over 80 sensitive domains (e.g. accounts.google.com, mail.google.com, passwords.google.com) which allowed us to mitigate the majority of externally reported XSS vulnerabilities in such applications over the past two years [4]. 

Based on our conversations with folks who implemented  'strict-dynamic' in Chrome and Firefox, the process was fairly easy; there is also a fairly robust suite of WPTs [5] that can help with this. It would be really great to have 'strict-dynamic' available in Safari, especially since upcoming proposals such as Scripting Policy [6] build on top of this model of deploying CSP.


[1] https://twitter.com/johnwilander/status/1226868430860537857
[2] https://speakerdeck.com/lweichselbaum/o-securing-web-apps-with-modern-platform-features?slide=20
[3] https://storage.googleapis.com/pub-tools-public-publication-data/pdf/45542.pdf
[4] https://speakerdeck.com/lweichselbaum/csp-a-successful-mess-between-hardening-and-mitigation?slide=13
[5] https://github.com/web-platform-tests/wpt/tree/master/content-security-policy/script-src
[6] https://mikewest.github.io/csp-next/scripting-policy.html
Comment 4 Binyamin 2020-07-28 05:48:01 PDT
Any ETA when this will land in Safari?
Comment 5 John Wilander 2020-07-28 09:35:25 PDT
(In reply to Binyamin from comment #4)
> Any ETA when this will land in Safari?

Sorry, we don't comment on future releases. However, we do understand the need for this and it is not being ignored or anything similar.
Comment 6 Dominic Couture 2021-03-09 06:38:09 PST
Hello team, we're working on deploying an improved CSP over at GitLab and I wanted to add my voice to the great Comment 3 above by saying that 'strict-dynamic' is allowing us to deploy a simpler, more secure CSP for most of our users and unfortunately the lack of support in Safari leaves those users more exposed than others if an XSS is exploited.

'strict-dynamic' is especially interesting to us as GitLab allows hosting files in repositories by design so path allow-listing in CSP can be a bit complicated, especially on self-hosted installations that don't have the same level of subdomain isolation across their different resources. It also greatly reduces maintenance across environments running the same applications but that are pulling their external scripts from different domains, so the benefits are also on the productivity side and not only for security.
Comment 7 Kate Cheney 2021-09-21 17:09:59 PDT
Created attachment 438890 [details]
Patch
Comment 8 Kate Cheney 2021-09-21 17:10:12 PDT
Partial implementation, hoping to simplify. Let’s see what EWS thinks.
Comment 9 Kate Cheney 2021-09-21 17:52:57 PDT
Created attachment 438897 [details]
Patch
Comment 10 Brent Fulgham 2021-09-22 11:31:19 PDT
Comment on attachment 438897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438897&action=review

> Source/WebCore/dom/Document.cpp:6728
> +    queueTaskToDispatchEventOnWindow(TaskSource::DOMManipulation, SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes));

Is it appropriate to dispatch the event on window in all cases? Or do we need to limit this to some set of events?

> Source/WebCore/dom/ScriptElement.cpp:303
> +        const auto& contentSecurityPolicy = *m_element.document().contentSecurityPolicy();

Is the contentSecurityPolicy guaranteed to be present in all cases? I think there are some where it is not.

> Source/WebCore/dom/ScriptElement.cpp:379
> +    if (!contentSecurityPolicy.allowInlineScript(m_element.document().url().string(), m_startLineNumber, sourceCode.source(), m_parserInserted ? ParserInserted::Yes : ParserInserted::No, hasKnownNonce))

It seems like it would be better if m_parserInserted was actually a ParserInserted type, then we wouldn't need to make these conversions everywhere.

> LayoutTests/imported/w3c/web-platform-tests/content-security-policy/script-src/script-src-strict_dynamic_discard_source_expressions-expected.txt:4
> +PASS Allowed scripts without a correct nonce are not permitted with `strict-dynamic`.

Yay!
Comment 11 Kate Cheney 2021-09-22 11:37:11 PDT
(In reply to Brent Fulgham from comment #10)
> Comment on attachment 438897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438897&action=review
> 
> > Source/WebCore/dom/Document.cpp:6728
> > +    queueTaskToDispatchEventOnWindow(TaskSource::DOMManipulation, SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes));
> 
> Is it appropriate to dispatch the event on window in all cases? Or do we
> need to limit this to some set of events?
> 

I am not sure. I was hoping EWS would give me insight into what other tests might be affected but it seems like this caused lots of tests to fail.

> > Source/WebCore/dom/ScriptElement.cpp:303
> > +        const auto& contentSecurityPolicy = *m_element.document().contentSecurityPolicy();
> 
> Is the contentSecurityPolicy guaranteed to be present in all cases? I think
> there are some where it is not.

In other places in this file we assume it is, and have  ASSERT(m_element.document().contentSecurityPolicy()); 

But those are at the execute script phase, while these are only in the request script phase. Maybe we don't always have a contentSecurityPolicy. I will look into it.

> 
> > Source/WebCore/dom/ScriptElement.cpp:379
> > +    if (!contentSecurityPolicy.allowInlineScript(m_element.document().url().string(), m_startLineNumber, sourceCode.source(), m_parserInserted ? ParserInserted::Yes : ParserInserted::No, hasKnownNonce))
> 
> It seems like it would be better if m_parserInserted was actually a
> ParserInserted type, then we wouldn't need to make these conversions
> everywhere.
> 

Will change.

> > LayoutTests/imported/w3c/web-platform-tests/content-security-policy/script-src/script-src-strict_dynamic_discard_source_expressions-expected.txt:4
> > +PASS Allowed scripts without a correct nonce are not permitted with `strict-dynamic`.
> 
> Yay!

:) Getting there!
Comment 12 Brent Fulgham 2021-09-22 13:22:39 PDT
> CONSOLE MESSAGE: ReferenceError: Can't find variable: runAnimationTest

The failures all seem like the test case can't load some of the tests infrastructure JS files.
Comment 13 Kate Cheney 2021-09-22 13:45:18 PDT
Comment on attachment 438897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438897&action=review

> Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:61
> +    return !directive || directive->strictDynamicEnabled();

It was this line :P I am returning strict-dynamic enabled even if there is no CSP policy, so all tests without CSP couldn't load scripts.

New patch coming soon.
Comment 14 Kate Cheney 2021-09-22 17:44:31 PDT
Created attachment 438997 [details]
Patch
Comment 15 Brent Fulgham 2021-09-22 19:36:44 PDT
I spot checked a couple of failures: They look  just need rebaselines, though I notice we get multiple reports. Not sure if that is due to sending the event to the window as well as the document, or if this is just the nature of our reporting system.

It’s not the first time we’ve seen multiple copies of a message from CSP, so may be unrelated to your patch.
Comment 16 Kate Cheney 2021-09-23 08:36:09 PDT
(In reply to Brent Fulgham from comment #15)
> I spot checked a couple of failures: They look  just need rebaselines,
> though I notice we get multiple reports. Not sure if that is due to sending
> the event to the window as well as the document, or if this is just the
> nature of our reporting system.
> 
> It’s not the first time we’ve seen multiple copies of a message from CSP, so
> may be unrelated to your patch.

I had to move some CSP checks to check for strict-dynamic because we only have info about parser inserted scripts in certain places, so I think some of these other tests are hitting those early checks and have extra console logging. I think I can fix it by just performing early checks if strict-dynamic appears in the CSP policy. 

It would be ideal if we could have all checks in one place, but that means threading the script information (isParserInserted, script nonce) through a lot of places, which got really messy fast.
Comment 17 Kate Cheney 2021-09-23 10:20:43 PDT
Created attachment 439057 [details]
Patch
Comment 18 Kate Cheney 2021-09-23 10:57:15 PDT
(In reply to Kate Cheney from comment #17)
> Created attachment 439057 [details]
> Patch

This is fun. sending the report to the window instead of the document is making lots of CSP tests not timeout anymore. Maybe I will split that into its own patch with rebaselined tests.
Comment 19 Kate Cheney 2021-09-23 16:13:52 PDT
(In reply to Kate Cheney from comment #18)
> (In reply to Kate Cheney from comment #17)
> > Created attachment 439057 [details]
> > Patch
> 
> This is fun. sending the report to the window instead of the document is
> making lots of CSP tests not timeout anymore. Maybe I will split that into
> its own patch with rebaselined tests.

See https://bugs.webkit.org/show_bug.cgi?id=230728
Comment 20 Kate Cheney 2021-09-27 10:34:03 PDT
Created attachment 439368 [details]
Patch
Comment 21 Kate Cheney 2021-09-27 16:19:28 PDT
Created attachment 439411 [details]
Patch
Comment 22 Kate Cheney 2021-09-28 08:42:30 PDT
Created attachment 439478 [details]
Patch
Comment 23 Brent Fulgham 2021-09-28 11:34:38 PDT
Comment on attachment 439478 [details]
Patch

r=me. though iOS-wk2 might still be grumpy?
Comment 24 Brent Fulgham 2021-09-28 11:35:19 PDT
No - -the iOS-wk2 doesn't seem related.
Comment 25 EWS 2021-09-28 14:00:47 PDT
Committed r283192 (242239@main): <https://commits.webkit.org/242239@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439478 [details].