IsolatedWorldCSP: Enforce isolated world CSP checks for img resources.

This CL ensures that the isolated world CSP is used for CSP checks while loading images from isolated worlds. It also simplifies the ImageLoader code. BUG=1099975 Change-Id: I9ea9be9c0b960e49b0420c1db17b7eed4a1ed47e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2276816 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#785608}
chromium · Jul 7, 2020 · 10111bd · 10111bd
1 parent edb1628
commit 10111bd
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 98 deletions.
diff --git a/third_party/blink/renderer/core/execution_context/execution_context.cc b/third_party/blink/renderer/core/execution_context/execution_context.cc
@@ -232,19 +232,23 @@ ExecutionContext::GetContentSecurityPolicyDelegate() {
   return *csp_delegate_;
 }
 
-ContentSecurityPolicy*
-ExecutionContext::GetContentSecurityPolicyForCurrentWorld() {
+scoped_refptr<const DOMWrapperWorld> ExecutionContext::GetCurrentWorld() const {
   v8::Isolate* isolate = GetIsolate();
   v8::HandleScope handle_scope(isolate);
   v8::Local<v8::Context> v8_context = isolate->GetCurrentContext();
 
-  // This can be called before we enter v8, hence the context might be empty,
-  // which implies we are not in an isolated world.
+  // This can be called before we enter v8, hence the context might be empty.
   if (v8_context.IsEmpty())
-    return GetContentSecurityPolicy();
+    return nullptr;
+
+  return &DOMWrapperWorld::Current(isolate);
+}
 
-  return GetContentSecurityPolicyForWorld(
-      DOMWrapperWorld::Current(GetIsolate()));
+ContentSecurityPolicy*
+ExecutionContext::GetContentSecurityPolicyForCurrentWorld() {
+  scoped_refptr<const DOMWrapperWorld> world = GetCurrentWorld();
+  return world ? GetContentSecurityPolicyForWorld(*world)
+               : GetContentSecurityPolicy();
 }
 
 ContentSecurityPolicy* ExecutionContext::GetContentSecurityPolicyForWorld(

diff --git a/third_party/blink/renderer/core/execution_context/execution_context.h b/third_party/blink/renderer/core/execution_context/execution_context.h
@@ -161,6 +161,10 @@ class CORE_EXPORT ExecutionContext : public Supplementable<ExecutionContext>,
   network::mojom::blink::WebSandboxFlags GetSandboxFlags() const;
   bool IsSandboxed(network::mojom::blink::WebSandboxFlags mask) const;
 
+  // Returns a reference to the current world we are in. If the current v8
+  // context is empty, returns null.
+  scoped_refptr<const DOMWrapperWorld> GetCurrentWorld() const;
+
   // Returns the content security policy to be used based on the current
   // JavaScript world we are in.
   // Note: As part of crbug.com/896041, existing usages of

diff --git a/third_party/blink/renderer/core/loader/image_loader.cc b/third_party/blink/renderer/core/loader/image_loader.cc
@@ -120,69 +120,38 @@ bool CanReuseFromListOfAvailableImages(
 
 }  // namespace
 
-static ImageLoader::BypassMainWorldBehavior ShouldBypassMainWorldCSP(
-    ImageLoader* loader) {
-  DCHECK(loader);
-  DCHECK(loader->GetElement());
-  if (ContentSecurityPolicy::ShouldBypassMainWorld(
-          loader->GetElement()->GetExecutionContext())) {
-    return ImageLoader::kBypassMainWorldCSP;
-  }
-  return ImageLoader::kDoNotBypassMainWorldCSP;
-}
-
 class ImageLoader::Task {
  public:
   Task(ImageLoader* loader,
        UpdateFromElementBehavior update_behavior,
        network::mojom::ReferrerPolicy referrer_policy)
       : loader_(loader),
-        should_bypass_main_world_csp_(ShouldBypassMainWorldCSP(loader)),
         update_behavior_(update_behavior),
         referrer_policy_(referrer_policy) {
     ExecutionContext* context = loader_->GetElement()->GetExecutionContext();
     probe::AsyncTaskScheduled(context, "Image", &async_task_id_);
-    v8::Isolate* isolate = V8PerIsolateData::MainThreadIsolate();
-    v8::HandleScope scope(isolate);
-    // If we're invoked from C++ without a V8 context on the stack, we should
-    // run the microtask in the context of the element's document's main world.
-    if (!isolate->GetCurrentContext().IsEmpty()) {
-      script_state_ = ScriptState::Current(isolate);
-    } else {
-      script_state_ = ToScriptStateForMainWorld(
-          loader->GetElement()->GetDocument().GetFrame());
-      DCHECK(script_state_);
-    }
+    world_ = context->GetCurrentWorld();
   }
 
   void Run() {
     if (!loader_)
       return;
     ExecutionContext* context = loader_->GetElement()->GetExecutionContext();
     probe::AsyncTask async_task(context, &async_task_id_);
-    if (script_state_ && script_state_->ContextIsValid()) {
-      ScriptState::Scope scope(script_state_);
-      loader_->DoUpdateFromElement(should_bypass_main_world_csp_,
-                                   update_behavior_, referrer_policy_);
-    } else {
-      // This call does not access v8::Context internally.
-      loader_->DoUpdateFromElement(should_bypass_main_world_csp_,
-                                   update_behavior_, referrer_policy_);
-    }
+    loader_->DoUpdateFromElement(world_, update_behavior_, referrer_policy_);
   }
 
   void ClearLoader() {
     loader_ = nullptr;
-    script_state_ = nullptr;
+    world_ = nullptr;
   }
 
   base::WeakPtr<Task> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
 
  private:
   WeakPersistent<ImageLoader> loader_;
-  BypassMainWorldBehavior should_bypass_main_world_csp_;
   UpdateFromElementBehavior update_behavior_;
-  WeakPersistent<ScriptState> script_state_;
+  scoped_refptr<const DOMWrapperWorld> world_;
   network::mojom::ReferrerPolicy referrer_policy_;
 
   probe::AsyncTaskId async_task_id_;
@@ -401,14 +370,8 @@ void ImageLoader::SetImageWithoutConsideringPendingLoadEvent(
 
 static void ConfigureRequest(
     FetchParameters& params,
-    ImageLoader::BypassMainWorldBehavior bypass_behavior,
     Element& element,
     const ClientHintsPreferences& client_hints_preferences) {
-  if (bypass_behavior == ImageLoader::kBypassMainWorldCSP) {
-    params.SetContentSecurityCheck(
-        network::mojom::CSPDisposition::DO_NOT_CHECK);
-  }
-
   CrossOriginAttributeValue cross_origin = GetCrossOriginAttributeValue(
       element.FastGetAttribute(html_names::kCrossoriginAttr));
   if (cross_origin != kCrossOriginAttributeNotSet) {
@@ -483,7 +446,7 @@ void ImageLoader::UpdateImageState(ImageResourceContent* new_image_content) {
 }
 
 void ImageLoader::DoUpdateFromElement(
-    BypassMainWorldBehavior bypass_behavior,
+    scoped_refptr<const DOMWrapperWorld> world,
     UpdateFromElementBehavior update_behavior,
     network::mojom::ReferrerPolicy referrer_policy,
     UpdateType update_type) {
@@ -511,6 +474,7 @@ void ImageLoader::DoUpdateFromElement(
     // Unlike raw <img>, we block mixed content inside of <picture> or
     // <img srcset>.
     ResourceLoaderOptions resource_loader_options;
+    resource_loader_options.world = std::move(world);
     resource_loader_options.initiator_info.name = GetElement()->localName();
     ResourceRequest resource_request(url);
     if (update_behavior == kUpdateForcedReload) {
@@ -557,7 +521,7 @@ void ImageLoader::DoUpdateFromElement(
     DCHECK(document.GetFrame());
     FetchParameters params(std::move(resource_request),
                            resource_loader_options);
-    ConfigureRequest(params, bypass_behavior, *element_,
+    ConfigureRequest(params, *element_,
                      document.GetFrame()->GetClientHintsPreferences());
 
     if (update_behavior != kUpdateForcedReload &&
@@ -712,8 +676,8 @@ void ImageLoader::UpdateFromElement(
   }
 
   if (ShouldLoadImmediately(ImageSourceToKURL(image_source_url))) {
-    DoUpdateFromElement(kDoNotBypassMainWorldCSP, update_behavior,
-                        referrer_policy, UpdateType::kSync);
+    DoUpdateFromElement(element_->GetExecutionContext()->GetCurrentWorld(),
+                        update_behavior, referrer_policy, UpdateType::kSync);
     return;
   }
   // Allow the idiom "img.src=''; img.src='.." to clear down the image before an

diff --git a/third_party/blink/renderer/core/loader/image_loader.h b/third_party/blink/renderer/core/loader/image_loader.h
@@ -40,6 +40,7 @@
 namespace blink {
 
 class ContainerNode;
+class DOMWrapperWorld;
 class Element;
 class ExceptionState;
 class IncrementLoadEventDelayCount;
@@ -74,11 +75,6 @@ class CORE_EXPORT ImageLoader : public GarbageCollected<ImageLoader>,
     kUpdateForcedReload
   };
 
-  enum BypassMainWorldBehavior {
-    kBypassMainWorldCSP,
-    kDoNotBypassMainWorldCSP
-  };
-
   void UpdateFromElement(UpdateFromElementBehavior = kUpdateNormal,
                          network::mojom::ReferrerPolicy =
                              network::mojom::ReferrerPolicy::kDefault);
@@ -170,7 +166,7 @@ class CORE_EXPORT ImageLoader : public GarbageCollected<ImageLoader>,
 
   // Called from the task or from updateFromElement to initiate the load.
   void DoUpdateFromElement(
-      BypassMainWorldBehavior,
+      scoped_refptr<const DOMWrapperWorld> world,
       UpdateFromElementBehavior,
       network::mojom::ReferrerPolicy = network::mojom::ReferrerPolicy::kDefault,
       UpdateType = UpdateType::kAsync);

diff --git a/...arty/blink/web_tests/http/tests/security/isolatedWorld/bypass-main-world-csp-expected.txt b/...arty/blink/web_tests/http/tests/security/isolatedWorld/bypass-main-world-csp-expected.txt
@@ -1,30 +1,23 @@
-ALERT: Running test #6
+ALERT: Running test #4
 
-CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?6' because it violates the following Content Security Policy directive: "img-src 'none'".
+ALERT: Test in main world.
+CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?4' because it violates the following Content Security Policy directive: "img-src 'none'".
 
 ALERT: BLOCKED
-ALERT: Running test #5
+ALERT: Running test #3
 
-CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?5' because it violates the following Content Security Policy directive: "img-src 'none'".
+ALERT: Test in isolated world without a CSP.
+CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?3' because it violates the following Content Security Policy directive: "img-src 'none'".
 
 ALERT: BLOCKED
-ALERT: Running test #4
-
-ALERT: Starting to bypass main world's CSP:
-ALERT: LOADED
-ALERT: Running test #3
-
-ALERT: LOADED
 ALERT: Running test #2
 
-CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?2' because it violates the following Content Security Policy directive: "img-src 'none'".
-
-ALERT: BLOCKED
+ALERT: Test in isolated world with lax CSP
+ALERT: LOADED
 ALERT: Running test #1
 
-CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?1' because it violates the following Content Security Policy directive: "img-src 'none'".
-
-ALERT: BLOCKED
+ALERT: Test in isolated world with restrictive CSP
+ALERT: LOADED
 ALERT: Running test #0
 
- This test ensures that scripts run in isolated worlds marked with their own Content Security Policy aren't affected by the page's content security policy. Extensions, for example, should be able to load any resource they like.
+ This test ensures that img-src checks respect the isolated world CSP when the IsolatedWorldCSP feature is enabled and bypass the main world CSP checks otherwise.
diff --git a/third_party/blink/web_tests/http/tests/security/isolatedWorld/bypass-main-world-csp.html b/third_party/blink/web_tests/http/tests/security/isolatedWorld/bypass-main-world-csp.html
@@ -8,7 +8,7 @@
         testRunner.waitUntilDone();
     }
 
-    tests = 6;
+    tests = 4;
     window.addEventListener("message", function(message) {
         tests -= 1;
         test();
@@ -32,36 +32,30 @@
     }
 
     function test() {
-        function setImgSrc(isolated, num) {
+        function setImgSrc(num) {
             var img = document.getElementById('testimg');
             img.src = "../resources/abe.png?" + num;
         }
 
         alert("Running test #" + tests + "\n");
         switch (tests) {
-            case 6:
-                setImgSrc(false, 6);
-                break;
-            case 5:
-                testRunner.evaluateScriptInIsolatedWorld(1, String(eval("setImgSrc")) + "\nsetImgSrc(true, 5);");
-                break;
             case 4:
-                alert("Starting to bypass main world's CSP:");
-                testRunner.setIsolatedWorldInfo(1, 'chrome-extension://123', 'img-src *');
-                testRunner.evaluateScriptInIsolatedWorld(1, String(eval("setImgSrc")) + "\nsetImgSrc(true, 4);");
+                alert("Test in main world.");
+                setImgSrc(4);
                 break;
             case 3:
-                // Main world, then isolated world -> should load
-                setImgSrc(false, 3);
-                testRunner.evaluateScriptInIsolatedWorld(1, String(eval("setImgSrc")) + "\nsetImgSrc(true, 3);");
+                alert("Test in isolated world without a CSP.");
+                testRunner.evaluateScriptInIsolatedWorld(1, String(eval("setImgSrc")) + "\nsetImgSrc(3);");
                 break;
             case 2:
-                // Isolated world, then main world -> should block
-                testRunner.evaluateScriptInIsolatedWorld(1, String(eval("setImgSrc")) + "\nsetImgSrc(true, 2);");
-                setImgSrc(false, 2);
+                alert("Test in isolated world with lax CSP");
+                testRunner.setIsolatedWorldInfo(1, 'chrome-extension://123', 'img-src *');
+                testRunner.evaluateScriptInIsolatedWorld(1, String(eval("setImgSrc")) + "\nsetImgSrc(2);");
                 break;
             case 1:
-                setImgSrc(false, 1);
+                alert("Test in isolated world with restrictive CSP");
+                testRunner.setIsolatedWorldInfo(1, 'chrome-extension://123', "img-src 'self'");
+                testRunner.evaluateScriptInIsolatedWorld(1, String(eval("setImgSrc")) + "\nsetImgSrc(0);");
                 break;
             case 0:
                 testRunner.setIsolatedWorldInfo(1, null, null);
@@ -74,10 +68,9 @@
 <body onload='setup();'>
     <p>
         <img id="testimg">
-        This test ensures that scripts run in isolated worlds marked with their
-        own Content Security Policy aren't affected by the page's content
-        security policy. Extensions, for example, should be able to load any
-        resource they like.
+        This test ensures that img-src checks respect the isolated world CSP
+        when the IsolatedWorldCSP feature is enabled and bypass the main world
+        CSP checks otherwise.
     </p>
 </body>
 </html>
diff --git a/...l/isolated_world_csp/http/tests/security/isolatedWorld/bypass-main-world-csp-expected.txt b/...l/isolated_world_csp/http/tests/security/isolatedWorld/bypass-main-world-csp-expected.txt
@@ -0,0 +1,25 @@
+ALERT: Running test #4
+
+ALERT: Test in main world.
+CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?4' because it violates the following Content Security Policy directive: "img-src 'none'".
+
+ALERT: BLOCKED
+ALERT: Running test #3
+
+ALERT: Test in isolated world without a CSP.
+CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?3' because it violates the following Content Security Policy directive: "img-src 'none'".
+
+ALERT: BLOCKED
+ALERT: Running test #2
+
+ALERT: Test in isolated world with lax CSP
+ALERT: LOADED
+ALERT: Running test #1
+
+ALERT: Test in isolated world with restrictive CSP
+CONSOLE ERROR: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png?0' because it violates the following Content Security Policy directive: "img-src 'self'".
+
+ALERT: BLOCKED
+ALERT: Running test #0
+
+ This test ensures that img-src checks respect the isolated world CSP when the IsolatedWorldCSP feature is enabled and bypass the main world CSP checks otherwise.