CSP: Implement unsafe-hashes
rdar://83724376
Created attachment 440407 [details] Patch
(In reply to Kate Cheney from comment #2) > Created attachment 440407 [details] > Patch WIP, not ready for review quite yet but wanted to see what EWS thinks. Will update ChangeLogs with more detail in the next patch.
Created attachment 440410 [details] Patch
Created attachment 440550 [details] Patch
Created attachment 440604 [details] Patch
Comment on attachment 440604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440604&action=review r=me > Source/WebCore/ChangeLog:9 > + unsafe-hashes allows specific event handlers and style attributes double-space between 'handlers' and 'and'. > Source/WebCore/dom/StyledElement.cpp:208 > + if (reason == ModifiedByCloning || document().contentSecurityPolicy()->allowInlineStyle(document().url().string(), startLineNumber, newStyleString.string(), CheckUnsafeHashes::Yes, isInUserAgentShadowTree())) Looks like two spaces before "newStyleString.string()". > Source/WebCore/page/csp/ContentSecurityPolicy.h:98 > + bool allowInlineEventHandlers(const String& contextURL, const WTF::OrdinalNumber& contextLine, const String&, bool overrideContentSecurityPolicy = false) const; I think showing 'code' for the new string makes sense here. Otherwise it's not clear what that parameter is for. > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:169 > + ContentSecurityPolicySourceListDirective* operativeDirective = this->operativeDirective(m_scriptSrc.get()); Could this be auto* ? > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:177 > + ContentSecurityPolicySourceListDirective* operativeDirective = this->operativeDirective(m_styleSrc.get()); Ditto > LayoutTests/ChangeLog:15 > + bug in our DOM code. Could you please file a Bugzilla about that one failing test (and send me the #?) > LayoutTests/TestExpectations:-957 > -imported/w3c/web-platform-tests/content-security-policy/unsafe-hashes/style_attribute_allowed.html [ Skip ] Yes! > LayoutTests/imported/w3c/web-platform-tests/content-security-policy/unsafe-hashes/javascript_src_allowed-window_location-expected.txt:2 > +PASS Test that the javascript: src is allowed to run Yes! > LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-classic-expected.txt:10 > +PASS inline event handlers triggered via UA code must not inherit the nonce from the triggering script, thus fail Nice!
Comment on attachment 440604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440604&action=review > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:389 > + auto [foundHashInEnforcedPolicies, foundHashInReportOnlyPolicies] = findHashOfContentInPolicies(WTFMove(searchPolicy), source, hashAlgorithms); It is unclear why this function and findHashOfContentInPolicies() take searchPolicy as an r-value reference instead of simply a const reference. > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:407 > + reportViolation(ContentSecurityPolicyDirectiveNames::scriptSrcElem, violatedDirective, "inline", consoleMessage, contextURL, TextPosition(contextLine, WTF::OrdinalNumber())); Assuming the parameter is a String, "inline"_s would be better (same comment elsewhere).
Comment on attachment 440604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440604&action=review >> Source/WebCore/ChangeLog:9 >> + unsafe-hashes allows specific event handlers and style attributes > > double-space between 'handlers' and 'and'. Oops, will fix. >> Source/WebCore/dom/StyledElement.cpp:208 >> + if (reason == ModifiedByCloning || document().contentSecurityPolicy()->allowInlineStyle(document().url().string(), startLineNumber, newStyleString.string(), CheckUnsafeHashes::Yes, isInUserAgentShadowTree())) > > Looks like two spaces before "newStyleString.string()". Ditto, will fix. >> Source/WebCore/page/csp/ContentSecurityPolicy.h:98 >> + bool allowInlineEventHandlers(const String& contextURL, const WTF::OrdinalNumber& contextLine, const String&, bool overrideContentSecurityPolicy = false) const; > > I think showing 'code' for the new string makes sense here. Otherwise it's not clear what that parameter is for. Fair point, I'll add that back. >> Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:169 >> + ContentSecurityPolicySourceListDirective* operativeDirective = this->operativeDirective(m_scriptSrc.get()); > > Could this be auto* ? Yes, I'll fix it. We should probably change the rest of this file to use auto in another patch. >> LayoutTests/ChangeLog:15 >> + bug in our DOM code. > > Could you please file a Bugzilla about that one failing test (and send me the #?) Yes. Bug: https://bugs.webkit.org/show_bug.cgi?id=231642
(In reply to Chris Dumez from comment #8) > Comment on attachment 440604 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440604&action=review > > > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:389 > > + auto [foundHashInEnforcedPolicies, foundHashInReportOnlyPolicies] = findHashOfContentInPolicies(WTFMove(searchPolicy), source, hashAlgorithms); > > It is unclear why this function and findHashOfContentInPolicies() take > searchPolicy as an r-value reference instead of simply a const reference. > No good reason. Will change in the patch-for-landing. > > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:407 > > + reportViolation(ContentSecurityPolicyDirectiveNames::scriptSrcElem, violatedDirective, "inline", consoleMessage, contextURL, TextPosition(contextLine, WTF::OrdinalNumber())); > > Assuming the parameter is a String, "inline"_s would be better (same comment > elsewhere). Will change.
Created attachment 441013 [details] Patch for landing
Created attachment 441014 [details] Patch for landing
Committed r284067 (242896@main): <https://commits.webkit.org/242896@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441014 [details].
*** Bug 205885 has been marked as a duplicate of this bug. ***