Bug 231313 – CSP: Implement unsafe-hashes

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

CSP: Implement unsafe-hashes

[Kate Cheney]

rdar://83724376

[Kate Cheney]

Created attachment 440407 [details]
Patch

[Kate Cheney]

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

[Kate Cheney]

Created attachment 440410 [details]
Patch

[Kate Cheney]

Created attachment 440550 [details]
Patch

[Kate Cheney]

Created attachment 440604 [details]
Patch

[Brent Fulgham]

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!

[Chris Dumez]

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

[Kate Cheney]

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

[Kate Cheney]

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

[Kate Cheney]

Created attachment 441013 [details]
Patch for landing

[Kate Cheney]

Created attachment 441014 [details]
Patch for landing

[EWS]

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

[Sam Sneddon [:gsnedders]]

*** Bug 205885 has been marked as a duplicate of this bug. ***