Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
481643002:
Disallow non-subdomain wildcards in the extension's CSP (Closed)
DescriptionDisallow non-subdomain wildcards such as https:// and https://*.com wildcard
patterns in the extension's Content Security policy and update the documentation
to clarify the constraints of the CSP.
BUG=404295
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290699
Patch Set 1 #
Total comments: 4
Patch Set 2 : .length() > 8 replaced with != "https://" and add comment #Patch Set 3 : Disallow RCD and non-subdomain wildcards #
Total comments: 13
Patch Set 4 : check for chrome:// and chrome-extension:// wildcards, update documentation #
Total comments: 2
Patch Set 5 : www.w3.org/TR/CSP11 -> www.w3.org/TR/CSP2 #
Total comments: 3
Patch Set 6 : combine url parsing logic #
Total comments: 3
Patch Set 7 : add code comments #Patch Set 8 : . -> , #
Messages
Total messages: 32 (0 generated)
Based on the existing unit tests, it seems that wildcards CSP directives are undesired, so I have updated the documentation. I've also fixed a bug where https:// is accepted. Should we also disallow TLD wildcard such as https://*.com ?
I'd be more comfortable if mkwst and jww had a look at this. My CSP isn't up to scratch. https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... extensions/common/csp_validator.cc:74: (StartsWithASCII(source, "https://", true) && source.length() > 8) || what is 8?
https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... extensions/common/csp_validator.cc:74: (StartsWithASCII(source, "https://", true) && source.length() > 8) || On 2014/08/18 17:17:18, kalman wrote: > what is 8? The length of "https://".
https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... extensions/common/csp_validator.cc:74: (StartsWithASCII(source, "https://", true) && source.length() > 8) || On 2014/08/18 19:50:05, robwu wrote: > On 2014/08/18 17:17:18, kalman wrote: > > what is 8? > > The length of "https://". I think that (StartsWithASCII(source, "https://") && source != "https://") would be clearer, and accompany with a comment.
https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... extensions/common/csp_validator.cc:74: (StartsWithASCII(source, "https://", true) && source.length() > 8) || On 2014/08/18 19:53:40, kalman wrote: > On 2014/08/18 19:50:05, robwu wrote: > > On 2014/08/18 17:17:18, kalman wrote: > > > what is 8? > > > > The length of "https://". > > I think that (StartsWithASCII(source, "https://") && source != "https://") would > be clearer, and accompany with a comment. Done. See the bug report and the other CL at the linked bug if you need some more context to review the comment.
On 2014/08/18 21:08:18, robwu wrote: > https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... > File extensions/common/csp_validator.cc (right): > > https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... > extensions/common/csp_validator.cc:74: (StartsWithASCII(source, "https://", > true) && source.length() > 8) || > On 2014/08/18 19:53:40, kalman wrote: > > On 2014/08/18 19:50:05, robwu wrote: > > > On 2014/08/18 17:17:18, kalman wrote: > > > > what is 8? > > > > > > The length of "https://". > > > > I think that (StartsWithASCII(source, "https://") && source != "https://") > would > > be clearer, and accompany with a comment. > > Done. See the bug report and the other CL at the linked bug if you need some > more context to review the comment. lgtm I'd be in favor of disallowing *.com and the like, but that gets complicated quickly (should we stop *.in or *.co.in?). Also, that's an extensions team decision, I think.
On 2014/08/18 21:54:35, jww wrote: > On 2014/08/18 21:08:18, robwu wrote: > > > https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... > > File extensions/common/csp_validator.cc (right): > > > > > https://codereview.chromium.org/481643002/diff/1/extensions/common/csp_valida... > > extensions/common/csp_validator.cc:74: (StartsWithASCII(source, "https://", > > true) && source.length() > 8) || > > On 2014/08/18 19:53:40, kalman wrote: > > > On 2014/08/18 19:50:05, robwu wrote: > > > > On 2014/08/18 17:17:18, kalman wrote: > > > > > what is 8? > > > > > > > > The length of "https://". > > > > > > I think that (StartsWithASCII(source, "https://") && source != "https://") > > would > > > be clearer, and accompany with a comment. > > > > Done. See the bug report and the other CL at the linked bug if you need some > > more context to review the comment. > > lgtm > > I'd be in favor of disallowing *.com and the like, but that gets complicated > quickly (should we stop *.in or *.co.in?). Also, that's an extensions team > decision, I think. Oops missed that question. Yes I think we should disable all wildcard TLDs. There's code for that here: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
I've updated the CL to disallow RCD wildcards.
Comments aside I'm stalling a little until mkwst can have a look, I'm not comfortable making CSP changes without his input. So, ping @ mkwst :) https://codereview.chromium.org/481643002/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/contentSecurityPolicy.html (right): https://codereview.chromium.org/481643002/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/contentSecurityPolicy.html:276: not be accepted. Currently, we allow whitelisting origins with the following This paragraph is getting unwieldy, could you split it up between "accepted." and "Currently"? https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... File extensions/common/csp_validator.cc (left): https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:63: } Isn't this a regression insofar as earlier we could disallow wildcard on http/chrome as well, but now we only disallow on https? Could you make isAllowedHttpsUrl agnostic of the scheme (like, find the first occurrence of ":" rather than "https:") then continue to apply that check here? https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:42: static bool isAllowedHttpsUrl(const std::string& url) { This function is inside an anonymous namespace, static unnecessary. Also "isHttpsUrlAllowed" would read more naturally. https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:47: size_t start_of_host = 8; How about: const char* kHttps = "https://"; if (!StartsWithASCII(..)) return false; size_t start_of_host = strlen(kHttps); https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:53: bool isWildcardSubdomain = end_of_host > start_of_host + 2 && is_wildcard_subdomain https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:56: start_of_host += 2; All of this manual URL parsing is making me nervous. Mike any idea if we have wildcard-aware URL parsing library available on the browser? I had a look at GURL but it escapes "*" as "%2A" which is pretty awkward. But maybe less awkward than doing this by hand? Of course if you were to do some sort of un-escaping of "%2A" in the URL it would prevent legitimate uses of "%2A" but I don't even know if that's a valid hostname anyway. https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:57: ^ I didn't notice that most of this code was basically moved from below.
https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:56: start_of_host += 2; (We do have URLPattern but I'm not sure I trust it for parsing CSP directives).
https://codereview.chromium.org/481643002/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/contentSecurityPolicy.html (right): https://codereview.chromium.org/481643002/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/contentSecurityPolicy.html:276: not be accepted. Currently, we allow whitelisting origins with the following On 2014/08/19 15:05:57, kalman wrote: > This paragraph is getting unwieldy, could you split it up between "accepted." > and "Currently"? Done. https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... File extensions/common/csp_validator.cc (left): https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:63: } On 2014/08/19 15:05:57, kalman wrote: > Isn't this a regression insofar as earlier we could disallow wildcard on > http/chrome as well, but now we only disallow on https? Could you make > isAllowedHttpsUrl agnostic of the scheme (like, find the first occurrence of ":" > rather than "https:") then continue to apply that check here? Fixed. http://* has never been allowed by the way. https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:42: static bool isAllowedHttpsUrl(const std::string& url) { On 2014/08/19 15:05:58, kalman wrote: > This function is inside an anonymous namespace, static unnecessary. > > Also "isHttpsUrlAllowed" would read more naturally. Done. https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:47: size_t start_of_host = 8; On 2014/08/19 15:05:58, kalman wrote: > How about: > > const char* kHttps = "https://"; > if (!StartsWithASCII(..)) > return false; > > size_t start_of_host = strlen(kHttps); Done. https://codereview.chromium.org/481643002/diff/40001/extensions/common/csp_va... extensions/common/csp_validator.cc:53: bool isWildcardSubdomain = end_of_host > start_of_host + 2 && On 2014/08/19 15:05:58, kalman wrote: > is_wildcard_subdomain Done. https://codereview.chromium.org/481643002/diff/60001/extensions/common/csp_va... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/60001/extensions/common/csp_va... extensions/common/csp_validator.cc:126: StartsWithASCII(source, "http://localhost:", true) || Drive-by fix. source has already been transformed to lowercase a few lines before, so a case-sensitive comparison is sufficient.
Code change LGTM. I'm not personally sure that there's a ton of value in locking developers out of whitelisting all HTTPS sources for a particular directive, but that's a product decision the extensions team is perfectly qualified to make. :) https://codereview.chromium.org/481643002/diff/60001/extensions/common/csp_va... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/60001/extensions/common/csp_va... extensions/common/csp_validator.cc:56: // host-source and host-part at http://www.w3.org/TR/CSP11/#source-list-syntax Nit: Can you change this link to the editor's draft[1] (or the latest WD[2]). [1]: https://w3c.github.io/webappsec/specs/content-security-policy/ [2]: http://www.w3.org/TR/CSP2/
> I'm not personally sure that there's a ton of value in locking developers out of > whitelisting all HTTPS sources for a particular directive, but that's a product > decision the extensions team is perfectly qualified to make. :) Mike... isn't that code yours?
On 2014/08/19 16:59:47, kalman wrote: > > I'm not personally sure that there's a ton of value in locking developers out > of > > whitelisting all HTTPS sources for a particular directive, but that's a > product > > decision the extensions team is perfectly qualified to make. :) > > Mike... isn't that code yours? Looks like Adam's: https://codereview.chromium.org/8773028 @abarth Why are wildcards disallowed for all directive types?
https://codereview.chromium.org/481643002/diff/80001/extensions/common/csp_va... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/80001/extensions/common/csp_va... extensions/common/csp_validator.cc:30: const char kHttpSchemeAndSeparator[] = "https://"; Https https://codereview.chromium.org/481643002/diff/80001/extensions/common/csp_va... extensions/common/csp_validator.cc:69: It's a shame not to be reusing isNonWildcardScheme in this method. I also don't know why you would be doing the RCD check for https and not http. Perhaps you could combine this and isNonWildcardScheme into a single isNonWildcardTLD which has a bool for checking RCD as well (since there's no point checking RCD for chrome://).
On 2014/08/19 at 17:18:31, rob wrote: > Looks like Adam's: https://codereview.chromium.org/8773028 > @abarth Why are wildcards disallowed for all directive types? This intent was to prevent policies like the following: default-src * That white lists every host on the Internet. Presumably no one trusts every host on the Internet.
https://www.*.example.com ^^^ CSP doesn't support this sort of wildcarding. The * has to be leftmost in the host name.
On 2014/08/19 18:22:44, abarth wrote: > https://www.*.example.com > > ^^^ CSP doesn't support this sort of wildcarding. The * has to be leftmost in > the host name. I should have mentioned that this validation logic serves two purposes in my view: 1. Implementing a restrictive CSP. 2. Providing guidance to developers to avoid common errors. When the validation fails, Chrome refuses tot load the extension and displays an error message with a link to the documentation. For this reason, I check whether the host is not empty and whether it contains a wildcard even though it will be rejected by the CSP anyway. Does this make sense? Should I amend the CL to only perform validation and nothing else?
On 2014/08/19 at 18:35:56, rob wrote: > Does this make sense? Should I amend the CL to only perform validation and nothing else? I have no opinion about what you should do with this CL. I was just providing facts.
IMO what you have is fine.
https://codereview.chromium.org/481643002/diff/80001/extensions/common/csp_va... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/80001/extensions/common/csp_va... extensions/common/csp_validator.cc:69: On 2014/08/19 17:39:22, kalman wrote: > It's a shame not to be reusing isNonWildcardScheme in this method. > > I also don't know why you would be doing the RCD check for https and not http. > > Perhaps you could combine this and isNonWildcardScheme into a single > isNonWildcardTLD which has a bool for checking RCD as well (since there's no > point checking RCD for chrome://). Done. https://codereview.chromium.org/481643002/diff/100001/extensions/common/csp_v... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/100001/extensions/common/csp_v... extensions/common/csp_validator.cc:63: if (start_of_port > start_of_host && url[start_of_port - 1] != ':') { I changed this because :: could be a part of an IPv6 address. The CSP does not specify IPv6 addresses at the moment, but I expect that that is an oversight that will be corrected in the future: https://github.com/w3c/webappsec/issues/49
lgtm https://codereview.chromium.org/481643002/diff/100001/extensions/common/csp_v... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/481643002/diff/100001/extensions/common/csp_v... extensions/common/csp_validator.cc:41: Comment for this method. https://codereview.chromium.org/481643002/diff/100001/extensions/common/csp_v... extensions/common/csp_validator.cc:63: if (start_of_port > start_of_host && url[start_of_port - 1] != ':') { On 2014/08/19 20:04:16, robwu wrote: > I changed this because :: could be a part of an IPv6 address. The CSP does not > specify IPv6 addresses at the moment, but I expect that that is an oversight > that will be corrected in the future: https://github.com/w3c/webappsec/issues/49 Write that in a comment?
Added comments. Thanks for the review.
No problem, thanks for the contribution!
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/481643002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43817) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/48986) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rob@robwu.nl/481643002/140001
Message was sent while issue was closed.
Committed patchset #8 (140001) as 290699 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
