Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891443002:
Keep subdomains of an isolated origin in the isolated origin's SiteInstance. (Closed)
|
Created:
3 years, 7 months ago by alexmos Modified:
3 years, 5 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, ajwong+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionKeep subdomains of an isolated origin in the isolated origin's SiteInstance.
Previously, if bar.foo.com was an isolated origin, subdomains like
subdomain.bar.foo.com would end up in a different SiteInstance (using
"foo.com" for its site URL) than the isolated origin, which was
confusing/undesirable. There was also confusion with subdomains if an
etld+1 (e.g., isolated.com) was marked as an isolated origin: we would
try to assign a different SiteInstance to foo.isolated.com than
isolated.com, yet the site URL would still resolve to "isolated.com".
This CL changes this behavior to keep subdomains in the isolated
origin's SiteInstance. It also adds conflict resolution, which allows
adding multiple isolated origins with a common domain (e.g., c.b.a.com
and a.com), where the most specific isolated origin is used when
picking the SiteInstance site URL for a particular URL (e.g., b.a.com
would use a.com, but d.c.b.a.com would use c.b.a.com).
For more discussion about this, see the isolated origin design doc:
https://goo.gl/99ynqK
BUG=713444
Review-Url: https://codereview.chromium.org/2891443002
Cr-Commit-Position: refs/heads/master@{#483881}
Committed: https://chromium.googlesource.com/chromium/src/+/4bc26323b196f3b8957e85fc1eb48c08456fc84d
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Most specific origin matching; tests; bug fixes #Patch Set 4 : Test fixes #Patch Set 5 : Various cleanup #Patch Set 6 : More cleanup #
Total comments: 20
Patch Set 7 : Charlie's comments #
Total comments: 3
Patch Set 8 : Rebase #
Total comments: 6
Patch Set 9 : Addressing Nick's comments #
Total comments: 4
Patch Set 10 : Reorder checks in SiteInstance::IsSameWebSite #
Messages
Total messages: 52 (40 generated)
The CQ bit was checked by alexmos@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexmos@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexmos@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexmos@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 alexmos@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 ========== Keep subdomains of an isolated origin in the isolated origin's process. BUG=713444 ========== to ========== Keep subdomains of an isolated origin in the isolated origin's SiteInstance. Previously, if bar.foo.com was an isolated origin, subdomains like subdomain.bar.foo.com would end up in a different SiteInstance (using "foo.com" for its site URL) than the isolated origin, which was confusing/undesirable. There was also confusion with subdomains if an etld+1 (e.g., isolated.com) was marked as an isolated origin: we would try to assign a different SiteInstance to foo.isolated.com than isolated.com, yet the site URL would still resolve to "isolated.com". This CL changes this behavior to keep subdomains in the isolated origin's SiteInstance. It also adds conflict resolution, which allows adding multiple isolated origins with a common domain (e.g., c.b.a.com and a.com), where the most specific isolated origin is used when picking the SiteInstance site URL for a particular URL (e.g., b.a.com would use a.com, but d.c.b.a.com would use c.b.a.com). For more discussion about this, see the isolated origin design doc: https://goo.gl/99ynqK BUG=713444 ==========
Description was changed from ========== Keep subdomains of an isolated origin in the isolated origin's SiteInstance. Previously, if bar.foo.com was an isolated origin, subdomains like subdomain.bar.foo.com would end up in a different SiteInstance (using "foo.com" for its site URL) than the isolated origin, which was confusing/undesirable. There was also confusion with subdomains if an etld+1 (e.g., isolated.com) was marked as an isolated origin: we would try to assign a different SiteInstance to foo.isolated.com than isolated.com, yet the site URL would still resolve to "isolated.com". This CL changes this behavior to keep subdomains in the isolated origin's SiteInstance. It also adds conflict resolution, which allows adding multiple isolated origins with a common domain (e.g., c.b.a.com and a.com), where the most specific isolated origin is used when picking the SiteInstance site URL for a particular URL (e.g., b.a.com would use a.com, but d.c.b.a.com would use c.b.a.com). For more discussion about this, see the isolated origin design doc: https://goo.gl/99ynqK BUG=713444 ========== to ========== Keep subdomains of an isolated origin in the isolated origin's SiteInstance. Previously, if bar.foo.com was an isolated origin, subdomains like subdomain.bar.foo.com would end up in a different SiteInstance (using "foo.com" for its site URL) than the isolated origin, which was confusing/undesirable. There was also confusion with subdomains if an etld+1 (e.g., isolated.com) was marked as an isolated origin: we would try to assign a different SiteInstance to foo.isolated.com than isolated.com, yet the site URL would still resolve to "isolated.com". This CL changes this behavior to keep subdomains in the isolated origin's SiteInstance. It also adds conflict resolution, which allows adding multiple isolated origins with a common domain (e.g., c.b.a.com and a.com), where the most specific isolated origin is used when picking the SiteInstance site URL for a particular URL (e.g., b.a.com would use a.com, but d.c.b.a.com would use c.b.a.com). For more discussion about this, see the isolated origin design doc: https://goo.gl/99ynqK BUG=713444 ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you please take a look when you get a chance? No rush on this - I know you're at bootcamp.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
creis@chromium.org changed reviewers: + nick@chromium.org
Nice! Basically looks good with a few clarifications. Nick, can you give a sanity check to the origin comparisons in ChildProcessSecurityPolicyImpl::TryGetMostSpecificMatchForIsolatedOrigin and IsolatedOriginUtil::DoesOriginMatchIsolatedOrigin, given your DAFSA work? We're basically trying to find the longest matching origin in a list for a given suborigin. https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:197: // origin's site. I.e., if https://isolated.foo.com is added as an isolated nit: s/I.e./For example/ https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:206: // *are* supported, since process placement decisions will be based on the minor nit: Rewrap? https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:226: // it will return false for https://foo.com/ or https://unisolated.foo.com/. Maybe clarify that site URLs are not included here? (I know we have to deal with whether ports are considered or not somewhere-- I think that's in SiteInstanceImpl?) https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:244: // https://baz.bar.foo.isolated.com/ --> https://bar.foo.isolated.com/ Maybe add a negative example? https://example.com --> (unique origin) https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:245: bool TryGetMostSpecificMatchForIsolatedOrigin(const url::Origin& origin, Maybe simplify the name to GetMatchingIsolatedOrigin? https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:248: // Removes a previously added isolated origin. Might want to mention what considerations there are for calling this. Is it safe to do while there are active SiteInstances in that origin, or will we start seeing undefined behavior? (That is, do callers need to ensure that all existing instances are closed or reloaded?) Or should this have ForTesting in the name since it's only used from tests? (I'm not sure if it's safe in general.) https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_util.cc (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... content/browser/isolated_origin_util.cc:24: return origin.GetURL().DomainIs(isolated_origin.host()); Huh, I wouldn't have expected this to work given the name. (Sounds like it just compares against eTLD+1.) I suppose it's really more of a IsSubdomainOf? https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_util.h (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... content/browser/isolated_origin_util.h:19: // host is a subdomain of |isolated_origin|'s host. As above, we may also want to mention that this doesn't consider sites, which don't care about port.
The CQ bit was checked by alexmos@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...
Thanks for reviewing! https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:197: // origin's site. I.e., if https://isolated.foo.com is added as an isolated On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > nit: s/I.e./For example/ Done. https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:206: // *are* supported, since process placement decisions will be based on the On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > minor nit: Rewrap? Oops, done. https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:226: // it will return false for https://foo.com/ or https://unisolated.foo.com/. On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > Maybe clarify that site URLs are not included here? (I know we have to deal > with whether ports are considered or not somewhere-- I think that's in > SiteInstanceImpl?) Done. Yeah, ports would be included in the site URL for an isolated origin in SiteInstanceImpl::GetSiteForURL, when it calls url::Origin::GetURL(), though only if it's a non-default, non-empty port. For default ports, if we mark an etld+1 as an isolated origin, its site URL would actually match a regular site URL for that etld+1. Is your worry here that someone makes url::Origin out of a GetSiteInstance()->GetSiteURL() and passes into here? I can also add a note specifically not to do that. Perhaps it's easier to just drop the port if we want to reuse this to isolate subsets of sites (to keep document.domain working across ports). I actually don't know if there's enough of a benefit to use ports. https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:244: // https://baz.bar.foo.isolated.com/ --> https://bar.foo.isolated.com/ On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > Maybe add a negative example? > > https://example.com --> (unique origin) Good idea, done. https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:245: bool TryGetMostSpecificMatchForIsolatedOrigin(const url::Origin& origin, On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > Maybe simplify the name to GetMatchingIsolatedOrigin? Done. https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:248: // Removes a previously added isolated origin. On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > Might want to mention what considerations there are for calling this. Is it > safe to do while there are active SiteInstances in that origin, or will we start > seeing undefined behavior? (That is, do callers need to ensure that all > existing instances are closed or reloaded?) > > Or should this have ForTesting in the name since it's only used from tests? > (I'm not sure if it's safe in general.) I've changed it to *ForTesting for now, and added a comment hinting at the issues if we ever need to expose this more generally. I think you're right that we'll likely need to ensure there are no active SiteInstances for sanity: if a renderer is origin-locked to "isolated.foo.com" and suddenly GetSiteForURL starts returning "foo.com", it seems that we'll kill it on cookie requests. Another issue might be that despite trying to minimize TOCTTOU, I think the UI thread still checks isolated origins multiple times from different places during transfers, etc. So it might only be safe to allow removals from the UI thread. We can come back to those concerns if we ever need this outside of tests. https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_util.cc (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... content/browser/isolated_origin_util.cc:24: return origin.GetURL().DomainIs(isolated_origin.host()); On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > Huh, I wouldn't have expected this to work given the name. (Sounds like it just > compares against eTLD+1.) I suppose it's really more of a IsSubdomainOf? Yeah, it behaves like IsSubdomainOf; this is more clear from comments on url::DomainIs, which implements it internally. I think the name is really misleading and also caused me to take a while to find this. Initially, I tried reusing IsSubdomainOfHost in Blink's OriginAccessEntry.cpp, where it's an anonymous namespace helper used for document.domain validation, and exposing that more generally from url/, but then noticed GURL::DomainIs does pretty much the same thing. The Blink code should probably also use url::DomainIs now that url/ can be used there. Maybe we should consider renaming these to IsSubdomainOf, though that name does feel vague about allowing exact domain matches, and not necessarily subdomains. https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_util.h (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... content/browser/isolated_origin_util.h:19: // host is a subdomain of |isolated_origin|'s host. On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > As above, we may also want to mention that this doesn't consider sites, which > don't care about port. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. LGTM with one question about whether we need the util file. https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/child_... content/browser/child_process_security_policy_impl.h:226: // it will return false for https://foo.com/ or https://unisolated.foo.com/. On 2017/06/28 18:29:51, alexmos wrote: > On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > > Maybe clarify that site URLs are not included here? (I know we have to deal > > with whether ports are considered or not somewhere-- I think that's in > > SiteInstanceImpl?) > > Done. Yeah, ports would be included in the site URL for an isolated origin in > SiteInstanceImpl::GetSiteForURL, when it calls url::Origin::GetURL(), though > only if it's a non-default, non-empty port. For default ports, if we mark an > etld+1 as an isolated origin, its site URL would actually match a regular site > URL for that etld+1. Is your worry here that someone makes url::Origin out of a > GetSiteInstance()->GetSiteURL() and passes into here? I can also add a note > specifically not to do that. I mainly wanted to point out the difference for others reading the code, since we're careful about excluding ports in some cases. Agreed that the type difference will probably help distinguish the cases as well. > Perhaps it's easier to just drop the port if we want to reuse this to isolate > subsets of sites (to keep document.domain working across ports). I actually > don't know if there's enough of a benefit to use ports. I think we might as well keep the port, so that we can really talk about isolating origins in the usual definition when we're not limited to sites. https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_util.cc (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... content/browser/isolated_origin_util.cc:24: return origin.GetURL().DomainIs(isolated_origin.host()); On 2017/06/28 18:29:51, alexmos wrote: > On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > > Huh, I wouldn't have expected this to work given the name. (Sounds like it > just > > compares against eTLD+1.) I suppose it's really more of a IsSubdomainOf? > > Yeah, it behaves like IsSubdomainOf; this is more clear from comments on > url::DomainIs, which implements it internally. I think the name is really > misleading and also caused me to take a while to find this. Initially, I tried > reusing IsSubdomainOfHost in Blink's OriginAccessEntry.cpp, where it's an > anonymous namespace helper used for document.domain validation, and exposing > that more generally from url/, but then noticed GURL::DomainIs does pretty much > the same thing. The Blink code should probably also use url::DomainIs now that > url/ can be used there. Maybe we should consider renaming these to > IsSubdomainOf, though that name does feel vague about allowing exact domain > matches, and not necessarily subdomains. Yes, there may be some ways to make this clearer. We can revisit independent of this CL. https://codereview.chromium.org/2891443002/diff/120001/content/browser/isolat... File content/browser/isolated_origin_util.h (right): https://codereview.chromium.org/2891443002/diff/120001/content/browser/isolat... content/browser/isolated_origin_util.h:26: static bool DoesOriginMatchIsolatedOrigin(const url::Origin& origin, Looking closer, it seems like there's only one caller to this (ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin). Is there a reason we need a util file for this, or can it just be a helper function inside CPSP? Maybe you see additional uses for it in the future?
https://codereview.chromium.org/2891443002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2891443002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1097: << "Cannot register a unique origin as an isolated origin."; Should this enforce SchemeIsHttpOrHttps? The domain chopping it does assumes that the host is a network host. https://codereview.chromium.org/2891443002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1098: Do we get into any trouble if origin's hostname is an IP literal? Similarly/relatedly, can we get into trouble if if the origin is, say, "http://co.uk"? (in registry_controlled_domains terms, I think this means that GetRegistryLength() returns 0). An interesting case here is whether we'd want to ever support a privately-listed PSL entry (appspot.com, github.io) to "process isolate all my subdomains". Cookies are already not assignable across these because of the PSL inclusion, but GetRegistryLength("appspot.com") will return 0 if you include the private registries. https://codereview.chromium.org/2891443002/diff/140001/content/browser/site_i... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/2891443002/diff/140001/content/browser/site_i... content/browser/site_instance_impl_unittest.cc:995: GURL baz_isolated_foo_url("http://baz.isolated.foo.com"); You might want to toss in a test somewhere that does something with "foo.com." (with the terminal dot). Chrome generally treats foo.com and foo.com. as different domains.
https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_util.cc (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... content/browser/isolated_origin_util.cc:24: return origin.GetURL().DomainIs(isolated_origin.host()); On 2017/06/28 20:56:57, Charlie Reis (slow) wrote: > On 2017/06/28 18:29:51, alexmos wrote: > > On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > > > Huh, I wouldn't have expected this to work given the name. (Sounds like it > > just > > > compares against eTLD+1.) I suppose it's really more of a IsSubdomainOf? > > > > Yeah, it behaves like IsSubdomainOf; this is more clear from comments on > > url::DomainIs, which implements it internally. I think the name is really > > misleading and also caused me to take a while to find this. Initially, I > tried > > reusing IsSubdomainOfHost in Blink's OriginAccessEntry.cpp, where it's an > > anonymous namespace helper used for document.domain validation, and exposing > > that more generally from url/, but then noticed GURL::DomainIs does pretty > much > > the same thing. The Blink code should probably also use url::DomainIs now > that > > url/ can be used there. Maybe we should consider renaming these to > > IsSubdomainOf, though that name does feel vague about allowing exact domain > > matches, and not necessarily subdomains. > > Yes, there may be some ways to make this clearer. We can revisit independent of > this CL. fwiw I did the same double take too. url::Origin has a DomainIs method, just use that?
The CQ bit was checked by alexmos@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...
Thanks for the reviews! Please take another look. Charlie: replied about the util file. Nick: thanks for bringing up those cases! I was thinking we'd be safe since we'd be careful about what we whitelist, but given that --isolate-origins is out there already, I agree it makes sense to go through these now. Responses below. https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_util.cc (right): https://codereview.chromium.org/2891443002/diff/100001/content/browser/isolat... content/browser/isolated_origin_util.cc:24: return origin.GetURL().DomainIs(isolated_origin.host()); On 2017/06/28 21:07:49, ncarter (slow) wrote: > On 2017/06/28 20:56:57, Charlie Reis (slow) wrote: > > On 2017/06/28 18:29:51, alexmos wrote: > > > On 2017/06/28 01:02:18, Charlie Reis (slow) wrote: > > > > Huh, I wouldn't have expected this to work given the name. (Sounds like > it > > > just > > > > compares against eTLD+1.) I suppose it's really more of a IsSubdomainOf? > > > > > > Yeah, it behaves like IsSubdomainOf; this is more clear from comments on > > > url::DomainIs, which implements it internally. I think the name is really > > > misleading and also caused me to take a while to find this. Initially, I > > tried > > > reusing IsSubdomainOfHost in Blink's OriginAccessEntry.cpp, where it's an > > > anonymous namespace helper used for document.domain validation, and exposing > > > that more generally from url/, but then noticed GURL::DomainIs does pretty > > much > > > the same thing. The Blink code should probably also use url::DomainIs now > > that > > > url/ can be used there. Maybe we should consider renaming these to > > > IsSubdomainOf, though that name does feel vague about allowing exact domain > > > matches, and not necessarily subdomains. > > > > Yes, there may be some ways to make this clearer. We can revisit independent > of > > this CL. > > fwiw I did the same double take too. > > url::Origin has a DomainIs method, just use that? Done. Thanks, I didn't notice it was also exposed from url::Origin. https://codereview.chromium.org/2891443002/diff/120001/content/browser/isolat... File content/browser/isolated_origin_util.h (right): https://codereview.chromium.org/2891443002/diff/120001/content/browser/isolat... content/browser/isolated_origin_util.h:26: static bool DoesOriginMatchIsolatedOrigin(const url::Origin& origin, On 2017/06/28 20:56:57, Charlie Reis (slow) wrote: > Looking closer, it seems like there's only one caller to this > (ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin). Is there a reason > we need a util file for this, or can it just be a helper function inside CPSP? > Maybe you see additional uses for it in the future? You're right that CPSP is the only caller (originally I had other call sites that got cleaned up). I was about to move it, but while addressing Nick's comments, I ended up adding another helper here, IsValidIsolatedOrigin, which I also wanted to expose to unit tests, so now we need to decide whether the util file makes sense for both. :) I guess I was hesitant to clutter CPSP with this logic, which felt like it didn't belong there, even as a private helper. I think this also might be needed in other places later on, e.g., to throw away invalid origins in clickthrough isolation. But I also see the point that we could move these out of CPSP at the point when we need to use them more widely, and keep them internal to CPSP until then. WDYT? https://codereview.chromium.org/2891443002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2891443002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1097: << "Cannot register a unique origin as an isolated origin."; On 2017/06/28 20:59:19, ncarter (slow) wrote: > Should this enforce SchemeIsHttpOrHttps? The domain chopping it does assumes > that the host is a network host. That's a good idea. Besides problematic subdomain matching, I also don't know how this would interact with process-per-site behavior if we enforced this on something like a chrome:// URL. To address this and your other corner cases, I moved all validity checks into a new helper and added a unit test for it. https://codereview.chromium.org/2891443002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1098: On 2017/06/28 20:59:19, ncarter (slow) wrote: > Do we get into any trouble if origin's hostname is an IP literal? It probably should be ok to isolate an IP address, but we shouldn't do subdomain matching on it, similarly to Blink's document.domain logic [1]. This was actually broken, and I added a case to handle it in DoesOriginMatchIsolatedOrigin. Thanks for bringing this up! [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor... > Similarly/relatedly, can we get into trouble if if the origin is, say, > "http://co.uk"? (in registry_controlled_domains terms, I think this means that > GetRegistryLength() returns 0). I think we can indeed get into trouble with these, as subdomain matching on http://co.uk would cause us to group http://foo.co.uk and http://bar.co.uk into the same origin/process, which the regular site rules would not normally allow. So we definitely need to require that isolated origins cannot be a TLD. I added a check for this using HostHasRegistryControlledDomain(), which seems to be equivalent to GetRegistryLength() returning 0. However, this also disallows use cases like http://go/ or http://localhost/, which might actually be legitimate usage of --isolate-origins by someone trying to protect their intranet sites, etc. I don't see how to easily make an exception for those, as registry_controlled_domains doesn't have anything to check only if the host is itself a registry, which is what we really want here. Or maybe I missed it? (We could always add something, or maybe check kDafsa directly with LookupStringInFixedSet, but that would be for another CL, and maybe worth doing only when someone actually needs this.) > An interesting case here is whether we'd want to ever support a privately-listed > PSL entry (http://appspot.com, github.io) to "process isolate all my subdomains". > Cookies are already not assignable across these because of the PSL inclusion, > but GetRegistryLength("appspot.com") will return 0 if you include the private > registries. That's an interesting use case. It'll require additional support to treat each subdomain as its own isolated origin, i.e., the opposite of what this CL is doing, and when we add an isolated origin, we might specify a flag to indicate what kind of treatment subdomains require. https://codereview.chromium.org/2891443002/diff/140001/content/browser/site_i... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/2891443002/diff/140001/content/browser/site_i... content/browser/site_instance_impl_unittest.cc:995: GURL baz_isolated_foo_url("http://baz.isolated.foo.com"); On 2017/06/28 20:59:19, ncarter (slow) wrote: > You might want to toss in a test somewhere that does something with "foo.com." > (with the terminal dot). Chrome generally treats http://foo.com and http://foo.com. as > different domains. This is really interesting. I played around with this, and indeed we treat them as different hosts when we make process model decisions (i.e., a subframe "foo.com." on "foo.com" ends up in an OOPIF with --site-per-process) and same-origin policy (the origins are different and include the trailing dot). But I'm not sure if doing the same for isolated origins is the right thing to do. If we add process isolation to "https://accounts.google.com", we should probably make sure that an attacker can't trivially bypass it by going to "https://accounts.google.com." instead. In practice, it won't get the same cookies, as you pointed out, but it might give a way to bypass the user gesture requirement that signin cares about and needs the process isolation for. (Then again, accounts.google.com. redirects to accounts.google.com, but this might come up for another site that doesn't do this.) The current implementation almost already does this correctly because DomainIs ignores the dot in |origin|. I added some coverage for "foo.bar." to be considered part of a "foo.bar" isolated origin to SubdomainOnIsolatedSite. We might want to look into this for regular site URLs as well, where "foo.com." currently ends up in a different SiteInstance from "foo.com". For example, I don't know if process isolation we enforce for default search engines is broken by adding a trailing dot. Another question is what to do when someone adds an isolated origin with a host that has the trailing dot, "foo.bar.", and then asks whether "foo.bar" should be isolated. To be consistent, we might also consider them the same, but there's a corner case where foo.bar is an intranet host and bar is a TLD. In that case, "foo.bar" would normally stand for "foo.bar.corp.internal.com", and someone would use "foo.bar." to override that and go to the external site. So if someone only tried to isolate the external site, they'd specify it with the trailing dot, and it would make sense to treat these as different isolated origins. This sounds complicated though, and there are probably other cases I'm not thinking of, so for now I've just disabled adding isolated origins with a trailing dot until we have a use case for it and can think through it. Let me know if that sounds ok.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks-- one reply below, and I'll let Nick handle the other parts. https://codereview.chromium.org/2891443002/diff/120001/content/browser/isolat... File content/browser/isolated_origin_util.h (right): https://codereview.chromium.org/2891443002/diff/120001/content/browser/isolat... content/browser/isolated_origin_util.h:26: static bool DoesOriginMatchIsolatedOrigin(const url::Origin& origin, On 2017/06/29 21:54:02, alexmos wrote: > On 2017/06/28 20:56:57, Charlie Reis (slow) wrote: > > Looking closer, it seems like there's only one caller to this > > (ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin). Is there a > reason > > we need a util file for this, or can it just be a helper function inside CPSP? > > > Maybe you see additional uses for it in the future? > > You're right that CPSP is the only caller (originally I had other call sites > that got cleaned up). I was about to move it, but while addressing Nick's > comments, I ended up adding another helper here, IsValidIsolatedOrigin, which I > also wanted to expose to unit tests, so now we need to decide whether the util > file makes sense for both. :) > > I guess I was hesitant to clutter CPSP with this logic, which felt like it > didn't belong there, even as a private helper. I think this also might be > needed in other places later on, e.g., to throw away invalid origins in > clickthrough isolation. But I also see the point that we could move these out > of CPSP at the point when we need to use them more widely, and keep them > internal to CPSP until then. WDYT? If it's more than one function and has a chance of being useful elsewhere, it's fine to leave it here. Thanks!
lgtm https://codereview.chromium.org/2891443002/diff/160001/content/browser/isolat... File content/browser/isolated_origin_util.cc (right): https://codereview.chromium.org/2891443002/diff/160001/content/browser/isolat... content/browser/isolated_origin_util.cc:51: net::registry_controlled_domains::HostHasRegistryControlledDomain( This variant will redo the canonicalization internally, but it's the most readable option. If this won't be called often (I suspect not), this is probably fine. If performance matters we can use GetCanonicalHostRegistryLength. https://codereview.chromium.org/2891443002/diff/160001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2891443002/diff/160001/content/browser/site_i... content/browser/site_instance_impl.cc:346: net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); I wonder if we should do the SameDomainOrHost check first here, before the isolated origin match: if (!SameDomainOrHost(...)) return false; That would kind of fix the co.uk case kind of automatically, and provide a faster result in the cross-site case.
The CQ bit was checked by alexmos@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...
https://codereview.chromium.org/2891443002/diff/160001/content/browser/isolat... File content/browser/isolated_origin_util.cc (right): https://codereview.chromium.org/2891443002/diff/160001/content/browser/isolat... content/browser/isolated_origin_util.cc:51: net::registry_controlled_domains::HostHasRegistryControlledDomain( On 2017/06/30 22:00:56, ncarter (slow) wrote: > This variant will redo the canonicalization internally, but it's the most > readable option. If this won't be called often (I suspect not), this is probably > fine. If performance matters we can use GetCanonicalHostRegistryLength. Acknowledged. This is only called when adding isolated origins right now, so perf probably won't matter until we start adding lots of isolated origins. https://codereview.chromium.org/2891443002/diff/160001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2891443002/diff/160001/content/browser/site_i... content/browser/site_instance_impl.cc:346: net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); On 2017/06/30 22:00:56, ncarter (slow) wrote: > I wonder if we should do the SameDomainOrHost check first here, before the > isolated origin match: > > if (!SameDomainOrHost(...)) > return false; > > That would kind of fix the co.uk case kind of automatically, and provide a > faster result in the cross-site case. Done. Good idea, it makes sense to ensure that isolated origin checks can't relax the regular site checks. I'm still inclined to keep the restriction on not being able to add http://co.uk/ as an isolated origin: while this fixes the subdomain matching here, GetSiteForURL will still put http://foo.co.uk in a "http://co.uk" site URL, which might lead to other weird behavior. We could fix GetSiteForURL to still prefer regular site URL when it's more specific than the isolated_origin URL, but that gets into the behavior of process-isolating all sites on a TLD, and I'd rather do that if/when we have a good use for it.
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 alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/2891443002/#ps180001 (title: "Reorder checks in SiteInstance::IsSameWebSite")
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": 180001, "attempt_start_ts": 1498870267236760,
"parent_rev": "0aa2fe9b404eb8a461aa147e394700c08d087f45", "commit_rev":
"4bc26323b196f3b8957e85fc1eb48c08456fc84d"}
Message was sent while issue was closed.
Description was changed from ========== Keep subdomains of an isolated origin in the isolated origin's SiteInstance. Previously, if bar.foo.com was an isolated origin, subdomains like subdomain.bar.foo.com would end up in a different SiteInstance (using "foo.com" for its site URL) than the isolated origin, which was confusing/undesirable. There was also confusion with subdomains if an etld+1 (e.g., isolated.com) was marked as an isolated origin: we would try to assign a different SiteInstance to foo.isolated.com than isolated.com, yet the site URL would still resolve to "isolated.com". This CL changes this behavior to keep subdomains in the isolated origin's SiteInstance. It also adds conflict resolution, which allows adding multiple isolated origins with a common domain (e.g., c.b.a.com and a.com), where the most specific isolated origin is used when picking the SiteInstance site URL for a particular URL (e.g., b.a.com would use a.com, but d.c.b.a.com would use c.b.a.com). For more discussion about this, see the isolated origin design doc: https://goo.gl/99ynqK BUG=713444 ========== to ========== Keep subdomains of an isolated origin in the isolated origin's SiteInstance. Previously, if bar.foo.com was an isolated origin, subdomains like subdomain.bar.foo.com would end up in a different SiteInstance (using "foo.com" for its site URL) than the isolated origin, which was confusing/undesirable. There was also confusion with subdomains if an etld+1 (e.g., isolated.com) was marked as an isolated origin: we would try to assign a different SiteInstance to foo.isolated.com than isolated.com, yet the site URL would still resolve to "isolated.com". This CL changes this behavior to keep subdomains in the isolated origin's SiteInstance. It also adds conflict resolution, which allows adding multiple isolated origins with a common domain (e.g., c.b.a.com and a.com), where the most specific isolated origin is used when picking the SiteInstance site URL for a particular URL (e.g., b.a.com would use a.com, but d.c.b.a.com would use c.b.a.com). For more discussion about this, see the isolated origin design doc: https://goo.gl/99ynqK BUG=713444 Review-Url: https://codereview.chromium.org/2891443002 Cr-Commit-Position: refs/heads/master@{#483881} Committed: https://chromium.googlesource.com/chromium/src/+/4bc26323b196f3b8957e85fc1eb4... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/4bc26323b196f3b8957e85fc1eb4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
