Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708873002:
Stop CSP from matching independent scheme/port upgrades (Closed)
DescriptionStop CSP from matching independent scheme/port upgrades
Content-Security-Policy allows an url to match a source-expression even
if the scheme or the port doesn't matches, but in this case it must be an
upgrade to a more secure scheme(http->https) and more secure port(80->443).
The problem is that it happens independently, so it is allowed to have an
upgrade of the port without the scheme (http over 443) or an upgrade of the
scheme without the port (https over 80).
This is a change to force the upgrade to be both over port and scheme.
BUG=692499, 692442
Review-Url: https://codereview.chromium.org/2708873002
Cr-Commit-Position: refs/heads/master@{#456376}
Committed: https://chromium.googlesource.com/chromium/src/+/d25a9b7ba1e9858fe4d444267524f8ebe8ad8594
Patch Set 1 #
Total comments: 8
Patch Set 2 : Refactoring port/scheme matching logic to have an easier time with auto-upgrading #
Total comments: 8
Patch Set 3 : CR suggestions #Patch Set 4 : rebase-update #
Messages
Total messages: 37 (25 generated)
The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
andypaicu@chromium.org changed reviewers: + mkwst@chromium.org
A few comments inline... https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:122: isSchemeHttp = m_policy->protocolIsEqual("http"); This will return true if the page's scheme is `http`, right? Why is that relevant? https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:128: isPortUpgrade = true; 1) Nit: You need braces if the `if` clause is multi-line. I know there are lots of places in Blink that don't do this, because we used to have very long lines and an automated tool wrapped them, but didn't add braces. C'est la vie. 2) A similar check is in `::portMatches`. Can we remove it? https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1473: bool ContentSecurityPolicy::protocolIsEqual(const String& protocol) const { Is equal to what? It's not clear to me why I'd use this method and not `protocolMatchesSelf`... couldn't you implement one in terms of the other?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Honestly the more I look at it the more I fell like the auto upgrade thing was only added a bit after and in a bit of a rush. I'm thinking about refactoring the thing to maybe make the upgrade logic be more cohesive and in one place instead of scattered about. https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:122: isSchemeHttp = m_policy->protocolIsEqual("http"); On 2017/02/21 at 14:24:04, Mike West (sloooooow) wrote: > This will return true if the page's scheme is `http`, right? Why is that relevant? I believe that a fallback mechanism is necessary if the scheme is not present to have something to rely on. For instance the test at line 234 fails if we don't have this in place. https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:128: isPortUpgrade = true; On 2017/02/21 at 14:24:04, Mike West (sloooooow) wrote: > 1) Nit: You need braces if the `if` clause is multi-line. I know there are lots of places in Blink that don't do this, because we used to have very long lines and an automated tool wrapped them, but didn't add braces. C'est la vie. > > 2) A similar check is in `::portMatches`. Can we remove it? ::portMatches is used in a lot of places and the check is similar but not quite identical. https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1473: bool ContentSecurityPolicy::protocolIsEqual(const String& protocol) const { On 2017/02/21 at 14:24:04, Mike West (sloooooow) wrote: > Is equal to what? It's not clear to me why I'd use this method and not `protocolMatchesSelf`... couldn't you implement one in terms of the other? We specifically don't want this function to match http family protocols if m_selfProtocol is http because we are trying to determine not if it's something in the http family but rather if it's exactly http I will rename it and perhaps refactor protocolMatchesSelf to call this maybe (not sure if really worth it)
On 2017/02/21 at 15:51:44, andypaicu wrote: > Honestly the more I look at it the more I fell like the auto upgrade thing was only added a bit after and in a bit of a rush. I'm thinking about refactoring the thing to maybe make the upgrade logic be more cohesive and in one place instead of scattered about. I'm totally open to a larger refactoring. What do you have in mind? https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:122: isSchemeHttp = m_policy->protocolIsEqual("http"); On 2017/02/21 at 15:51:44, andypaicu wrote: > On 2017/02/21 at 14:24:04, Mike West (sloooooow) wrote: > > This will return true if the page's scheme is `http`, right? Why is that relevant? > > I believe that a fallback mechanism is necessary if the scheme is not present to have something to rely on. > For instance the test at line 234 fails if we don't have this in place. Ok, so in the case that the source doesn't specify a scheme, you're falling back to the page's scheme. That makes sense, thanks. The naming here is weird, then. "protocolIsEqual" doesn't tell me that I'm comparing it to the protocol of the page to which the policy is applying. https://codereview.chromium.org/2708873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:128: isPortUpgrade = true; On 2017/02/21 at 15:51:44, andypaicu wrote: > On 2017/02/21 at 14:24:04, Mike West (sloooooow) wrote: > > 1) Nit: You need braces if the `if` clause is multi-line. I know there are lots of places in Blink that don't do this, because we used to have very long lines and an automated tool wrapped them, but didn't add braces. C'est la vie. > > > > 2) A similar check is in `::portMatches`. Can we remove it? > > ::portMatches is used in a lot of places and the check is similar but not quite identical. Then I think we should figure out how to make it identical. :) The current patch seems confusing.
The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Stop CSP from matching independent scheme/port upgrades Content-Security-Policy allows an url to match a source-expression even if the scheme or the port doesn't matches, but in this case it must be an upgrade to a more secure scheme(http->https) and more secure port(80->443). The problem is that it happens independently, so it is allowed to have an upgrade of the port without the scheme (http over 443) or an upgrade of the scheme without the port (https over 80). This is a change to force the upgrade to be both over port and scheme. BUG=692499 ========== to ========== Stop CSP from matching independent scheme/port upgrades Content-Security-Policy allows an url to match a source-expression even if the scheme or the port doesn't matches, but in this case it must be an upgrade to a more secure scheme(http->https) and more secure port(80->443). The problem is that it happens independently, so it is allowed to have an upgrade of the port without the scheme (http over 443) or an upgrade of the scheme without the port (https over 80). This is a change to force the upgrade to be both over port and scheme. BUG=692499,692442 ==========
https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:167: EXPECT_TRUE(source.matches(KURL(base, "https-so://a.com"))); Added 692442 since this will also fix that bug https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:64: m_policy->protocolEqualsSelf(url.protocol())) We have modified protocolMatchesSelf to be protocolEqualsSelf which does not treat http family protocols specially. Luckily enough we don't need to do anything else here because it already handles http family protocols previously in this check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:26: // matches. NotMatching should always be 0 to let if statements work nicely Nit: Did you consider making this an `enum class` and doing explicit comparisons to `WhateverNotMatching` instead? It looks like there's only one place it matters, and being explicit might be better. https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:32: }; Tiny nit: Newline after the enum. https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:167: EXPECT_TRUE(source.matches(KURL(base, "https-so://a.com"))); On 2017/02/24 at 08:41:00, andypaicu wrote: > Added 692442 since this will also fix that bug Hrm. I'm not actually sure this is a bug. :( This is dumb, but please add something like `// TODO(jochen): Maybe it should return false?` since he's now responsible for suborigins. Muwahaha.
The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
andypaicu@chromium.org changed reviewers: + jochen@chromium.org
@jochen could you have a quick look at the suborigin test and confirm if it should return true or false? CSPSourceTest.cpp:167 https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:26: // matches. NotMatching should always be 0 to let if statements work nicely On 2017/02/24 at 10:56:28, Mike West (Slow.) wrote: > Nit: Did you consider making this an `enum class` and doing explicit comparisons to `WhateverNotMatching` instead? It looks like there's only one place it matters, and being explicit might be better. I have no strong feelings one way or the other https://www.youtube.com/watch?v=ussCHoQttyQ https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:32: }; On 2017/02/24 at 10:56:28, Mike West (Slow.) wrote: > Tiny nit: Newline after the enum. Done. https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2708873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:167: EXPECT_TRUE(source.matches(KURL(base, "https-so://a.com"))); On 2017/02/24 at 10:56:28, Mike West (Slow.) wrote: > On 2017/02/24 at 08:41:00, andypaicu wrote: > > Added 692442 since this will also fix that bug > > Hrm. I'm not actually sure this is a bug. :( > > This is dumb, but please add something like `// TODO(jochen): Maybe it should return false?` since he's now responsible for suborigins. Muwahaha. I've added the comment and let Jochen know.
Still LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by andypaicu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1481
error: third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:
patch does not apply
Patch: third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
Index: third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
b/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
index
26b547d8f99820c8d29097f836d39bae5b471c33..bc64051f92b89a7dfe1b84a767a1a9bd12a54983
100644
--- a/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
@@ -1481,10 +1481,12 @@ bool ContentSecurityPolicy::urlMatchesSelf(const KURL&
url) const {
return m_selfSource->matches(url, RedirectStatus::NoRedirect);
}
-bool ContentSecurityPolicy::protocolMatchesSelf(const KURL& url) const {
- if (equalIgnoringCase("http", m_selfProtocol))
- return url.protocolIsInHTTPFamily();
- return equalIgnoringCase(url.protocol(), m_selfProtocol);
+bool ContentSecurityPolicy::protocolEqualsSelf(const String& protocol) const {
+ return equalIgnoringCase(protocol, m_selfProtocol);
+}
+
+const String& ContentSecurityPolicy::getSelfProtocol() const {
+ return m_selfProtocol;
}
bool ContentSecurityPolicy::selfMatchesInnerURL() const {
The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by andypaicu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2708873002/#ps60001 (title: "rebase-update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489415551530600,
"parent_rev": "178891545bb2f76efeb22802e069de1be23712f5", "commit_rev":
"d25a9b7ba1e9858fe4d444267524f8ebe8ad8594"}
Message was sent while issue was closed.
Description was changed from ========== Stop CSP from matching independent scheme/port upgrades Content-Security-Policy allows an url to match a source-expression even if the scheme or the port doesn't matches, but in this case it must be an upgrade to a more secure scheme(http->https) and more secure port(80->443). The problem is that it happens independently, so it is allowed to have an upgrade of the port without the scheme (http over 443) or an upgrade of the scheme without the port (https over 80). This is a change to force the upgrade to be both over port and scheme. BUG=692499,692442 ========== to ========== Stop CSP from matching independent scheme/port upgrades Content-Security-Policy allows an url to match a source-expression even if the scheme or the port doesn't matches, but in this case it must be an upgrade to a more secure scheme(http->https) and more secure port(80->443). The problem is that it happens independently, so it is allowed to have an upgrade of the port without the scheme (http over 443) or an upgrade of the scheme without the port (https over 80). This is a change to force the upgrade to be both over port and scheme. BUG=692499,692442 Review-Url: https://codereview.chromium.org/2708873002 Cr-Commit-Position: refs/heads/master@{#456376} Committed: https://chromium.googlesource.com/chromium/src/+/d25a9b7ba1e9858fe4d444267524... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d25a9b7ba1e9858fe4d444267524... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
