Bug 235873 - CSP: Improve compatibility of source matching
Summary: CSP: Improve compatibility of source matching
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Patrick Griffis
URL:
Keywords: InRadar
: 201591 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-01-30 12:44 PST by Patrick Griffis
Modified: 2022-04-02 10:24 PDT (History)
10 users (show)

See Also:


Attachments
Patch (12.87 KB, patch)
2022-01-30 12:45 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (12.85 KB, patch)
2022-01-30 13:04 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (14.40 KB, patch)
2022-02-01 13:46 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (12.73 KB, patch)
2022-04-01 11:09 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (12.83 KB, patch)
2022-04-01 11:13 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (13.00 KB, patch)
2022-04-01 12:28 PDT, Patrick Griffis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Griffis 2022-01-30 12:44:41 PST
CSP: Improve compatibility of source matching
Comment 1 Patrick Griffis 2022-01-30 12:45:22 PST Comment hidden (obsolete)
Comment 2 Patrick Griffis 2022-01-30 13:04:03 PST
Created attachment 450366 [details]
Patch
Comment 3 Darin Adler 2022-01-30 13:56:39 PST
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.
Comment 4 Patrick Griffis 2022-02-01 13:46:48 PST
Created attachment 450564 [details]
Patch
Comment 5 Patrick Griffis 2022-02-01 13:49:28 PST
(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.
Comment 6 Darin Adler 2022-02-01 14:02:50 PST
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.
Comment 7 Darin Adler 2022-02-01 14:05:54 PST
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.
Comment 8 Patrick Griffis 2022-02-01 16:52:14 PST
(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.
Comment 9 Darin Adler 2022-02-01 16:56:45 PST
Well the data member is just a bool, not a HasWildcard, so it’t not completely consistent.
Comment 10 Patrick Griffis 2022-02-01 17:46:25 PST
Alright, I'll change it back.

Also found bug 235992 while in the area.
Comment 11 Kate Cheney 2022-02-02 09:09:11 PST
rdar://83788668
Comment 12 Brent Fulgham 2022-03-04 16:59:19 PST
(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?
Comment 13 Patrick Griffis 2022-03-05 07:42:37 PST
(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.
Comment 14 Brent Fulgham 2022-03-15 13:37:53 PDT
*** Bug 201591 has been marked as a duplicate of this bug. ***
Comment 15 Patrick Griffis 2022-04-01 11:09:54 PDT Comment hidden (obsolete)
Comment 16 Patrick Griffis 2022-04-01 11:13:09 PDT
Created attachment 456376 [details]
Patch
Comment 17 Kate Cheney 2022-04-01 11:35:42 PDT
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.
Comment 18 Patrick Griffis 2022-04-01 12:28:40 PDT
Created attachment 456390 [details]
Patch
Comment 19 Patrick Griffis 2022-04-01 12:29:44 PDT
(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.
Comment 20 Kate Cheney 2022-04-01 12:35:27 PDT
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.
Comment 21 EWS 2022-04-02 09:23:29 PDT
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].