Skip to content

Commit

Permalink
Allow trailing dot in host while checking valid origins for opt-in/out.
Browse files Browse the repository at this point in the history 
Prior to this CL, any origin with a trailing dot in the host name would
fail the origin-validity check in ChildProcessSecurityPolicyImpl::
AddOriginIsolationStateForBrowsingInstance() during attempts to
opt-in or out of origin isolation.

This CL corrects this, and adds a test.

(cherry picked from commit d8d31f0)

Bug: 1446661
Change-Id: Iba17968adcb7d3c3af24b605f662318f5c5dc25b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4544436
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1149853}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4569065
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#112}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
W. James MacLean authored and Chromium LUCI CQ committed May 29, 2023
1 parent bc23735 commit a072ee0
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 10 deletions.
118 changes: 118 additions & 0 deletions content/browser/isolated_origin_browsertest.cc
View file
Expand Up @@ -621,6 +621,96 @@ IN_PROC_BROWSER_TEST_F(OriginIsolationOptInHeaderCommandLineTest,
browser_context, url::Origin::Create(non_isolated_sub_origin)));
}

// A test to verify that an origin with a trailing dot in the domain name
// doesn't crash when it opts-out of origin isolation when
// kOriginAgentClusterDefaultEnabled is enabled.
IN_PROC_BROWSER_TEST_F(OriginIsolationOptInHeaderTest,
TrailingDotDomainOptOutDoesNotCrash) {
GURL dotted_nonisolated_url(
https_server()->GetURL("a.com.", "/isolate_origin"));

// Set header to opt this domain out of default OriginAgentCluster.
SetHeaderValue("?0");
EXPECT_TRUE(NavigateToURL(shell(), dotted_nonisolated_url));
url::Origin origin(url::Origin::Create(dotted_nonisolated_url));
EXPECT_FALSE(ShouldOriginGetOptInProcessIsolation(origin));
}

// A test to confirm that "a.com." is treated as a separate host (and hence
// a separate origin) from "a.com". See example at
// https://url.spec.whatwg.org/#concept-domain.
IN_PROC_BROWSER_TEST_F(OriginIsolationOptInHeaderTest,
TrailingDotDomainIsolatesSeparately1) {
GURL main_frame_url(https_server()->GetURL(
"foo.com", "/cross_site_iframe_factory.html?foo.com(foo.com,foo.com)"));
GURL isolated_url(https_server()->GetURL("a.com", "/isolate_origin"));
GURL dotted_isolated_url(https_server()->GetURL("a.com.", "/isolate_origin"));
SetHeaderValue("?1");

// Create page with sibling iframes.
EXPECT_TRUE(NavigateToURL(shell(), main_frame_url));
FrameTreeNode* root = web_contents()->GetPrimaryFrameTree().root();
EXPECT_EQ(2u, root->child_count());
FrameTreeNode* child0_frame_node = root->child_at(0);
FrameTreeNode* child1_frame_node = root->child_at(1);
EXPECT_TRUE(NavigateToURLFromRenderer(child0_frame_node, isolated_url));
EXPECT_TRUE(
NavigateToURLFromRenderer(child1_frame_node, dotted_isolated_url));

url::Origin child0_origin(url::Origin::Create(isolated_url));
url::Origin child1_origin(url::Origin::Create(dotted_isolated_url));
EXPECT_NE(isolated_url, dotted_isolated_url);
EXPECT_NE(child0_origin, child1_origin);

EXPECT_TRUE(ShouldOriginGetOptInProcessIsolation(child0_origin));
EXPECT_TRUE(ShouldOriginGetOptInProcessIsolation(child1_origin));

scoped_refptr<SiteInstanceImpl> child0_site_instance =
child0_frame_node->current_frame_host()->GetSiteInstance();
scoped_refptr<SiteInstanceImpl> child1_site_instance =
child1_frame_node->current_frame_host()->GetSiteInstance();
EXPECT_NE(child0_site_instance, child1_site_instance);
EXPECT_NE(child0_site_instance->GetProcess(),
child1_site_instance->GetProcess());
}

// A test similar to TrailingDotDomainIsolatesSeparately1, but this time the
// "a.com" domain does not opt-in via a header, and does not get an origin-
// keyed process. Thus, it ends up in a separate process from "a.com.".
IN_PROC_BROWSER_TEST_F(OriginIsolationOptInHeaderTest,
TrailingDotDomainIsolatesSeparately2) {
GURL main_frame_url(https_server()->GetURL(
"foo.com", "/cross_site_iframe_factory.html?foo.com(foo.com,foo.com)"));
GURL non_isolated_url(https_server()->GetURL("a.com", "/title1.html"));
GURL dotted_isolated_url(https_server()->GetURL("a.com.", "/isolate_origin"));

// Create page with sibling iframes.
EXPECT_TRUE(NavigateToURL(shell(), main_frame_url));
FrameTreeNode* root = web_contents()->GetPrimaryFrameTree().root();
EXPECT_EQ(2u, root->child_count());
FrameTreeNode* child0_frame_node = root->child_at(0);
FrameTreeNode* child1_frame_node = root->child_at(1);
SetHeaderValue("?1");
EXPECT_TRUE(
NavigateToURLFromRenderer(child0_frame_node, dotted_isolated_url));
SetHeaderValue("");
EXPECT_TRUE(NavigateToURLFromRenderer(child1_frame_node, non_isolated_url));

url::Origin child0_origin(url::Origin::Create(dotted_isolated_url));
url::Origin child1_origin(url::Origin::Create(non_isolated_url));

EXPECT_TRUE(ShouldOriginGetOptInProcessIsolation(child0_origin));
EXPECT_FALSE(ShouldOriginGetOptInProcessIsolation(child1_origin));

scoped_refptr<SiteInstanceImpl> child0_site_instance =
child0_frame_node->current_frame_host()->GetSiteInstance();
scoped_refptr<SiteInstanceImpl> child1_site_instance =
child1_frame_node->current_frame_host()->GetSiteInstance();
EXPECT_NE(child0_site_instance, child1_site_instance);
EXPECT_NE(child0_site_instance->GetProcess(),
child1_site_instance->GetProcess());
}

// A test to confirm that if an Origin-Agent-Cluster header is encountered (but
// not committed) as part of a redirect, that it does not opt-in to
// OriginAgentCluster isolation. The setup in this test is subtle, since in
Expand Down Expand Up @@ -704,6 +794,34 @@ IN_PROC_BROWSER_TEST_F(OriginIsolationOptInHeaderTest, Basic) {
1)));
}

// A test to ensure that origins whose host has a trailing dot pass the
// validation checks for explicit opt-ins and opt-outs. This is an
// `OriginKeyedProcessByDefaultTest` test in order that the explicit opt-out
// will be tracked. Note: failure for either part of this test will involve
// crashing on a CHECK in
// ChildProcessSecurityPolicyImpl::AddOriginIsolationStateForBrowsingInstance():
IN_PROC_BROWSER_TEST_F(OriginKeyedProcessByDefaultTest,
HostWithTrailingDotAllowed) {
// Explicit opt-in with a trailing dot.
SetHeaderValue("?1");
GURL opt_in_url(https_server()->GetURL("opt-in.foo.com.", "/isolate_origin"));
url::Origin opt_in_origin(url::Origin::Create(opt_in_url));

EXPECT_FALSE(ShouldOriginGetOptInProcessIsolation(opt_in_origin));
EXPECT_TRUE(NavigateToURL(shell(), opt_in_url));
EXPECT_TRUE(ShouldOriginGetOptInProcessIsolation(opt_in_origin));

// Explicit opt-out with a trailing dot.
SetHeaderValue("?0");
GURL opt_out_url(
https_server()->GetURL("opt-out.foo.com.", "/isolate_origin"));
url::Origin opt_out_origin(url::Origin::Create(opt_out_url));

EXPECT_FALSE(ShouldOriginGetOptInProcessIsolation(opt_out_origin));
EXPECT_TRUE(NavigateToURL(shell(), opt_out_url));
EXPECT_FALSE(ShouldOriginGetOptInProcessIsolation(opt_out_origin));
}

// A simple test that, when OAC-by-default is enabled with process-isolation, an
// origin that receives default OAC is put in an origin-keyed process.
IN_PROC_BROWSER_TEST_F(OriginKeyedProcessByDefaultTest,
Expand Down
23 changes: 15 additions & 8 deletions content/browser/isolated_origin_util.cc
View file
Expand Up @@ -109,15 +109,17 @@ bool IsolatedOriginUtil::DoesOriginMatchIsolatedOrigin(

// static
bool IsolatedOriginUtil::IsValidIsolatedOrigin(const url::Origin& origin) {
return IsValidIsolatedOriginImpl(origin, true);
return IsValidIsolatedOriginImpl(origin,
/* is_legacy_isolated_origin_check=*/true);
}

// static
bool IsolatedOriginUtil::IsValidOriginForOptInIsolation(
const url::Origin& origin) {
// Per https://html.spec.whatwg.org/C/#initialise-the-document-object,
// non-secure contexts cannot be isolated via opt-in origin isolation.
return IsValidIsolatedOriginImpl(origin, false) &&
return IsValidIsolatedOriginImpl(
origin, /* is_legacy_isolated_origin_check=*/false) &&
network::IsOriginPotentiallyTrustworthy(origin);
}

Expand All @@ -127,13 +129,14 @@ bool IsolatedOriginUtil::IsValidOriginForOptOutIsolation(
// Per https://html.spec.whatwg.org/C/#initialise-the-document-object,
// non-secure contexts cannot be isolated via opt-in origin isolation,
// but we allow non-secure contexts to opt-out for legacy sites.
return IsValidIsolatedOriginImpl(origin, false);
return IsValidIsolatedOriginImpl(origin,
/* is_legacy_isolated_origin_check=*/false);
}

// static
bool IsolatedOriginUtil::IsValidIsolatedOriginImpl(
const url::Origin& origin,
bool check_has_registry_domain) {
bool is_legacy_isolated_origin_check) {
if (origin.opaque())
return false;

Expand All @@ -154,7 +157,7 @@ bool IsolatedOriginUtil::IsValidIsolatedOriginImpl(
// This is not relevant for opt-in origin isolation, which doesn't need to
// match subdomains. (And it'd be bad to check this in that case, as it
// prohibits http://localhost/; see https://crbug.com/1142894.)
if (check_has_registry_domain) {
if (is_legacy_isolated_origin_check) {
const bool has_registry_domain =
net::registry_controlled_domains::HostHasRegistryControlledDomain(
origin.host(),
Expand All @@ -164,11 +167,15 @@ bool IsolatedOriginUtil::IsValidIsolatedOriginImpl(
return false;
}

// For now, disallow hosts with a trailing dot.
// TODO(alexmos): Enabling this would require carefully thinking about
// Disallow hosts with a trailing dot for legacy isolated origins, but allow
// them for opt-in origin isolation since the spec says that they represent
// a distinct origin: https://url.spec.whatwg.org/#concept-domain.
// TODO(alexmos): Legacy isolated origins should probably support trailing
// dots as well, but enabling this would require carefully thinking about
// whether hosts without a trailing dot should match it.
if (origin.host().back() == '.')
if (is_legacy_isolated_origin_check && origin.host().back() == '.') {
return false;
}

return true;
}
Expand Down
7 changes: 5 additions & 2 deletions content/browser/isolated_origin_util.h
View file
Expand Up @@ -108,9 +108,12 @@ class CONTENT_EXPORT IsolatedOriginUtil {

private:
// Used to implement both IsValidIsolatedOrigin and
// IsValidOriginForOptInIsolation.
// IsValidOriginForOptInIsolation. The legacy isolated origin case performs
// some additional checks that don't apply to the opt-in case: it verifies the
// origin has a registry domain (for subdomain matching) and disallows
// trailing dots in the domain.
static bool IsValidIsolatedOriginImpl(const url::Origin& origin,
bool check_has_registry_domain);
bool is_legacy_isolated_origin_check);
};

} // namespace content
Expand Down

0 comments on commit a072ee0

Please sign in to comment.