Bug 184031 – CSP: Implement 'strict-dynamic' source expression

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[sideshowbarker]

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).

[Radar WebKit Bug Importer]

<rdar://problem/38900632>

[Mike West]

+wilander, as we discussed this at TPAC.

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

[Binyamin]

Any ETA when this will land in Safari?

[John Wilander]

(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.

[Dominic Couture]

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.

[Kate Cheney]

Created attachment 438890 [details]
Patch

[Kate Cheney]

Partial implementation, hoping to simplify. Let’s see what EWS thinks.

[Kate Cheney]

Created attachment 438897 [details]
Patch

[Brent Fulgham]

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!

[Kate Cheney]

(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!

[Brent Fulgham]

> 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.

[Kate Cheney]

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.

[Kate Cheney]

Created attachment 438997 [details]
Patch

[Brent Fulgham]

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.

[Kate Cheney]

(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.

[Kate Cheney]

Created attachment 439057 [details]
Patch

[Kate Cheney]

(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.

[Kate Cheney]

(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

[Kate Cheney]

Created attachment 439368 [details]
Patch

[Kate Cheney]

Created attachment 439411 [details]
Patch

[Kate Cheney]

Created attachment 439478 [details]
Patch

[Brent Fulgham]

Comment on attachment 439478 [details]
Patch

r=me. though iOS-wk2 might still be grumpy?

[Brent Fulgham]

No - -the iOS-wk2 doesn't seem related.

[EWS]

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].