Bug 235873 – CSP: Improve compatibility of source matching

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Patrick Griffis]

CSP: Improve compatibility of source matching

[Patrick Griffis]

Created attachment 450365 [details]
Patch

[Patrick Griffis]

Created attachment 450366 [details]
Patch

[Darin Adler]

Comment on attachment 450366 [details]
Patch

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

Generally good; room for improvement

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:240
> +    m_selfSource = makeUnique<ContentSecurityPolicySource>(*this, m_selfSourceProtocol, securityOrigin.host(), effectivePort, emptyString(), false, false, true);

This “false, false, true” is not a good pattern: difficult to spot mistakes. This is why we end up using the cumbersome techniques that involve enumerations. Otherwise there is just super subtle code like this.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:173
> +    const String& selfScheme() const { return m_selfSourceProtocol; };

Why mix terminology between “scheme“ and “protocol“?

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:61
> +    const String& scheme = m_scheme.isEmpty() ? m_policy.selfScheme() : m_scheme;

auto&

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:62
> +    String urlScheme = url.protocol().convertToASCIILowercase();

auto

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:120
> +    bool isUpgradeSecure = (port && port.value() == 443) || (!port && WTF::isDefaultPortForProtocol(443, url.protocol()));

“port == 443” should so the same thing as “port && port.value() == 443”

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:121
> +    bool isCurrentUpgradable = (m_port && m_port.value() == 80) || (m_scheme == "http" && (!m_port || m_port.value() == 443));

Same thing here. “m_port == 80”

By the way, could also do m_port.value_or(443) == 443 or just leave out .value().

> Source/WebCore/page/csp/ContentSecurityPolicySource.h:39
> +    ContentSecurityPolicySource(const ContentSecurityPolicy&, const String& scheme, const String& host, std::optional<uint16_t> port, const String& path, bool hostHasWildcard, bool portHasWildcard, bool isSelfSource);

This new argument should be an enumeration, not a bool. All callers are passing constant values, and this is just the situation where WebKit style calls for enumerations so that you can see at the call site what the value means.

> LayoutTests/imported/w3c/web-platform-tests/content-security-policy/connect-src/connect-src-websocket-self.sub-expected.txt:2
> +PASS Expecting logs: ["allowed", "allowed"]

Not enough test coverage. This progression is great, but we need tests covering all the improvements this patch makes.

If not already in WPT, then add to WPT or at least to WebKit tests.

[Patrick Griffis]

Created attachment 450564 [details]
Patch

[Patrick Griffis]

(In reply to Darin Adler from comment #3)

I applied all of your fixes.

> Not enough test coverage. This progression is great, but we need tests
> covering all the improvements this patch makes.
> 
> If not already in WPT, then add to WPT or at least to WebKit tests.

I ended up limiting this patch is scope a bit by removing the default port changes, as WebKit tests asserted otherwise and I honestly think its correct. I'll try to have a discussion with some Chromium folks about this.

In the end the test cases covers http->wss and http->ws which is the majority of this change. I don't see an obvious way to set up WPT to use non-http custom protocols that can upgrade to https.

If that sounds reasonable to you I'll commit it soon.

[Darin Adler]

It’s always risky to make improvements that don’t have regression tests.

In past, when that happened, we expanded the capabilities of WebKit regression tests so we could test the improvements, sometimes using tests written in C++ or Objective-C if we couldn’t do them in LayoutTests-style tests.

But that would not block landing the fix. It’s important follow-through for the health of the product, though. We don’t want a future contributor to unknowingly break this.

[Darin Adler]

Comment on attachment 450564 [details]
Patch

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

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:120
> +    bool isUpgradeSecure = (port == 443) || (!port && WTF::isDefaultPortForProtocol(443, url.protocol()));

I don’t think the WTF:: prefix is needed here.

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.h:64
> -        bool hasWildcard { false };
> +        HasWildcard hasWildcard { HasWildcard::No };

Is this change helpful? I don’t strongly object, after all it’s consistent with what we have to do to make function arguments look good, and is helpful if we are using structure initialization syntax, but I’m not sure how you decided.

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.h:68
> -        bool hasWildcard { false };
> +        HasWildcard hasWildcard { HasWildcard::No };

Ditto.

[Patrick Griffis]

(In reply to Darin Adler from comment #7)
> Is this change helpful? I don’t strongly object, after all it’s consistent
> with what we have to do to make function arguments look good, and is helpful
> if we are using structure initialization syntax, but I’m not sure how you
> decided.

It was only for consistency in the area.

[Darin Adler]

Well the data member is just a bool, not a HasWildcard, so it’t not completely consistent.

[Patrick Griffis]

Alright, I'll change it back.

Also found bug 235992 while in the area.

[Kate Cheney]

rdar://83788668

[Brent Fulgham]

(In reply to Patrick Griffis from comment #10)
> Alright, I'll change it back.
> 
> Also found bug 235992 while in the area.

Did you intend to do more here? I'm not sure if we should review either of these patches, or if the work was done elsewhere?

[Patrick Griffis]

(In reply to Brent Fulgham from comment #12)
> (In reply to Patrick Griffis from comment #10)
> > Alright, I'll change it back.
> > 
> > Also found bug 235992 while in the area.
> 
> Did you intend to do more here? I'm not sure if we should review either of
> these patches, or if the work was done elsewhere?

I was intending for another revision. Just got sick and pulled to other tasks. It's on my list.

[Brent Fulgham]

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

[Patrick Griffis]

Created attachment 456375 [details]
Patch

[Patrick Griffis]

Created attachment 456376 [details]
Patch

[Kate Cheney]

Comment on attachment 456376 [details]
Patch

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

Looks good! I see Darin already reviewed it but I just had one spec question.

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:70
> +        return true;

I'm looking at the spec:

"
More formally, two ASCII strings (A and B) are said to scheme-part match if the following algorithm returns "Matches":

1. If one of the following is true, return "Matches":
    1. A is an ASCII case-insensitive match for B.
    2. A is an ASCII case-insensitive match for "http", and B is an ASCII case-insensitive match for "https".
    3. A is an ASCII case-insensitive match for "ws", and B is an ASCII case-insensitive match for "wss", "http", or "https".
    4. A is an ASCII case-insensitive match for "wss", and B is an ASCII case-insensitive match for "https".

2. Return "Does Not Match".
"

It seems we directly cover (1) with the first check of (scheme == urlScheme). We cover (2) with (scheme == "http" && urlScheme == "https"). And we cover part of (3) with (scheme == "ws" && urlScheme == "wss"). Do we also need a check for scheme == 'ws' && urlScheme == 'http'/'https' and (4)?

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:125
> +    // Also not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match

Tiny nit that already existed before your patch: we usually try to make comments full sentences with periods at the end.

[Patrick Griffis]

Created attachment 456390 [details]
Patch

[Patrick Griffis]

(In reply to Kate Cheney from comment #17)
> It seems we directly cover (1) with the first check of (scheme ==
> urlScheme). We cover (2) with (scheme == "http" && urlScheme == "https").
> And we cover part of (3) with (scheme == "ws" && urlScheme == "wss"). Do we
> also need a check for scheme == 'ws' && urlScheme == 'http'/'https' and (4)?

You're right. Updated to handle this.

> > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:125
> > +    // Also not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match
> 
> Tiny nit that already existed before your patch: we usually try to make
> comments full sentences with periods at the end.

This was a multi-line comment that ended with a period.

[Kate Cheney]

Comment on attachment 456390 [details]
Patch

> > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:125
> > > +    // Also not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match
> >
> > Tiny nit that already existed before your patch: we usually try to make
> > comments full sentences with periods at the end.

> This was a multi-line comment that ended with a period.

Nice! I missed the second line.

[EWS]

Committed r292266 (249164@main): <https://commits.webkit.org/249164@main>

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