Add precursor origin checks to CanAccessDataForOrigin().

- Updated url::Origin version of CanAccessDataForOrigin() to verify
  precursor information if present and preserves existing behavior
  for opaque origins w/o precursor info. (i.e. only allow if lock is
  not set).
- Created tests for url::Origin version of CanAccessDataForOrigin().
- Added a few new tests cases for GURL version of
  CanAccessDataForOrigin().

Bug: 991607
Change-Id: Ic102e5f9f90a0f2412b5f18d7c48d46c706b818c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1756763
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689543}

content/browser/child_process_security_policy_impl.cc

@@ -128,6 +128,19 @@ base::debug::CrashKeyString* GetRequestedOriginCrashKey() {
   return requested_origin_key;
 }
 
+void LogCanAccessDataForOriginCrashKeys(
+    const std::string& expected_process_lock,
+    const std::string& killed_process_origin_lock,
+    const std::string& requested_origin) {
+  base::debug::SetCrashKeyString(bad_message::GetRequestedSiteURLKey(),
+                                 expected_process_lock);
+  base::debug::SetCrashKeyString(bad_message::GetKilledProcessOriginLockKey(),
+                                 killed_process_origin_lock);
+
+  auto* requested_origin_key = GetRequestedOriginCrashKey();
+  base::debug::SetCrashKeyString(requested_origin_key, requested_origin);
+}
+
 }  // namespace
 
 // The SecurityState class is used to maintain per-child process security state
@@ -1262,10 +1275,43 @@ bool ChildProcessSecurityPolicyImpl::ChildProcessHasPermissionsForFile(
 bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(
     int child_id,
     const url::Origin& origin) {
-  bool success = CanAccessDataForOrigin(child_id, origin.GetURL());
+  GURL url_to_check;
+  if (origin.opaque()) {
+    auto precursor_tuple = origin.GetTupleOrPrecursorTupleIfOpaque();
+    if (precursor_tuple.IsInvalid()) {
+      // We don't have precursor information so we only allow access if
+      // the process lock isn't set yet.
+      base::AutoLock lock(lock_);
+      SecurityState* security_state = GetSecurityState(child_id);
+
+      if (security_state && security_state->origin_lock().is_empty())
+        return true;
+
+      std::string killed_process_origin_lock;
+      if (!security_state) {
+        killed_process_origin_lock = "(child id not found)";
+      } else {
+        killed_process_origin_lock = security_state->origin_lock().spec();
+      }
+      LogCanAccessDataForOriginCrashKeys("(empty)" /* expected_process_lock */,
+                                         killed_process_origin_lock,
+                                         origin.GetDebugString());
+
+      return false;
+    } else {
+      url_to_check = precursor_tuple.GetURL();
+    }
+  } else {
+    url_to_check = origin.GetURL();
+  }
+  bool success = CanAccessDataForOrigin(child_id, url_to_check);
   if (success)
     return true;
 
+  // Note: LogCanAccessDataForOriginCrashKeys() is called in the
+  // CanAccessDataForOrigin() call above. The code below overrides the origin
+  // crash key set in that call with data from |origin| because it provides
+  // more accurate information than the origin derived from |url_to_check|.
   auto* requested_origin_key = GetRequestedOriginCrashKey();
   base::debug::SetCrashKeyString(requested_origin_key, origin.GetDebugString());
   return false;
@@ -1298,9 +1344,6 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(int child_id,
   if (!can_access) {
     // Returning false here will result in a renderer kill.  Set some crash
     // keys that will help understand the circumstances of that kill.
-    base::debug::SetCrashKeyString(bad_message::GetRequestedSiteURLKey(),
-                                   expected_process_lock.spec());
-
     std::string killed_process_origin_lock;
     if (!security_state) {
       killed_process_origin_lock = "(child id not found)";
@@ -1309,12 +1352,9 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(int child_id,
     } else {
       killed_process_origin_lock = security_state->origin_lock().spec();
     }
-    base::debug::SetCrashKeyString(bad_message::GetKilledProcessOriginLockKey(),
-                                   killed_process_origin_lock);
-
-    auto* requested_origin_key = GetRequestedOriginCrashKey();
-    base::debug::SetCrashKeyString(requested_origin_key,
-                                   url.GetOrigin().spec());
+    LogCanAccessDataForOriginCrashKeys(expected_process_lock.spec(),
+                                       killed_process_origin_lock,
+                                       url.GetOrigin().spec());
   }
   return can_access;
 }

content/browser/child_process_security_policy_unittest.cc

@@ -1225,42 +1225,50 @@ TEST_F(ChildProcessSecurityPolicyTest, RemoveRace_CanAccessDataForOrigin) {
   EXPECT_FALSE(io_after_remove_complete);
 }
 
-TEST_F(ChildProcessSecurityPolicyTest, CanAccessDataForOrigin) {
+TEST_F(ChildProcessSecurityPolicyTest, CanAccessDataForOrigin_URL) {
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();
 
   GURL file_url("file:///etc/passwd");
-  GURL http_url("http://foo.com/index.html");
-  GURL http2_url("http://bar.com/index.html");
+  GURL foo_http_url("http://foo.com/index.html");
+  GURL foo_blob_url("blob:http://foo.com/43d75119-d7af-4471-a293-07c6b3d7e61a");
+  GURL foo_filesystem_url("filesystem:http://foo.com/temporary/test.html");
+  GURL bar_http_url("http://bar.com/index.html");
 
   // Test invalid ID case.
   EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, file_url));
-  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, http_url));
-  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, http2_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, foo_http_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, foo_blob_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, foo_filesystem_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, bar_http_url));
 
   TestBrowserContext browser_context;
   p->Add(kRendererID, &browser_context);
 
   // Verify unlocked origin permissions.
   EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, file_url));
-  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, http_url));
-  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, http2_url));
+  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, foo_http_url));
+  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, foo_blob_url));
+  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, foo_filesystem_url));
+  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, bar_http_url));
 
   // Isolate |http_url| so we can't get a default SiteInstance.
-  p->AddIsolatedOrigins({url::Origin::Create(http_url)},
+  p->AddIsolatedOrigins({url::Origin::Create(foo_http_url)},
                         IsolatedOriginSource::TEST, &browser_context);
 
   // Lock process to |http_url| origin.
   scoped_refptr<SiteInstanceImpl> foo_instance =
-      SiteInstanceImpl::CreateForURL(&browser_context, http_url);
+      SiteInstanceImpl::CreateForURL(&browser_context, foo_http_url);
   EXPECT_FALSE(foo_instance->IsDefaultSiteInstance());
   p->LockToOrigin(foo_instance->GetIsolationContext(), kRendererID,
                   foo_instance->GetSiteURL());
 
   // Verify that file access is no longer allowed.
   EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, file_url));
-  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, http_url));
-  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, http2_url));
+  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, foo_http_url));
+  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, foo_blob_url));
+  EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, foo_filesystem_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, bar_http_url));
 
   p->Remove(kRendererID);
 
@@ -1271,10 +1279,107 @@ TEST_F(ChildProcessSecurityPolicyTest, CanAccessDataForOrigin) {
                          run_loop.QuitClosure());
   run_loop.Run();
 
-  // Verify invalid ID is rejected now that Remove() has complted.
+  // Verify invalid ID is rejected now that Remove() has completed.
   EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, file_url));
-  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, http_url));
-  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, http2_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, foo_http_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, foo_blob_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, foo_filesystem_url));
+  EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, bar_http_url));
+}
+
+TEST_F(ChildProcessSecurityPolicyTest, CanAccessDataForOrigin_Origin) {
+  ChildProcessSecurityPolicyImpl* p =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+
+  const std::vector<const char*> foo_urls = {
+      "http://foo.com/index.html",
+      "blob:http://foo.com/43d75119-d7af-4471-a293-07c6b3d7e61a",
+      "filesystem:http://foo.com/temporary/test.html",
+      // Port differences considered equal.
+      "http://foo.com:1234/index.html",
+      "blob:http://foo.com:1234/43d75119-d7af-4471-a293-07c6b3d7e61a",
+      "filesystem:http://foo.com:1234/temporary/test.html"};
+
+  const std::vector<const char*> non_foo_urls = {
+      "file:///etc/passwd",
+      "http://bar.com/index.html",
+      "blob:http://bar.com/43d75119-d7af-4471-a293-07c6b3d7e61a",
+      "filesystem:http://bar.com/temporary/test.html",
+      "data:text/html,Hello!"
+      // foo.com with a different scheme not considered equal.
+      "https://foo.com/index.html",
+      "blob:https://foo.com/43d75119-d7af-4471-a293-07c6b3d7e61a",
+      "filesystem:https://foo.com/temporary/test.html"};
+
+  std::vector<url::Origin> foo_origins;
+  std::vector<url::Origin> non_foo_origins;
+  std::vector<url::Origin> all_origins;
+  for (auto* url : foo_urls) {
+    auto origin = url::Origin::Create(GURL(url));
+    foo_origins.push_back(origin);
+    all_origins.push_back(origin);
+  }
+  auto foo_origin = url::Origin::Create(GURL("http://foo.com"));
+  auto opaque_with_foo_precursor = foo_origin.DeriveNewOpaqueOrigin();
+  foo_origins.push_back(opaque_with_foo_precursor);
+  all_origins.push_back(opaque_with_foo_precursor);
+
+  for (auto* url : non_foo_urls) {
+    auto origin = url::Origin::Create(GURL(url));
+    non_foo_origins.push_back(origin);
+    all_origins.push_back(origin);
+  }
+  url::Origin opaque_origin_without_precursor;
+  non_foo_origins.push_back(opaque_origin_without_precursor);
+  all_origins.push_back(opaque_origin_without_precursor);
+
+  auto opaque_with_bar_precursor =
+      url::Origin::Create(GURL("http://bar.com")).DeriveNewOpaqueOrigin();
+  non_foo_origins.push_back(opaque_with_bar_precursor);
+  all_origins.push_back(opaque_with_bar_precursor);
+
+  // Test invalid ID case.
+  for (const auto& origin : all_origins)
+    EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, origin)) << origin;
+
+  TestBrowserContext browser_context;
+  p->Add(kRendererID, &browser_context);
+
+  // Verify unlocked process permissions.
+  for (const auto& origin : all_origins)
+    EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, origin)) << origin;
+
+  // Isolate |foo_origin| so we can't get a default SiteInstance.
+  p->AddIsolatedOrigins({foo_origin}, IsolatedOriginSource::TEST,
+                        &browser_context);
+
+  // Lock process to |foo_origin| origin.
+  scoped_refptr<SiteInstanceImpl> foo_instance =
+      SiteInstanceImpl::CreateForURL(&browser_context, foo_origin.GetURL());
+  EXPECT_FALSE(foo_instance->IsDefaultSiteInstance());
+  p->LockToOrigin(foo_instance->GetIsolationContext(), kRendererID,
+                  foo_instance->GetSiteURL());
+
+  // Verify that access is no longer allowed for origins that are not associated
+  // with foo.com.
+  for (const auto& origin : foo_origins)
+    EXPECT_TRUE(p->CanAccessDataForOrigin(kRendererID, origin)) << origin;
+
+  for (const auto& origin : non_foo_origins)
+    EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, origin)) << origin;
+
+  p->Remove(kRendererID);
+
+  // Post a task to the IO loop that then posts a task to the UI loop.
+  // This should cause the |run_loop| to return after the removal has completed.
+  base::RunLoop run_loop;
+  base::PostTaskAndReply(FROM_HERE, {BrowserThread::IO}, base::DoNothing(),
+                         run_loop.QuitClosure());
+  run_loop.Run();
+
+  // Verify invalid ID is rejected now that Remove() has completed.
+  for (const auto& origin : all_origins)
+    EXPECT_FALSE(p->CanAccessDataForOrigin(kRendererID, origin)) << origin;
 }
 
 // Test the granting of origin permissions, and their interactions with