Reject IPC requests for isolated origin, sent by non-isolated renderer.

The CL tweaks StoragePartitonInterceptor (in
isolated_origin_browsertest.cc) so that it can be configured to inject
different origins, depending on needs of individual tests.  This tweak
is supported by changes in base/lazy_instance.h (adding of inequality
operator implemented on top of the already existing equality operator),
and in render_process_host_impl.cc/.h (to support creating a test-only
StoragePartitionService via a base::Callback, rather than via a function
pointer).

Tweaks of StoragePartitonInterceptor allow forking of
IsolatedOriginTest.LocalStorageOriginEnforcement test into 2 separate
tests:
- LocalStorageOriginEnforcement_IsolatedAccessingNonIsolated
- LocalStorageOriginEnforcement_NonIsolatedAccessingIsolated
The second test is introduced by this CL and was failing before this CL.

Tweaks of ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin
disallow requests from non-isolated renderers (in which case
CheckOriginLock will return NO_LOCK) if the request is for
an isolated origin.  This makes the new test pass.

Bug: 509125, 764958
Change-Id: Ibfff2c91cb2ac51966e1d89295f10592a3814c08
Reviewed-on: https://chromium-review.googlesource.com/775060
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521838}

content/browser/child_process_security_policy_impl.cc

@@ -259,12 +259,6 @@ 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) {
     origin_lock_ = gurl;
   }
@@ -1046,6 +1040,7 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(int child_id,
   // URLs. Currently, hosted apps cannot set cookies in this mode. See
   // http://crbug.com/160576.
   GURL site_url = SiteInstanceImpl::GetSiteForURL(nullptr, url);
+  bool is_isolated = IsIsolatedOrigin(url::Origin::Create(site_url));
 
   base::AutoLock lock(lock_);
   SecurityStateMap::iterator state = security_state_.find(child_id);
@@ -1054,7 +1049,30 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(int child_id,
     // workaround for https://crbug.com/600441
     return true;
   }
-  bool can_access = state->second->CanAccessDataForOrigin(site_url);
+
+  // Check access in a way that ensures that:
+  // - isolated origins are only allowed access to data for the same origin
+  //   ("jail" enforcement)
+  // - other sites should not be allowed to access data of an isolated origin
+  //   ("citadel" enforcement)
+  // TODO(lukasza): https://crbug.com/509125, https://crbug.com/764958:
+  // Prevent other kinds of forbidden access.  Some cherry-picked examples
+  // of forbidden access:
+  // - open web -> extension
+  // - open web -> WebUI
+  bool can_access = false;
+  switch (state->second->CheckOriginLock(site_url)) {
+    case CheckOriginLockResult::NO_LOCK:
+      can_access = !is_isolated;
+      break;
+    case CheckOriginLockResult::HAS_EQUAL_LOCK:
+      can_access = true;
+      break;
+    case CheckOriginLockResult::HAS_WRONG_LOCK:
+      can_access = false;
+      break;
+  }
+
   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.
@@ -1063,6 +1081,7 @@ bool ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin(int child_id,
     base::debug::SetCrashKeyValue("killed_process_origin_lock",
                                   state->second->origin_lock().spec());
   }
+
   return can_access;
 }
 

content/browser/isolated_origin_browsertest.cc

@@ -2,13 +2,21 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
 #include "content/browser/bad_message.h"
 #include "content/browser/child_process_security_policy_impl.h"
+#include "content/browser/frame_host/render_frame_message_filter.h"
+#include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
+#include "content/common/render_frame_message_filter.mojom.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_features.h"
@@ -28,6 +36,16 @@
 
 namespace content {
 
+namespace {
+
+mojom::RenderFrameMessageFilter* GetFilterForProcess(
+    RenderProcessHost* process) {
+  return static_cast<RenderProcessHostImpl*>(process)
+      ->render_frame_message_filter_for_testing();
+}
+
+}  // namespace
+
 class IsolatedOriginTest : public ContentBrowserTest {
  public:
   IsolatedOriginTest() {}
@@ -854,7 +872,9 @@ class StoragePartitonInterceptor
       public RenderProcessHostObserver {
  public:
   StoragePartitonInterceptor(RenderProcessHostImpl* rph,
-                             mojom::StoragePartitionServiceRequest request) {
+                             mojom::StoragePartitionServiceRequest request,
+                             const url::Origin& origin_to_inject)
+      : origin_to_inject_(origin_to_inject) {
     StoragePartitionImpl* storage_partition =
         static_cast<StoragePartitionImpl*>(rph->GetStoragePartition());
 
@@ -893,9 +913,7 @@ class StoragePartitonInterceptor
   // security checks can be tested.
   void OpenLocalStorage(const url::Origin& origin,
                         mojom::LevelDBWrapperRequest request) override {
-    url::Origin mismatched_origin =
-        url::Origin::Create(GURL("http://abc.foo.com"));
-    GetForwardingInterface()->OpenLocalStorage(mismatched_origin,
+    GetForwardingInterface()->OpenLocalStorage(origin_to_inject_,
                                                std::move(request));
   }
 
@@ -904,27 +922,36 @@ class StoragePartitonInterceptor
   // calls can be forwarded to it.
   mojom::StoragePartitionService* storage_partition_service_;
 
+  url::Origin origin_to_inject_;
+
   DISALLOW_COPY_AND_ASSIGN(StoragePartitonInterceptor);
 };
 
 void CreateTestStoragePartitionService(
+    const url::Origin& origin_to_inject,
     RenderProcessHostImpl* rph,
     mojom::StoragePartitionServiceRequest request) {
   // This object will register as RenderProcessHostObserver, so it will
   // clean itself automatically on process exit.
-  new StoragePartitonInterceptor(rph, std::move(request));
+  new StoragePartitonInterceptor(rph, std::move(request), origin_to_inject);
 }
 
 // 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) {
-  RenderProcessHostImpl::SetCreateStoragePartitionServiceFunction(
-      CreateTestStoragePartitionService);
+IN_PROC_BROWSER_TEST_F(
+    IsolatedOriginTest,
+    LocalStorageOriginEnforcement_IsolatedAccessingNonIsolated) {
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+
+  auto mismatched_origin = url::Origin::Create(GURL("http://abc.foo.com"));
+  EXPECT_FALSE(policy->IsIsolatedOrigin(mismatched_origin));
+  RenderProcessHostImpl::SetStoragePartitionServiceFactoryForTesting(
+      base::BindRepeating(&CreateTestStoragePartitionService,
+                          mismatched_origin));
 
   GURL isolated_url(
       embedded_test_server()->GetURL("isolated.foo.com", "/title1.html"));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin::Create(isolated_url)));
   EXPECT_TRUE(NavigateToURL(shell(), isolated_url));
 
   content::RenderProcessHostWatcher crash_observer(
@@ -938,6 +965,120 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginTest, LocalStorageOriginEnforcement) {
   crash_observer.Wait();
 }
 
+// Verify that a non-isolated renderer process cannot read localStorage of an
+// isolated origin.
+IN_PROC_BROWSER_TEST_F(
+    IsolatedOriginTest,
+    LocalStorageOriginEnforcement_NonIsolatedAccessingIsolated) {
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+
+  GURL isolated_url(embedded_test_server()->GetURL("isolated.foo.com", "/"));
+  auto isolated_origin = url::Origin::Create(isolated_url);
+  EXPECT_TRUE(policy->IsIsolatedOrigin(isolated_origin));
+  RenderProcessHostImpl::SetStoragePartitionServiceFactoryForTesting(
+      base::BindRepeating(&CreateTestStoragePartitionService, isolated_origin));
+
+  GURL non_isolated_url(
+      embedded_test_server()->GetURL("blah.bar.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), non_isolated_url));
+
+  content::RenderProcessHostWatcher crash_observer(
+      shell()->web_contents()->GetMainFrame()->GetProcess(),
+      content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  // 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;"));
+  crash_observer.Wait();
+}
+
+// The RenderFrameMessageFilter should kill the renderer when cookies of an
+// isolated origin are accessed by another (possibly non-isolated, non-locked)
+// origin.
+IN_PROC_BROWSER_TEST_F(IsolatedOriginTest,
+                       GetCookieEnforcement_NonIsolatedAccessingIsolated) {
+  // Gather URLs and Origins used later in the test.
+  GURL isolated_url(embedded_test_server()->GetURL("isolated.foo.com", "/"));
+  auto isolated_origin = url::Origin::Create(isolated_url);
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  EXPECT_TRUE(policy->IsIsolatedOrigin(isolated_origin));
+  GURL non_isolated_url(
+      embedded_test_server()->GetURL("blah.bar.com", "/title2.html"));
+  auto non_isolated_origin = url::Origin::Create(non_isolated_url);
+  EXPECT_FALSE(policy->IsIsolatedOrigin(non_isolated_origin));
+
+  // Navigate to a non-isolated page and verify that it doesn't get origin lock
+  // (unless we are running with --site-per-process).
+  EXPECT_TRUE(NavigateToURL(shell(), non_isolated_url));
+  RenderFrameHost* main_frame = web_contents()->GetMainFrame();
+  GURL origin_lock = policy->GetOriginLock(main_frame->GetProcess()->GetID());
+  if (AreAllSitesIsolatedForTesting())
+    EXPECT_FALSE(origin_lock.is_empty());
+  else
+    EXPECT_TRUE(origin_lock.is_empty());
+
+  // Try to get cross-site cookies from the main frame's process and wait for it
+  // to be killed.
+  RenderProcessHostWatcher crash_observer(
+      main_frame->GetProcess(),
+      RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+      ->PostTask(FROM_HERE,
+                 base::BindOnce(
+                     [](const GURL& cookie_url, RenderFrameHost* frame) {
+                       GetFilterForProcess(frame->GetProcess())
+                           ->GetCookies(
+                               frame->GetRoutingID(), cookie_url, cookie_url,
+                               base::BindOnce([](const std::string&) {}));
+                     },
+                     isolated_url, base::Unretained(main_frame)));
+  crash_observer.Wait();
+}
+
+// The RenderFrameMessageFilter should kill the renderer when cookies of an
+// isolated origin are set by another (possibly non-isolated, non-locked)
+// origin.
+IN_PROC_BROWSER_TEST_F(IsolatedOriginTest,
+                       SetCookieEnforcement_NonIsolatedAccessingIsolated) {
+  // Gather URLs and Origins used later in the test.
+  GURL isolated_url(
+      embedded_test_server()->GetURL("isolated.foo.com", "/title1.html"));
+  auto isolated_origin = url::Origin::Create(isolated_url);
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  EXPECT_TRUE(policy->IsIsolatedOrigin(isolated_origin));
+  GURL non_isolated_url(
+      embedded_test_server()->GetURL("blah.bar.com", "/title2.html"));
+  auto non_isolated_origin = url::Origin::Create(non_isolated_url);
+  EXPECT_FALSE(policy->IsIsolatedOrigin(non_isolated_origin));
+
+  // Navigate to a non-isolated page and verify that it doesn't get origin lock.
+  EXPECT_TRUE(NavigateToURL(shell(), non_isolated_url));
+  RenderFrameHost* main_frame = web_contents()->GetMainFrame();
+  GURL origin_lock = policy->GetOriginLock(main_frame->GetProcess()->GetID());
+  if (AreAllSitesIsolatedForTesting())
+    EXPECT_FALSE(origin_lock.is_empty());
+  else
+    EXPECT_TRUE(origin_lock.is_empty());
+
+  // Try to get cross-site cookies from the main frame's process and wait for it
+  // to be killed.
+  RenderProcessHostWatcher crash_observer(
+      main_frame->GetProcess(),
+      RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+      ->PostTask(FROM_HERE,
+                 base::BindOnce(
+                     [](const GURL& cookie_url, RenderFrameHost* frame) {
+                       GetFilterForProcess(frame->GetProcess())
+                           ->SetCookie(frame->GetRoutingID(), cookie_url,
+                                       cookie_url, "pwn=ed",
+                                       base::BindOnce(&base::DoNothing));
+                     },
+                     isolated_url, main_frame));
+  crash_observer.Wait();
+}
+
 class IsolatedOriginFieldTrialTest : public ContentBrowserTest {
  public:
   IsolatedOriginFieldTrialTest() {

content/browser/renderer_host/render_process_host_impl.cc

@@ -804,7 +804,7 @@ class SiteProcessCountTracker : public base::SupportsUserData::Data,
     std::map<ProcessID, Count>& counts_per_process = result->second;
 
     --counts_per_process[render_process_host_id];
-    DCHECK(counts_per_process[render_process_host_id] >= 0);
+    DCHECK_GE(counts_per_process[render_process_host_id], 0);
 
     if (counts_per_process[render_process_host_id] == 0)
       counts_per_process.erase(render_process_host_id);
@@ -1057,8 +1057,9 @@ void CopyFeatureSwitch(const base::CommandLine& src,
 }  // namespace
 
 RendererMainThreadFactoryFunction g_renderer_main_thread_factory = nullptr;
-RenderProcessHostImpl::CreateStoragePartitionServiceFunction
-    g_create_storage_partition = nullptr;
+
+base::LazyInstance<RenderProcessHostImpl::StoragePartitionServiceFactory>::Leaky
+    g_storage_partition_service_factory = LAZY_INSTANCE_INITIALIZER;
 
 base::MessageLoop* g_in_process_thread;
 
@@ -1389,9 +1390,9 @@ void RenderProcessHostImpl::RegisterRendererMainThreadFactory(
   g_renderer_main_thread_factory = create;
 }
 
-void RenderProcessHostImpl::SetCreateStoragePartitionServiceFunction(
-    CreateStoragePartitionServiceFunction function) {
-  g_create_storage_partition = function;
+void RenderProcessHostImpl::SetStoragePartitionServiceFactoryForTesting(
+    StoragePartitionServiceFactory factory) {
+  g_storage_partition_service_factory.Get() = factory;
 }
 
 RenderProcessHostImpl::~RenderProcessHostImpl() {
@@ -2034,8 +2035,8 @@ void RenderProcessHostImpl::CreateStoragePartitionService(
     mojom::StoragePartitionServiceRequest request) {
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableMojoLocalStorage)) {
-    if (g_create_storage_partition) {
-      g_create_storage_partition(this, std::move(request));
+    if (!(g_storage_partition_service_factory == nullptr)) {
+      g_storage_partition_service_factory.Get().Run(this, std::move(request));
       return;
     }
 

content/browser/renderer_host/render_process_host_impl.h

@@ -13,7 +13,10 @@
 #include <queue>
 #include <set>
 #include <string>
+#include <utility>
+#include <vector>
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/observer_list.h"
@@ -315,11 +318,11 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // Allows external code to supply a function which creates a
   // StoragePartitionService. Used for supplying test versions of the
   // service.
-  using CreateStoragePartitionServiceFunction =
-      void (*)(RenderProcessHostImpl* rph,
-               mojom::StoragePartitionServiceRequest request);
-  static void SetCreateStoragePartitionServiceFunction(
-      CreateStoragePartitionServiceFunction function);
+  using StoragePartitionServiceFactory = base::RepeatingCallback<void(
+      RenderProcessHostImpl* rph,
+      mojom::StoragePartitionServiceRequest request)>;
+  static void SetStoragePartitionServiceFactoryForTesting(
+      StoragePartitionServiceFactory factory);
 
   RenderFrameMessageFilter* render_frame_message_filter_for_testing() const {
     return render_frame_message_filter_.get();