'strict-dynamic' does not bless dynamically created scripts if multiple policies are present |
|||||||||
Issue descriptionChrome Version : 55.0.2873.0 dev (64-bit) URLs (if applicable) : http://lingro.com:81/cgi-bin/csp-two-policies-with-strict-dynamic-simple.py What steps will reproduce the problem? (1) Set two CSP policies, one of which includes 'strict-dynamic' and one does not: Content-Security-Policy: script-src 'nonce-foobar123' 'strict-dynamic' Content-Security-Policy: script-src www.google.com 'unsafe-inline' (2) Observe that trust propagation with 'strict-dynamic' doesn't trigger for dynamically-created scripts. PoC from http://lingro.com:81/cgi-bin/csp-two-policies-with-strict-dynamic-simple.py: print """\ Content-Type: text/html; charset=utf-8 Content-Security-Policy: script-src 'nonce-foobar123' 'strict-dynamic'; report-uri /enforcing Content-Security-Policy-Report-Only: script-src www.google.com 'unsafe-inline'; report-uri /report-only <html> <body> <script nonce="foobar123"> var s = document.createElement('script'); s.src = "https://www.google.com/jsapi?callback=alert&4-ok-because-strict-dynamic"; document.head.appendChild(s); </script> </body> </html> """ What is the expected result? => Script loads and everyone is happy. What happens instead? => Script doesn't load and everyone is sad.
,
Oct 6 2016
Yup. Should have tested this; it never worked. Ah well. https://codereview.chromium.org/2401573003 is up for review.
,
Oct 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14 commit cbb622f22b8e8c42e509fa7e1229382ceb0b0b14 Author: mkwst <mkwst@chromium.org> Date: Thu Oct 06 16:23:25 2016 CSP: Fix 'strict-dynamic' with multiple policies. The checks we wrote for 'strict-dynamic' fail to allow dynamically- injected scripts if more than one policy is present. This patch addresses that by delegating the dynamic check to 'ContentSecurityPolicy' (rather than bypassing CSP entirely from 'ScriptLoader'). Most of the patch is just piping the "Was this parser-inserted?" bit from 'ScriptLoader::fetchScript' to 'CSPDirectiveList::allowScriptFromSource'. to another. BUG= 653511 Review-Url: https://codereview.chromium.org/2401573003 Cr-Commit-Position: refs/heads/master@{#423557} [add] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/strict-dynamic/script-src-multiple-allowed.php [rename] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/strict-dynamic/script-src-strict-dynamic-whitelist.html [rename] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/strict-dynamic/script-src-strict-dynamic.html [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/dom/ScriptLoader.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/fetch/FetchRequest.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/loader/HttpEquiv.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
,
Oct 7 2016
Hello, lovely release managers. I'd like to merge this change back to 54; it's larger than I'd like, but still quite contained, and should be a low-risk way of ensuring that Google services using `'strict-dynamic'` can be deployed in a timely manner. WDYT?
,
Oct 7 2016
[Automated comment] Less than 2 weeks to go before stable on M54, manual review required.
,
Oct 7 2016
I'd prefer to wait until M55, we're cutting stable in a few days and generally only taking security/crash fixes now. Plus it's large and scary. Feel free to push back, if you have a strong reason for getting this into this milestone.
,
Oct 8 2016
As someone in charge of adopting CSP policies with 'strict-dynamic' at Google, I would really really love to have this merged into M54 -- the bug is blocking us from adopting CSP in several large products which need to use multiple policies (ISE has a couple of folks working full-time on this).
,
Oct 11 2016
What Artur said. Getting this in would accelerate deployment on Google properties; without this fix, they'll need to wait a bit. I agree that the patch is larger than I'd like, but the majority of it is straightforward plumbing: * `ResourceLoaderOptions` (and therefore `FetchRequest`) grew a new enum value. * `ScriptLoader` is the core of the change: it now passes the parser state into CSP rather than making an all-or-nothing determination ahead of time. * The changes in ContentSecurityPolicy are boilerplate: `isAllowedByAllWithContextAndContentAndParser` and `isAllowedByAllWithURLNonceAndParser` are template specializations that allow the new `ParserDisposition` to flow through the various checks. * Likewise, `CSPDirectiveList` simply accepts the new value, and punts on checks if it's set on a policy that also sets the dynamic bit. *shrug* It isn't a terribly complicated bit of work. It's just verbose.
,
Oct 14 2016
Friendly ping -- is it still possible to merge this into M54? (Removed the Merge-rejected label in case this hides it from merge triage; apologies if this isn't the right way to do it.)
,
Oct 20 2016
We already cut the final M54 barring a critical regression, so this is too late. If we end up doing another stable refresh we can include it (note that it requires post-mortem, per the new process). Adding the label "Merge-Request-54" puts it back in our triage queue, sorry I should have stated that in #6. For M55 - this made it in time for branching (cut at revision 423768) so it should be available there.
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14 commit cbb622f22b8e8c42e509fa7e1229382ceb0b0b14 Author: mkwst <mkwst@chromium.org> Date: Thu Oct 06 16:23:25 2016 CSP: Fix 'strict-dynamic' with multiple policies. The checks we wrote for 'strict-dynamic' fail to allow dynamically- injected scripts if more than one policy is present. This patch addresses that by delegating the dynamic check to 'ContentSecurityPolicy' (rather than bypassing CSP entirely from 'ScriptLoader'). Most of the patch is just piping the "Was this parser-inserted?" bit from 'ScriptLoader::fetchScript' to 'CSPDirectiveList::allowScriptFromSource'. to another. BUG= 653511 Review-Url: https://codereview.chromium.org/2401573003 Cr-Commit-Position: refs/heads/master@{#423557} [add] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/strict-dynamic/script-src-multiple-allowed.php [rename] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/strict-dynamic/script-src-strict-dynamic-whitelist.html [rename] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/strict-dynamic/script-src-strict-dynamic.html [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/dom/ScriptLoader.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/fetch/FetchRequest.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/loader/HttpEquiv.cpp [modify] https://crrev.com/cbb622f22b8e8c42e509fa7e1229382ceb0b0b14/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840
,
Feb 23 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by sheriffbot@chromium.org
, Oct 6 2016