Bug 233087 – CSP: Implement protections against nonce-hijacking

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

[Patrick Griffis]

CSP: Implement protections against nonce-hijacking

[Patrick Griffis]

Created attachment 444141 [details]
Patch

[Patrick Griffis]

Created attachment 444143 [details]
Patch

[Patrick Griffis]

Comment on attachment 444143 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/content-security-policy/script-src/nonce-enforce-blocked-expected.txt:2
> +FAIL Unnonced scripts generate reports. Type error

Note that this is a different bug in CSP reporting code that I'll be looking into fixing. The previous error was an assert much earlier where unsafe code was ran which no longer does.

[Patrick Griffis]

(In reply to Patrick Griffis from comment #3)
> Note that this is a different bug in CSP reporting code that I'll be looking
> into fixing.

Fixed in bug 233102, I'll update expectations here as PASS after that lands.

[Patrick Griffis]

Created attachment 444244 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85635546>

[Brent Fulgham]

Comment on attachment 444244 [details]
Patch

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

r=me

> Source/WebCore/dom/Element.cpp:334
> +            const auto& name = attribute.localName();

If you did "localName().convertToASCIILowercase()", you could do the following four string comparisons without doing case insensitive compares.

> Source/WebCore/dom/Element.cpp:335
> +            const auto& value = attribute.value();

Ditto for value

[Chris Dumez]

Comment on attachment 444244 [details]
Patch

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

> Source/WebCore/dom/Element.h:769
> +    bool isNonceable() const;

Please move this up. We don't put getters in the middle of data members.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:517
> +    setAttributes(element, attributes, false, m_parserContentPolicy);

false here is really unclear. This should use an enum class, not a bool. e.g.:
enum class HasDuplicateAttribute : bool { No, Yes };

[Patrick Griffis]

Created attachment 446741 [details]
Patch

[Patrick Griffis]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 444244 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444244&action=review
> 
> > Source/WebCore/dom/Element.h:769
> > +    bool isNonceable() const;
> 
> Please move this up. We don't put getters in the middle of data members.
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:517
> > +    setAttributes(element, attributes, false, m_parserContentPolicy);
> 
> false here is really unclear. This should use an enum class, not a bool.
> e.g.:
> enum class HasDuplicateAttribute : bool { No, Yes };

Before committing this just wanted to make sure my changes look good.

[Brent Fulgham]

Comment on attachment 446741 [details]
Patch

r=me

[EWS]

Committed r286860 (245093@main): <https://commits.webkit.org/245093@main>

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