CSP: Port upgrade matching should only apply to insecure source schemes#62322
CSP: Port upgrade matching should only apply to insecure source schemes#62322webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Conversation
|
EWS run on previous version of this PR (hash 5632161) Details
|
5632161 to
e8c0bac
Compare
|
EWS run on previous version of this PR (hash e8c0bac) Details
|
e8c0bac to
96a61c8
Compare
|
EWS run on previous version of this PR (hash 96a61c8) Details
|
96a61c8 to
c336b4e
Compare
|
EWS run on previous version of this PR (hash c336b4e) Details
|
c336b4e to
22e46ff
Compare
|
EWS run on previous version of this PR (hash 22e46ff) Details
|
22e46ff to
4842d1d
Compare
|
The original reporter identified two cases where CSP source expressions with cross-scheme default ports were unexpectedly allowing resource loads:
After investigation, only case 1 is a bug. The source expression already specifies https, so there is no insecure-to-secure scheme upgrade involved. Since both the source expression and resource urls have an https scheme, port 80 should simply not match port 443. This has been fixed and the iframe is now correctly blocked. Case 2 is correct behavior per spec. The source expression http scheme upgrades to https (§6.7.2.9 step 1.2), and port 443 matches the resource url's default port (443 for https) (§6.7.2.11 step 5). No change needed. |
4842d1d to
2e16ac0
Compare
|
EWS run on previous version of this PR (hash 2e16ac0) Details
|
|
EWS run on previous version of this PR (hash 4842d1d) Details
|
brentfulgham
left a comment
There was a problem hiding this comment.
This is a great change. Thank you for tackling it. I had a few minor nits about variable names and the source of the test cases, but otherwise looks great!
| } | ||
|
|
||
| bool ContentSecurityPolicySource::portMatches(const URL& url) const | ||
| bool ContentSecurityPolicySource::portMatches(const URL& url, bool isSchemeUpgrade) const |
There was a problem hiding this comment.
I'm confused by the name isSchemeUpgrade. Is this a directive that we should upgrade any relevant scheme before comparing? Or does it mean the url being passed in has already BEEN upgraded? Or something else?
There was a problem hiding this comment.
Also, we try to avoid bool arguments in favor of class enums.
| ["yes", "script-src 127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"], | ||
|
|
||
| // Tests that HTTPS URL does not match WSS source expression with HTTP default port (secure scheme, no port upgrade). | ||
| ["no", "script-src wss://127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"], |
There was a problem hiding this comment.
Is this block of new tests something we're merging from upstream WPT? Or did you design these tests yourself?
There was a problem hiding this comment.
Custom tests. This bug involves default-port upgrade logic (port 80 vs 443), which requires internals.registerDefaultPortForProtocol() to test for which AFAICT WPT doesn't have an equivalent. The closest WPT test (embedded-enforcement/subsumption_algorithm-host_sources-ports.html) cover the subsumption algorithm, not source matching.
annevk
left a comment
There was a problem hiding this comment.
Also concerned about these not being WPTs.
| auto& scheme = m_scheme.isEmpty() ? m_policy->selfProtocol() : m_scheme; | ||
| auto urlScheme = url.protocol(); | ||
| // A is the source expression's scheme-part (m_scheme, or selfProtocol if empty). | ||
| // B is the URL's scheme being checked against the policy. |
There was a problem hiding this comment.
I don’t think these comments add much.
| } | ||
|
|
||
| bool ContentSecurityPolicySource::portMatches(const URL& url) const | ||
| bool ContentSecurityPolicySource::portMatches(const URL& url, bool isSchemeUpgrade) const |
There was a problem hiding this comment.
Also, we try to avoid bool arguments in favor of class enums.
| // A is the source expression's scheme-part (m_scheme, or selfProtocol if empty). | ||
| // B is the URL's scheme being checked against the policy. | ||
| const auto& scheme = m_scheme.isEmpty() ? m_policy->selfProtocol() : m_scheme; | ||
| const auto urlScheme = url.protocol(); |
There was a problem hiding this comment.
I think our typical style is to not use const for local variables
There was a problem hiding this comment.
Also concerned about these not being WPTs.
Testing this bug requires overriding default port mappings via internals.registerDefaultPortForProtocol(), which AFAICT WPT can't do. Maybe there are WPT test servers that run on ports 80/443? Otherwise I'm not sure about feasibility for this to be WPT.
2e16ac0 to
9692000
Compare
|
EWS run on current version of this PR (hash 9692000) Details
|
https://bugs.webkit.org/show_bug.cgi?id=308747 rdar://171265255 Reviewed by Brent Fulgham. A CSP source expression such as "frame-src https://host:80" incorrectly matches https://host (port 443). The mismatch (80 ≠ 443) should cause a block, but WebKit has scheme upgrade logic that treats default ports as equivalent during http-to-https transitions. This logic does not verify the source scheme is insecure, so it treats any source with port 80 as upgradable — even when the source scheme is already HTTPS. Have schemeMatches() return a SchemeMatchResult enum that distinguishes exact matches from insecure-to-secure upgrades, and pass this to portMatches() so the upgrade path only fires when a scheme upgrade is actually occurring. This also fixes an edge case with schemeless source expressions. Both functions are annotated with CSP3 spec step references. * LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number-expected.txt: * LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html: * LayoutTests/platform/wk2/TestExpectations: * Source/WebCore/page/csp/ContentSecurityPolicySource.cpp: (WebCore::ContentSecurityPolicySource::portMatches const): Canonical link: https://commits.webkit.org/311148@main
9692000 to
c82619c
Compare
|
Committed 311148@main (c82619c): https://commits.webkit.org/311148@main Reviewed commits have been landed. Closing PR #62322 and removing active labels. |
🛠 mac-apple
c82619c
9692000
🛠 win🧪 win-tests