Fetch: Plumb request initiator through passthrough service workers.

This CL contains essentially two changes: 1. The request initiator origin is plumbed through service workers that do `fetch(evt.request)`. In addition to plumbing, this requires changes to how we validate navigation requests in the CorsURLLoaderFactory. 2. Tracks the original destination of a request passed through a service worker. This is then used in the network service to force SameSite=Lax cookies to treat the request as a main frame navigation where appropriate. For more detailed information about these changes please see the internal design doc at: https://docs.google.com/document/d/1KZscujuV7bCFEnzJW-0DaCPU-I40RJimQKoCcI0umTQ/edit?usp=sharing In addition, there is some discussion of these features in the following spec issues: whatwg/fetch#1321 whatwg/fetch#1327 The test includes WPT tests that verify navigation headers and SameSite cookies. Note, chrome has a couple expected failures in the SameSite cookie tests because of the "lax-allowing-unsafe" intervention that is currently enabled. See: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/TestExpectations;l=4635;drc=e8133cbf2469adb99c6610483ab78bcfb8cc4c76 Bug: 1115847,1241188 Change-Id: I7e236fa20aeabb705aef40fcf8d5c36da6d2798c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3115917 Reviewed-by: Matt Menke <[email protected]> Reviewed-by: Yutaka Hirano <[email protected]> Reviewed-by: Nasko Oskov <[email protected]> Reviewed-by: Łukasz Anforowicz <[email protected]> Commit-Queue: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#936029}
jamlee-t · Oct 28, 2021 · da0a650 · da0a650
1 parent 8d14664
commit da0a650
Show file tree

Hide file tree

Showing 34 changed files with 1,206 additions and 48 deletions.
diff --git a/content/common/background_fetch/background_fetch_types.cc b/content/common/background_fetch/background_fetch_types.cc
@@ -54,10 +54,11 @@ blink::mojom::FetchAPIRequestPtr BackgroundFetchSettledFetch::CloneRequest(
       request->mode, request->is_main_resource_load, request->destination,
       request->frame_type, request->url, request->method, request->headers,
       CloneSerializedBlob(request->blob), request->body,
-      request->referrer.Clone(), request->credentials_mode, request->cache_mode,
-      request->redirect_mode, request->integrity, request->priority,
-      request->fetch_window_id, request->keepalive, request->is_reload,
-      request->is_history_navigation, request->devtools_stack_id);
+      request->request_initiator, request->referrer.Clone(),
+      request->credentials_mode, request->cache_mode, request->redirect_mode,
+      request->integrity, request->priority, request->fetch_window_id,
+      request->keepalive, request->is_reload, request->is_history_navigation,
+      request->devtools_stack_id);
 }
 
 }  // namespace content
diff --git a/content/common/fetch/fetch_request_type_converters.cc b/content/common/fetch/fetch_request_type_converters.cc
@@ -27,6 +27,7 @@ blink::mojom::FetchAPIRequestPtr TypeConverter<
   // nullptr.
   if (input.request_body)
     output->body = input.request_body;
+  output->request_initiator = input.request_initiator;
   output->referrer = blink::mojom::Referrer::New(
       input.referrer,
       blink::ReferrerUtils::NetToMojoReferrerPolicy(input.referrer_policy));

diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
@@ -563,6 +563,7 @@ URLRequest::URLRequest(const GURL& url,
       url_chain_(1, url),
       force_ignore_site_for_cookies_(false),
       force_ignore_top_frame_party_for_cookies_(false),
+      force_main_frame_for_same_site_cookies_(false),
       method_("GET"),
       referrer_policy_(
           ReferrerPolicy::CLEAR_ON_TRANSITION_FROM_SECURE_TO_INSECURE),

diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h
@@ -293,6 +293,16 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
     force_ignore_top_frame_party_for_cookies_ = force;
   }
 
+  // Indicates if the request should be treated as a main frame navigation for
+  // SameSite cookie computations.  This flag overrides the IsolationInfo
+  // request type associated with fetches from a service worker context.
+  bool force_main_frame_for_same_site_cookies() const {
+    return force_main_frame_for_same_site_cookies_;
+  }
+  void set_force_main_frame_for_same_site_cookies(bool value) {
+    force_main_frame_for_same_site_cookies_ = value;
+  }
+
   // The first-party URL policy to apply when updating the first party URL
   // during redirects. The first-party URL policy may only be changed before
   // Start() is called.
@@ -913,6 +923,7 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
 
   bool force_ignore_site_for_cookies_;
   bool force_ignore_top_frame_party_for_cookies_;
+  bool force_main_frame_for_same_site_cookies_;
   absl::optional<url::Origin> initiator_;
   GURL delegate_redirect_url_;
   std::string method_;  // "GET", "POST", etc. Should be all uppercase.

diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
@@ -597,8 +597,10 @@ void URLRequestHttpJob::AddCookieHeaderAndStart() {
                                                request_->site_for_cookies())) {
       force_ignore_site_for_cookies = true;
     }
-    bool is_main_frame_navigation = IsolationInfo::RequestType::kMainFrame ==
-                                    request_->isolation_info().request_type();
+    bool is_main_frame_navigation =
+        IsolationInfo::RequestType::kMainFrame ==
+            request_->isolation_info().request_type() ||
+        request_->force_main_frame_for_same_site_cookies();
     CookieOptions::SameSiteCookieContext same_site_context =
         net::cookie_util::ComputeSameSiteContextForRequest(
             request_->method(), request_->url_chain(),

diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
@@ -2490,6 +2490,31 @@ TEST_P(URLRequestSameSiteCookiesTest, SameSiteCookies) {
     EXPECT_EQ(0, network_delegate.blocked_set_cookie_count());
   }
 
+  // Verify that the lax cookie is sent for cross-site initiators when the
+  // method is "safe" and the request is being forced to be considered as a
+  // main frame navigation.
+  {
+    TestDelegate d;
+    std::unique_ptr<URLRequest> req(default_context().CreateRequest(
+        test_server.GetURL(kHost, "/echoheader?Cookie"), DEFAULT_PRIORITY, &d,
+        TRAFFIC_ANNOTATION_FOR_TESTS));
+    req->set_isolation_info(IsolationInfo::Create(
+        IsolationInfo::RequestType::kOther, kOrigin, kOrigin, kSiteForCookies,
+        {} /* party_context */));
+    req->set_site_for_cookies(kSiteForCookies);
+    req->set_initiator(kCrossOrigin);
+    req->set_method("GET");
+    req->set_force_main_frame_for_same_site_cookies(true);
+    req->Start();
+    d.RunUntilComplete();
+
+    EXPECT_EQ(std::string::npos,
+              d.data_received().find("StrictSameSiteCookie=1"));
+    EXPECT_NE(std::string::npos, d.data_received().find("LaxSameSiteCookie=1"));
+    EXPECT_EQ(0, network_delegate.blocked_annotate_cookies_count());
+    EXPECT_EQ(0, network_delegate.blocked_set_cookie_count());
+  }
+
   // Verify that neither cookie is sent for cross-site initiators when the
   // method is unsafe (e.g. POST), even if the request is a main frame
   // navigation.

diff --git a/services/network/BUILD.gn b/services/network/BUILD.gn
@@ -427,6 +427,7 @@ source_set("tests") {
     "//jingle/glue:fake_ssl_socket",
     "//mojo/public/cpp/bindings",
     "//mojo/public/cpp/system",
+    "//mojo/public/cpp/test_support:test_utils",
     "//net",
     "//net:extras",
     "//net:quic_test_tools",

diff --git a/services/network/cors/cors_url_loader.cc b/services/network/cors/cors_url_loader.cc
@@ -294,6 +294,18 @@ void CorsURLLoader::FollowRedirect(
     const net::HttpRequestHeaders& modified_headers,
     const net::HttpRequestHeaders& modified_cors_exempt_headers,
     const absl::optional<GURL>& new_url) {
+  // If this is a navigation from a renderer, then its a service worker
+  // passthrough of a navigation request.  Since this case uses manual
+  // redirect mode FollowRedirect() should never be called.
+  if (process_id_ != mojom::kBrowserProcessId &&
+      request_.mode == mojom::RequestMode::kNavigate) {
+    mojo::ReportBadMessage(
+        "CorsURLLoader: navigate from non-browser-process should not call "
+        "FollowRedirect");
+    HandleComplete(URLLoaderCompletionStatus(net::ERR_FAILED));
+    return;
+  }
+
   if (!network_loader_ || !deferred_redirect_url_) {
     HandleComplete(URLLoaderCompletionStatus(net::ERR_FAILED));
     return;

diff --git a/services/network/cors/cors_url_loader_factory.cc b/services/network/cors/cors_url_loader_factory.cc
@@ -383,18 +383,57 @@ bool CorsURLLoaderFactory::IsValidRequest(const ResourceRequest& request,
     return false;
   }
 
+  // The `force_main_frame_for_same_site_cookies` should only be set when a
+  // service worker passes through a navigation request.  In this case the
+  // mode must be `kNavigate` and the destination must be empty.
+  if (request.original_destination == mojom::RequestDestination::kDocument &&
+      (request.mode != mojom::RequestMode::kNavigate ||
+       request.destination != mojom::RequestDestination::kEmpty)) {
+    mojo::ReportBadMessage(
+        "CorsURLLoaderFactory: original_destination is unexpectedly set to "
+        "kDocument");
+    return false;
+  }
+
+  // By default we compare the `request_initiator` to the lock below.  This is
+  // overridden for renderer navigations, however.
+  absl::optional<url::Origin> origin_to_validate = request.request_initiator;
+
   // Ensure that renderer requests are covered either by CORS or CORB.
   if (process_id_ != mojom::kBrowserProcessId) {
     switch (request.mode) {
       case mojom::RequestMode::kNavigate:
-        // Only the browser process can initiate navigations.  This helps ensure
-        // that a malicious/compromised renderer cannot bypass CORB by issuing
-        // kNavigate, rather than kNoCors requests.  (CORB should apply only to
-        // no-cors requests as tracked in https://crbug.com/953315 and as
-        // captured in https://fetch.spec.whatwg.org/#main-fetch).
-        mojo::ReportBadMessage(
-            "CorsURLLoaderFactory: navigate from non-browser-process");
-        return false;
+        // A navigation request from a renderer can legally occur when a service
+        // worker passes it through from its `FetchEvent.request` to `fetch()`.
+        // In this case it is making a navigation request on behalf of the
+        // original initiator.  Since that initiator may be cross-origin, its
+        // possible the request's initiator will not match our lock.
+        //
+        // To make this operation safe we instead compare the request URL origin
+        // against the initiator lock.  We can do this since service workers
+        // should only ever handle same-origin navigations.
+        //
+        // With this approach its possible the initiator could be spoofed by the
+        // renderer.  However, since we have validated the request URL they can
+        // only every lie to the origin that they have already compromised.  It
+        // does not allow an attacker to target other arbitrary origins.
+        origin_to_validate = url::Origin::Create(request.url);
+
+        // We further validate the navigation request by ensuring it has the
+        // correct redirect mode.  This avoids an attacker attempting to
+        // craft a navigation that is then automatically followed to a separate
+        // target origin.  With manual mode the redirect will instead be
+        // processed as an opaque redirect response that is passed back to the
+        // renderer and navigation code.  The redirected requested must be
+        // sent anew and go through this validation again.
+        if (request.redirect_mode != mojom::RedirectMode::kManual) {
+          mojo::ReportBadMessage(
+              "CorsURLLoaderFactory: navigate from non-browser-process with "
+              "redirect_mode set to 'follow'");
+          return false;
+        }
+
+        break;
 
       case mojom::RequestMode::kSameOrigin:
       case mojom::RequestMode::kCors:
@@ -408,11 +447,11 @@ bool CorsURLLoaderFactory::IsValidRequest(const ResourceRequest& request,
     }
   }
 
-  // Compare |request_initiator| and |request_initiator_origin_lock_|.
+  // Depending on the type of request, compare either `request_initiator` or
+  // `request.url` to `request_initiator_origin_lock_`.
   InitiatorLockCompatibility initiator_lock_compatibility =
-      VerifyRequestInitiatorLockWithPluginCheck(process_id_,
-                                                request_initiator_origin_lock_,
-                                                request.request_initiator);
+      VerifyRequestInitiatorLockWithPluginCheck(
+          process_id_, request_initiator_origin_lock_, origin_to_validate);
   UMA_HISTOGRAM_ENUMERATION(
       "NetworkService.URLLoader.RequestInitiatorOriginLockCompatibility",
       initiator_lock_compatibility);
@@ -440,7 +479,6 @@ bool CorsURLLoaderFactory::IsValidRequest(const ResourceRequest& request,
 
     case InitiatorLockCompatibility::kIncorrectLock:
       // Requests from the renderer need to always specify a correct initiator.
-      NOTREACHED();
       mojo::ReportBadMessage(
           "CorsURLLoaderFactory: lock VS initiator mismatch");
       return false;

diff --git a/services/network/cors/cors_url_loader_factory_unittest.cc b/services/network/cors/cors_url_loader_factory_unittest.cc
@@ -7,6 +7,8 @@
 #include "base/macros.h"
 #include "base/test/task_environment.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "mojo/public/cpp/test_support/fake_message_dispatch_context.h"
+#include "mojo/public/cpp/test_support/test_utils.h"
 #include "net/base/load_flags.h"
 #include "net/proxy_resolution/configured_proxy_resolution_service.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -111,6 +113,7 @@ class CorsURLLoaderFactoryTest : public testing::Test {
  private:
   // Test environment.
   base::test::TaskEnvironment task_environment_;
+  mojo::FakeMessageDispatchContext mojo_context_;
   std::unique_ptr<net::URLRequestContext> url_request_context_;
   ResourceScheduler resource_scheduler_;
   std::unique_ptr<NetworkService> network_service_;
@@ -190,5 +193,77 @@ TEST_F(CorsURLLoaderFactoryTest, CleanupWithSharedCacheObjectInUse) {
   ResetFactory();
 }
 
+TEST_F(CorsURLLoaderFactoryTest,
+       NavigationFromRendererWithBadRequestURLOrigin) {
+  ResourceRequest request;
+  GURL url = test_server()->GetURL("/echoall");
+  request.mode = mojom::RequestMode::kNavigate;
+  request.redirect_mode = mojom::RedirectMode::kManual;
+  request.destination = mojom::RequestDestination::kEmpty;
+  request.method = net::HttpRequestHeaders::kPostMethod;
+  request.url = GURL("https://some.other.origin/echoall");
+  request.request_initiator = url::Origin::Create(url);
+  mojo::test::BadMessageObserver bad_message_observer;
+  CreateLoaderAndStart(request);
+  EXPECT_EQ("CorsURLLoaderFactory: lock VS initiator mismatch",
+            bad_message_observer.WaitForBadMessage());
+}
+
+TEST_F(CorsURLLoaderFactoryTest, NavigationFromRendererWithBadRedirectMode) {
+  ResourceRequest request;
+  GURL url = test_server()->GetURL("/echoall");
+  request.mode = mojom::RequestMode::kNavigate;
+  request.redirect_mode = mojom::RedirectMode::kFollow;
+  request.destination = mojom::RequestDestination::kEmpty;
+  request.method = net::HttpRequestHeaders::kPostMethod;
+  request.url = url;
+  request.request_initiator = url::Origin::Create(url).DeriveNewOpaqueOrigin();
+  mojo::test::BadMessageObserver bad_message_observer;
+  CreateLoaderAndStart(request);
+  EXPECT_EQ(
+      "CorsURLLoaderFactory: navigate from non-browser-process with "
+      "redirect_mode set to 'follow'",
+      bad_message_observer.WaitForBadMessage());
+}
+
+TEST_F(CorsURLLoaderFactoryTest, OriginalDestinationIsDocumentWithBadMode) {
+  ResourceRequest request;
+  GURL url = test_server()->GetURL("/echoall");
+  request.mode = mojom::RequestMode::kCors;
+  request.redirect_mode = mojom::RedirectMode::kFollow;
+  request.destination = mojom::RequestDestination::kEmpty;
+  request.method = net::HttpRequestHeaders::kGetMethod;
+  request.url = url;
+  request.request_initiator =
+      url::Origin::Create(GURL("https://some.other.origin"));
+  request.original_destination = mojom::RequestDestination::kDocument;
+  mojo::test::BadMessageObserver bad_message_observer;
+  CreateLoaderAndStart(request);
+  EXPECT_EQ(
+      "CorsURLLoaderFactory: original_destination is unexpectedly set to "
+      "kDocument",
+      bad_message_observer.WaitForBadMessage());
+}
+
+TEST_F(CorsURLLoaderFactoryTest,
+       OriginalDestinationIsDocumentWithBadDestination) {
+  ResourceRequest request;
+  GURL url = test_server()->GetURL("/echoall");
+  request.mode = mojom::RequestMode::kNavigate;
+  request.redirect_mode = mojom::RedirectMode::kManual;
+  request.destination = mojom::RequestDestination::kIframe;
+  request.method = net::HttpRequestHeaders::kGetMethod;
+  request.url = url;
+  request.request_initiator =
+      url::Origin::Create(GURL("https://some.other.origin"));
+  request.original_destination = mojom::RequestDestination::kDocument;
+  mojo::test::BadMessageObserver bad_message_observer;
+  CreateLoaderAndStart(request);
+  EXPECT_EQ(
+      "CorsURLLoaderFactory: original_destination is unexpectedly set to "
+      "kDocument",
+      bad_message_observer.WaitForBadMessage());
+}
+
 }  // namespace cors
 }  // namespace network
diff --git a/services/network/cors/cors_url_loader_unittest.cc b/services/network/cors/cors_url_loader_unittest.cc
@@ -207,12 +207,16 @@ class CorsURLLoaderTest : public testing::Test {
   }
   void SetUp() override { SetUp(mojom::NetworkContextParams::New()); }
 
-  void CreateLoaderAndStart(const GURL& origin,
-                            const GURL& url,
-                            mojom::RequestMode mode) {
+  void CreateLoaderAndStart(
+      const GURL& origin,
+      const GURL& url,
+      mojom::RequestMode mode,
+      mojom::RedirectMode redirect_mode = mojom::RedirectMode::kFollow,
+      mojom::CredentialsMode credentials_mode = mojom::CredentialsMode::kOmit) {
     ResourceRequest request;
     request.mode = mode;
-    request.credentials_mode = mojom::CredentialsMode::kOmit;
+    request.redirect_mode = redirect_mode;
+    request.credentials_mode = credentials_mode;
     request.method = net::HttpRequestHeaders::kGetMethod;
     request.url = url;
     request.request_initiator = url::Origin::Create(origin);
@@ -702,9 +706,13 @@ TEST_F(CorsURLLoaderTest, NavigateWithEarlyHints) {
 }
 
 TEST_F(CorsURLLoaderTest, NavigationFromRenderer) {
+  ResetFactory(url::Origin::Create(GURL("https://example.com/")),
+               kRendererProcessId);
+
   ResourceRequest request;
   request.mode = mojom::RequestMode::kNavigate;
-  request.url = GURL("https://example.com/");
+  request.redirect_mode = mojom::RedirectMode::kManual;
+  request.url = GURL("https://some.other.example.com/");
   request.request_initiator = absl::nullopt;
 
   BadMessageTestHelper bad_message_helper;
@@ -718,7 +726,7 @@ TEST_F(CorsURLLoaderTest, NavigationFromRenderer) {
   EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
   EXPECT_THAT(bad_message_helper.bad_message_reports(),
               ::testing::ElementsAre(
-                  "CorsURLLoaderFactory: navigate from non-browser-process"));
+                  "CorsURLLoaderFactory: lock VS initiator mismatch"));
 }
 
 TEST_F(CorsURLLoaderTest, SameOriginRequest) {
@@ -2875,6 +2883,39 @@ TEST_F(CorsURLLoaderTest, PreflightMissingAllowOrigin) {
                   mojom::CorsError::kPreflightMissingAllowOriginHeader)));
 }
 
+TEST_F(CorsURLLoaderTest, NonBrowserNavigationRedirect) {
+  BadMessageTestHelper bad_message_helper;
+
+  const GURL origin("https://example.com");
+  const GURL url("https://example.com/foo.png");
+  const GURL new_url("https://example.com/bar.png");
+
+  CreateLoaderAndStart(origin, url, mojom::RequestMode::kNavigate,
+                       mojom::RedirectMode::kManual,
+                       mojom::CredentialsMode::kInclude);
+  RunUntilCreateLoaderAndStartCalled();
+
+  EXPECT_EQ(1, num_created_loaders());
+  EXPECT_EQ(GetRequest().url, url);
+  EXPECT_EQ(GetRequest().method, "GET");
+
+  NotifyLoaderClientOnReceiveRedirect(CreateRedirectInfo(301, "GET", new_url));
+  RunUntilRedirectReceived();
+
+  EXPECT_TRUE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_completion());
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_redirect());
+
+  FollowRedirect();
+
+  RunUntilComplete();
+  EXPECT_THAT(
+      bad_message_helper.bad_message_reports(),
+      ::testing::ElementsAre("CorsURLLoader: navigate from non-browser-process "
+                             "should not call FollowRedirect"));
+}
+
 }  // namespace
 
 }  // namespace cors