[Backport] CVE-2022-0111 and CVE-2022-0117 (1/2)

Manual backprort of patch for
"CVE-2022-0111: Inappropriate implementation in Navigation." and
"CVE-2022-0117: Policy bypass in Service Workers", originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/3251368:
Reland "Fetch: Plumb request initiator through passthrough service workers."

This is a reland of da0a6501cf321579bd46a27ff9fba1bb8ea910bb

This CL also includes a change to mark the two WPT tests as requiring
long timeout durations.  On my fast build machine with an opt build
they take ~5 seconds each to complete and the default timeout is 10
seconds.  On slower bots with debug builds its highly likely that these
tests would be marked as timing out.  This change gives them a 60 second
timeout instead.

Original change's description:
> 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 <mmenke@chromium.org>
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
> Commit-Queue: Ben Kelly <wanderview@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#936029}

Bug: 1115847,1241188
Change-Id: Ia26acbdd0d7ce6583d9a44f83ed086708657b8bd
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Auto-Submit: Ben Kelly <wanderview@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936560}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

chromium/content/common/background_fetch/background_fetch_types.cc

@@ -52,10 +52,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

chromium/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));

chromium/net/url_request/url_request.cc

@@ -545,6 +545,7 @@ URLRequest::URLRequest(const GURL& url,
                                       NetLogSourceType::URL_REQUEST)),
       url_chain_(1, url),
       force_ignore_site_for_cookies_(false),
+      force_main_frame_for_same_site_cookies_(false),
       method_("GET"),
       referrer_policy_(
           ReferrerPolicy::CLEAR_ON_TRANSITION_FROM_SECURE_TO_INSECURE),

chromium/net/url_request/url_request.h

@@ -267,6 +267,16 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
     force_ignore_site_for_cookies_ = attach;
   }
 
+  // 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.
@@ -856,6 +866,7 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
   IsolationInfo isolation_info_;
 
   bool force_ignore_site_for_cookies_;
+  bool force_main_frame_for_same_site_cookies_;
   base::Optional<url::Origin> initiator_;
   GURL delegate_redirect_url_;
   std::string method_;  // "GET", "POST", etc. Should be all uppercase.

chromium/net/url_request/url_request_http_job.cc

@@ -549,8 +549,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(), request_->site_for_cookies(),

chromium/services/network/BUILD.gn

@@ -403,6 +403,7 @@ source_set("tests") {
     "//jingle: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",

chromium/services/network/cors/cors_url_loader.cc

@@ -127,6 +127,18 @@ void CorsURLLoader::FollowRedirect(
     const net::HttpRequestHeaders& modified_headers,
     const net::HttpRequestHeaders& modified_cors_exempt_headers,
     const base::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;

chromium/services/network/cors/cors_url_loader_factory.cc

@@ -380,18 +380,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.
+  base::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:
@@ -405,11 +444,12 @@ 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);
@@ -437,7 +477,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;

chromium/services/network/public/cpp/resource_request.h

@@ -132,6 +132,8 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceRequest {
   mojom::RedirectMode redirect_mode = mojom::RedirectMode::kFollow;
   std::string fetch_integrity;
   mojom::RequestDestination destination = mojom::RequestDestination::kEmpty;
+  mojom::RequestDestination original_destination =
+      mojom::RequestDestination::kEmpty;
   scoped_refptr<ResourceRequestBody> request_body;
   bool keepalive = false;
   bool has_user_gesture = false;

chromium/services/network/public/cpp/url_request_mojom_traits.cc

@@ -264,6 +264,7 @@ bool StructTraits<
   out->is_fetch_like_api = data.is_fetch_like_api();
   out->is_favicon = data.is_favicon();
   out->obey_origin_policy = data.obey_origin_policy();
+  out->original_destination = data.original_destination();
   return true;
 }
 

chromium/services/network/public/cpp/url_request_mojom_traits.h

@@ -294,6 +294,10 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE)
   static bool obey_origin_policy(const network::ResourceRequest& request) {
     return request.obey_origin_policy;
   }
+  static network::mojom::RequestDestination original_destination(
+      const network::ResourceRequest& request) {
+    return request.original_destination;
+  }
   static const base::Optional<network::ResourceRequest::TrustedParams>&
   trusted_params(const network::ResourceRequest& request) {
     return request.trusted_params;

chromium/services/network/public/mojom/url_loader.mojom

@@ -397,6 +397,10 @@ struct URLRequest {
   // Spec: https://wicg.github.io/origin-policy/
   bool obey_origin_policy;
 
+  // The original destination of a request that was passed through by a service
+  // worker.
+  RequestDestination original_destination;
+
   // Setting these from an untrusted URLLoader will cause the request to fail.
   TrustedUrlRequestParams? trusted_params;
 

chromium/services/network/url_loader.cc

@@ -567,6 +567,18 @@ URLLoader::URLLoader(
   if (require_network_isolation_key)
     DCHECK(!url_request_->isolation_info().IsEmpty());
 
+  // When a service worker forwards a navigation request it uses the
+  // service worker's IsolationInfo.  This causes the cookie code to fail
+  // to send SameSite=Lax cookies for main-frame navigations passed through
+  // a service worker.  To fix this we check to see if the original destination
+  // of the request was a main frame document and then set a flag indicating
+  // SameSite cookies should treat it as a main frame navigation.
+  if (request.mode == mojom::RequestMode::kNavigate &&
+      request.destination == mojom::RequestDestination::kEmpty &&
+      request.original_destination == mojom::RequestDestination::kDocument) {
+    url_request_->set_force_main_frame_for_same_site_cookies(true);
+  }
+
   if (factory_params_->disable_secure_dns) {
     url_request_->SetDisableSecureDns(true);
   } else if (request.trusted_params) {

chromium/third_party/blink/public/mojom/fetch/fetch_api_request.mojom

@@ -13,6 +13,7 @@ import "services/network/public/mojom/url_loader.mojom";
 import "third_party/blink/public/mojom/blob/serialized_blob.mojom";
 import "third_party/blink/public/mojom/loader/request_context_frame_type.mojom";
 import "third_party/blink/public/mojom/loader/referrer.mojom";
+import "url/mojom/origin.mojom";
 import "url/mojom/url.mojom";
 
 
@@ -162,6 +163,14 @@ struct FetchAPIRequest {
   SerializedBlob? blob;
   FetchAPIRequestBody? body;
 
+  // `request_initiator` indicates the origin that initiated the request.  See
+  // also `network::ResourceRequest::request_initiator`, and the doc comment
+  // for `request_initiator` in services/network/public/mojom/url_request.mojom.
+  //
+  // Note that the origin may be missing for browser-initiated navigations
+  // (e.g. ones initiated from the Omnibox).
+  url.mojom.Origin? request_initiator;
+
   Referrer? referrer;
   network.mojom.CredentialsMode credentials_mode =
     network.mojom.CredentialsMode.kOmit;

chromium/third_party/blink/renderer/core/fetch/fetch_manager.cc

@@ -679,22 +679,7 @@ void FetchManager::Loader::PerformHTTPFetch() {
   request.SetHttpMethod(fetch_request_data_->Method());
   request.SetFetchWindowId(fetch_request_data_->WindowId());
   request.SetTrustTokenParams(fetch_request_data_->TrustTokenParams());
-
-  switch (fetch_request_data_->Mode()) {
-    case RequestMode::kSameOrigin:
-    case RequestMode::kNoCors:
-    case RequestMode::kCors:
-    case RequestMode::kCorsWithForcedPreflight:
-      request.SetMode(fetch_request_data_->Mode());
-      break;
-    case RequestMode::kNavigate:
-      // NetworkService (i.e. CorsURLLoaderFactory::IsSane) rejects kNavigate
-      // requests coming from renderers, so using kSameOrigin here.
-      // TODO(lukasza): Tweak CorsURLLoaderFactory::IsSane to accept kNavigate
-      // if request_initiator and the target are same-origin.
-      request.SetMode(RequestMode::kSameOrigin);
-      break;
-  }
+  request.SetMode(fetch_request_data_->Mode());
 
   request.SetCredentialsMode(fetch_request_data_->Credentials());
   for (const auto& header : fetch_request_data_->HeaderList()->List()) {
@@ -741,6 +726,7 @@ void FetchManager::Loader::PerformHTTPFetch() {
     request.SetKeepalive(true);
     UseCounter::Count(execution_context_, mojom::WebFeature::kFetchKeepalive);
   }
+  request.SetOriginalDestination(fetch_request_data_->OriginalDestination());
 
   // "3. Append `Host`, ..."
   // FIXME: Implement this when the spec is fixed.

chromium/third_party/blink/renderer/core/fetch/fetch_request_data.cc

@@ -169,6 +169,8 @@ FetchRequestData* FetchRequestData::Create(
   // we deprecate SetContext.
 
   request->SetDestination(fetch_api_request->destination);
+  if (fetch_api_request->request_initiator)
+    request->SetOrigin(fetch_api_request->request_initiator);
   request->SetReferrerString(AtomicString(Referrer::NoReferrer()));
   if (fetch_api_request->referrer) {
     if (!fetch_api_request->referrer->url.IsEmpty()) {
@@ -189,6 +191,7 @@ FetchRequestData* FetchRequestData::Create(
       fetch_api_request->priority));
   if (fetch_api_request->fetch_window_id)
     request->SetWindowId(fetch_api_request->fetch_window_id.value());
+
   return request;
 }
 
@@ -210,6 +213,7 @@ FetchRequestData* FetchRequestData::CloneExceptBody() {
   request->integrity_ = integrity_;
   request->priority_ = priority_;
   request->importance_ = importance_;
+  request->original_destination_ = original_destination_;
   request->keepalive_ = keepalive_;
   request->is_history_navigation_ = is_history_navigation_;
   request->window_id_ = window_id_;

chromium/third_party/blink/renderer/core/fetch/fetch_request_data.h

@@ -106,6 +106,13 @@ class CORE_EXPORT FetchRequestData final
   void SetIntegrity(const String& integrity) { integrity_ = integrity; }
   ResourceLoadPriority Priority() const { return priority_; }
   void SetPriority(ResourceLoadPriority priority) { priority_ = priority; }
+  // The original destination of a request passed through by a service worker.
+  void SetOriginalDestination(network::mojom::RequestDestination value) {
+    original_destination_ = value;
+  }
+  network::mojom::RequestDestination OriginalDestination() const {
+    return original_destination_;
+  }
   bool Keepalive() const { return keepalive_; }
   void SetKeepalive(bool b) { keepalive_ = b; }
   bool IsHistoryNavigation() const { return is_history_navigation_; }
@@ -172,6 +179,8 @@ class CORE_EXPORT FetchRequestData final
   String mime_type_;
   String integrity_;
   ResourceLoadPriority priority_;
+  network::mojom::RequestDestination original_destination_ =
+      network::mojom::RequestDestination::kEmpty;
   bool keepalive_;
   bool is_history_navigation_ = false;
   // A specific factory that should be used for this request instead of whatever

chromium/third_party/blink/renderer/core/fetch/request.cc

@@ -71,7 +71,8 @@ FetchRequestData* CreateCopyOfFetchRequestDataForFetch(
   request->SetURL(original->Url());
   request->SetMethod(original->Method());
   request->SetHeaderList(original->HeaderList()->Clone());
-  request->SetOrigin(context->GetSecurityOrigin());
+  request->SetOrigin(original->Origin() ? original->Origin()
+                                        : context->GetSecurityOrigin());
   // FIXME: Set client.
   DOMWrapperWorld& world = script_state->World();
   if (world.IsIsolatedWorld()) {
@@ -98,6 +99,18 @@ FetchRequestData* CreateCopyOfFetchRequestDataForFetch(
   }
   request->SetWindowId(original->WindowId());
   request->SetTrustTokenParams(original->TrustTokenParams());
+
+  // When a new request is created from another the destination is always reset
+  // to be `kEmpty`.  In order to facilitate some later checks when a service
+  // worker forwards a navigation request we want to keep track of the
+  // destination of the original request.  Therefore record the original
+  // request's destination if its non-empty, otherwise just carry forward
+  // whatever "original destination" value was already set.
+  if (original->Destination() != network::mojom::RequestDestination::kEmpty)
+    request->SetOriginalDestination(original->Destination());
+  else
+    request->SetOriginalDestination(original->OriginalDestination());
+
   return request;
 }
 
@@ -312,6 +325,9 @@ Request* Request::CreateRequestWithRequestOrString(
 
   // "If any of |init|'s members are present, then:"
   if (AreAnyMembersPresent(init)) {
+    request->SetOrigin(execution_context->GetSecurityOrigin());
+    request->SetOriginalDestination(network::mojom::RequestDestination::kEmpty);
+
     // "If |request|'s |mode| is "navigate", then set it to "same-origin".
     if (request->Mode() == network::mojom::RequestMode::kNavigate)
       request->SetMode(network::mojom::RequestMode::kSameOrigin);
@@ -967,6 +983,7 @@ mojom::blink::FetchAPIRequestPtr Request::CreateFetchAPIRequest() const {
   fetch_api_request->integrity = request_->Integrity();
   fetch_api_request->is_history_navigation = request_->IsHistoryNavigation();
   fetch_api_request->destination = request_->Destination();
+  fetch_api_request->request_initiator = request_->Origin();
 
   // Strip off the fragment part of URL. So far, all callers expect the fragment
   // to be excluded.