Citadel-style, UI-thread-only, Android-only enforcement of Site Isolation.

This CL disallows an unlocked renderer process from accessing
origins/urls that require isolation (i.e. require a process lock).

The new enforcement works only on the UI thread - calls to
CanAccessDataForOrigin on the IO thread will unfortunately continue to
allow access for all unlocked renderer processes.  See also
https://docs.google.com/document/d/17t85azUYz-Wzo9vPrrghd47ilxjqCryDNJPGROKc8Es

The new enforcement is enabled only on Android.  On desktop, the
citadel-style enforcement is blocked by NTP support for OOPIFs - see
https://crbug.com/566091 and also
https://docs.google.com/document/d/1jtanAByotYMaJzdxd2aIknwoVz1kjxL_HAR_mKBsJls

Bug: 764958
Change-Id: I7864ca737c14c26c3c8cefe3cc2887028f891679
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545157Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699436}

android_webview/browser/aw_content_browser_client.cc

@@ -934,6 +934,17 @@ bool AwContentBrowserClient::ShouldEnableStrictSiteIsolation() {
   return false;
 }
 
+bool AwContentBrowserClient::ShouldLockToOrigin(
+    content::BrowserContext* browser_context,
+    const GURL& effective_url) {
+  // TODO(lukasza): https://crbug.cmo/869494: Once Android WebView supports
+  // OOPIFs, we should remove this ShouldLockToOrigin overload.  Till then,
+  // returning false helps avoid accidentally applying citadel-style Site
+  // Isolation enforcement to Android WebView (and causing incorrect renderer
+  // kills).
+  return false;
+}
+
 bool AwContentBrowserClient::WillCreateURLLoaderFactory(
     content::BrowserContext* browser_context,
     content::RenderFrameHost* frame,

android_webview/browser/aw_content_browser_client.h

@@ -191,6 +191,8 @@ class AwContentBrowserClient : public content::ContentBrowserClient {
       NonNetworkURLLoaderFactoryMap* factories) override;
   bool ShouldIsolateErrorPage(bool in_main_frame) override;
   bool ShouldEnableStrictSiteIsolation() override;
+  bool ShouldLockToOrigin(content::BrowserContext* browser_context,
+                          const GURL& effective_url) override;
   bool WillCreateURLLoaderFactory(
       content::BrowserContext* browser_context,
       content::RenderFrameHost* frame,

content/browser/child_process_security_policy_impl.cc

@@ -329,18 +329,22 @@ class ChildProcessSecurityPolicyImpl::SecurityState {
     return false;
   }
 
-  bool CanAccessDataForOrigin(const GURL& site_url) {
-    if (origin_lock_.is_empty())
-      return true;
-    return origin_lock_ == site_url;
-  }
-
   void LockToOrigin(const GURL& gurl, BrowsingInstanceId browsing_instance_id) {
     DCHECK(origin_lock_.is_empty());
+    DCHECK_NE(SiteInstanceImpl::GetDefaultSiteURL(), gurl);
     origin_lock_ = gurl;
     lowest_browsing_instance_id_ = browsing_instance_id;
   }
 
+  void SetLowestBrowsingInstanceId(
+      BrowsingInstanceId new_browsing_instance_id_to_include) {
+    DCHECK(!new_browsing_instance_id_to_include.is_null());
+    if (lowest_browsing_instance_id_.is_null() ||
+        (new_browsing_instance_id_to_include < lowest_browsing_instance_id_)) {
+      lowest_browsing_instance_id_ = new_browsing_instance_id_to_include;
+    }
+  }
+
   const GURL& origin_lock() { return origin_lock_; }
 
   BrowsingInstanceId lowest_browsing_instance_id() {
@@ -1320,43 +1324,99 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(
 bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(int child_id,
                                                             const GURL& url) {
   DCHECK(IsRunningOnExpectedThread());
-
   base::AutoLock lock(lock_);
+
   SecurityState* security_state = GetSecurityState(child_id);
+  BrowserOrResourceContext browser_or_resource_context;
+  if (security_state)
+    browser_or_resource_context = security_state->GetBrowserOrResourceContext();
 
-  // Determine the BrowsingInstance ID for calculating the expected process
-  // lock URL.
   GURL expected_process_lock;
-  BrowserOrResourceContext context;
-  if (security_state) {
-    context = security_state->GetBrowserOrResourceContext();
-    if (context) {
-      BrowsingInstanceId browsing_instance_id =
-          security_state->lowest_browsing_instance_id();
-      expected_process_lock = SiteInstanceImpl::DetermineProcessLockURL(
-          IsolationContext(browsing_instance_id, context), url);
+  if (security_state && browser_or_resource_context) {
+    IsolationContext isolation_context(
+        security_state->lowest_browsing_instance_id(),
+        browser_or_resource_context);
+    expected_process_lock =
+        SiteInstanceImpl::DetermineProcessLockURL(isolation_context, url);
+
+    GURL actual_process_lock = security_state->origin_lock();
+    if (!actual_process_lock.is_empty()) {
+      // Jail-style enforcement - a process with a lock can only access data
+      // from origins that require exactly the same lock.
+      if (actual_process_lock == expected_process_lock)
+        return true;
+    } else {
+      // Citadel-style enforcement - an unlocked process should not be able to
+      // access data from origins that require a lock.
+#if !defined(OS_ANDROID)
+      // TODO(lukasza): https://crbug.com/566091: Once remote NTP is capable of
+      // embedding OOPIFs, start enforcing citadel-style checks on desktop
+      // platforms.
+      // TODO(lukasza): https://crbug.com/614463: Enforce isolation within
+      // GuestView (once OOPIFs are supported within GuestView).
+      return true;
+#else
+      // TODO(acolwell, lukasza): https://crbug.com/764958: Make it possible to
+      // call ShouldLockToOrigin (and GetSiteForURL?) on the IO thread.
+      if (BrowserThread::CurrentlyOn(BrowserThread::IO))
+        return true;
+      DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+      // TODO(lukasza): Consider making the checks below IO-thread-friendly, by
+      // storing |is_unused| inside SecurityState.
+      RenderProcessHost* process = RenderProcessHostImpl::FromID(child_id);
+      if (process) {  // |process| can be null in unittests
+        // Unlocked process can be legitimately used when navigating from an
+        // unused process (about:blank, NTP on Android) to an isolated origin.
+        // See also https://crbug.com/945399.  Returning |true| below will allow
+        // such navigations to succeed (i.e. pass CanCommitOriginAndUrl checks).
+        // We don't expect unused processes to be used outside of navigations
+        // (e.g. when checking CanAccessDataForOrigin for localStorage, etc.).
+        if (process->IsUnused())
+          return true;
+      }
+
+      // TODO(alexmos, lukasza): https://crbug.com/764958: Consider making
+      // ShouldLockToOrigin work with |expected_process_lock| instead of
+      // |site_url|.
+      GURL site_url = SiteInstanceImpl::GetSiteForURL(isolation_context, url);
+
+      // A process with no lock can only access data from origins that do not
+      // require a locked process.
+      bool should_lock_target =
+          SiteInstanceImpl::ShouldLockToOrigin(isolation_context, site_url);
+      if (!should_lock_target)
+        return true;
+#endif
     }
   }
 
-  bool can_access =
-      context && security_state &&
-      security_state->CanAccessDataForOrigin(expected_process_lock);
-  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.
   std::string killed_process_origin_lock;
   if (!security_state) {
     killed_process_origin_lock = "(child id not found)";
-    } else if (!context) {
+  } else if (!browser_or_resource_context) {
     killed_process_origin_lock = "(context is null)";
+  } else if (security_state->origin_lock().is_empty()) {
+    killed_process_origin_lock = "(no lock - citadel-enforcement)";
   } else {
     killed_process_origin_lock = security_state->origin_lock().spec();
   }
   LogCanAccessDataForOriginCrashKeys(expected_process_lock.spec(),
                                      killed_process_origin_lock,
                                      url.GetOrigin().spec());
-  }
-  return can_access;
+  return false;
+}
+
+void ChildProcessSecurityPolicyImpl::IncludeIsolationContext(
+    int child_id,
+    const IsolationContext& isolation_context) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  base::AutoLock lock(lock_);
+  auto* state = GetSecurityState(child_id);
+  DCHECK(state);
+  state->SetLowestBrowsingInstanceId(isolation_context.browsing_instance_id());
 }
 
 void ChildProcessSecurityPolicyImpl::LockToOrigin(

content/browser/child_process_security_policy_impl.h

@@ -259,6 +259,12 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // Returns true if the specified child_id has been granted ReadRawCookies.
   bool CanReadRawCookies(int child_id);
 
+  // Notifies security state of |child_id| about the IsolationContext it will
+  // host.  The main side effect is proper setting of the lowest
+  // BrowsingInstanceId associated with the security state.
+  void IncludeIsolationContext(int child_id,
+                               const IsolationContext& isolation_context);
+
   // Sets the process identified by |child_id| as only permitted to access data
   // for the origin specified by |lock_url|. Most callers should use
   // RenderProcessHostImpl::LockToOrigin instead of calling this directly.

content/browser/isolated_origin_browsertest.cc

@@ -1239,8 +1239,6 @@ void CreateTestStoragePartitionService(
 
 // Verify that an isolated renderer process cannot read localStorage of an
 // origin outside of its isolated site.
-// TODO(nasko): Write a test to verify the opposite - any non-isolated renderer
-// process cannot access data of an isolated site.
 IN_PROC_BROWSER_TEST_F(
     IsolatedOriginTest,
     LocalStorageOriginEnforcement_IsolatedAccessingNonIsolated) {
@@ -1253,6 +1251,7 @@ IN_PROC_BROWSER_TEST_F(
   GURL isolated_url(
       embedded_test_server()->GetURL("isolated.foo.com", "/title1.html"));
   EXPECT_TRUE(IsIsolatedOrigin(url::Origin::Create(isolated_url)));
+
   EXPECT_TRUE(NavigateToURL(shell(), isolated_url));
 
   content::RenderProcessHostKillWaiter kill_waiter(
@@ -1265,6 +1264,45 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_EQ(bad_message::RPH_MOJO_PROCESS_ERROR, kill_waiter.Wait());
 }
 
+#if defined(OS_ANDROID)
+#define MAYBE_LocalStorageOriginEnforcement_NonIsolatedAccessingIsolated \
+  LocalStorageOriginEnforcement_NonIsolatedAccessingIsolated
+#else
+// TODO(lukasza): https://crbug.com/566091: Once remote NTP is capable of
+// embedding OOPIFs, start enforcing citadel-style checks on desktop
+// platforms.
+#define MAYBE_LocalStorageOriginEnforcement_NonIsolatedAccessingIsolated \
+  DISABLED_LocalStorageOriginEnforcement_NonIsolatedAccessingIsolated
+#endif
+// Verify that a non-isolated renderer process cannot read localStorage of an
+// isolated origin.
+//
+// TODO(alexmos, lukasza): https://crbug.com/764958: Replicate this test for
+// the IO-thread case.
+IN_PROC_BROWSER_TEST_F(
+    IsolatedOriginTest,
+    MAYBE_LocalStorageOriginEnforcement_NonIsolatedAccessingIsolated) {
+  auto isolated_origin = url::Origin::Create(GURL("http://isolated.foo.com"));
+  EXPECT_TRUE(IsIsolatedOrigin(isolated_origin));
+
+  GURL nonisolated_url(
+      embedded_test_server()->GetURL("non-isolated.com", "/title1.html"));
+  EXPECT_FALSE(IsIsolatedOrigin(url::Origin::Create(nonisolated_url)));
+
+  RenderProcessHostImpl::SetStoragePartitionServiceRequestHandlerForTesting(
+      base::BindRepeating(&CreateTestStoragePartitionService, isolated_origin));
+  EXPECT_TRUE(NavigateToURL(shell(), nonisolated_url));
+
+  content::RenderProcessHostKillWaiter kill_waiter(
+      shell()->web_contents()->GetMainFrame()->GetProcess());
+  // Use ignore_result here, since on Android the renderer process is
+  // terminated, but ExecuteScript still returns true. It properly returns
+  // false on all other platforms.
+  ignore_result(ExecuteScript(shell()->web_contents()->GetMainFrame(),
+                              "localStorage.length;"));
+  EXPECT_EQ(bad_message::RPH_MOJO_PROCESS_ERROR, kill_waiter.Wait());
+}
+
 // Verify that an IPC request for reading localStorage of an *opaque* origin
 // will be rejected.
 IN_PROC_BROWSER_TEST_F(IsolatedOriginTest,

content/browser/site_instance_impl.cc

@@ -1046,6 +1046,12 @@ void SiteInstanceImpl::LockToOriginIfNeeded() {
                    << " in process locked to " << process_lock;
     }
   }
+
+  // Track which isolation contexts use the given process.  This lets
+  // ChildProcessSecurityPolicyImpl (e.g. CanAccessDataForOrigin) determine
+  // whether a given URL should require a lock or not (a dynamically isolated
+  // origin may require a lock in some isolation contexts but not in others).
+  policy->IncludeIsolationContext(process_->GetID(), GetIsolationContext());
 }
 
 // static