0

request_initiator_origin_lock checks in SignedExchangeURLLoaderFactory.

This CL replicates the functionality of GetTrustworthyInitiator
underneath SignedExchangeURLLoaderFactory.  This is sufficient to
enable removal of GetTrustworthyInitiator in a follow-up CL.

In the long-term a mismatched `request_initiator_origin_lock` should
result in erroring out the resource request, but this is postponed to a
follow-up CL later.  We should pursue this if the DumpWithoutCrashing
call from the current CL doesn't result in a high volume of reports.

Bug: 1217825
Change-Id: I95f5d834d9097e13049a3ad022fe90fbdefe5aee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2923326
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892142}
This commit is contained in:
Lukasz Anforowicz
2021-06-14 17:35:12 +00:00
committed by Chromium LUCI CQ
parent 680d2b2741
commit 759fcff71e
9 changed files with 187 additions and 105 deletions

@ -7,6 +7,7 @@
#include "base/base64.h"
#include "base/feature_list.h"
#include "base/metrics/histogram_macros.h"
#include "base/notreached.h"
#include "base/strings/string_util.h"
#include "components/link_header_util/link_header_util.h"
#include "content/browser/loader/cross_origin_read_blocking_checker.h"
@ -29,6 +30,7 @@
#include "services/network/public/cpp/constants.h"
#include "services/network/public/cpp/cors/cors.h"
#include "services/network/public/cpp/features.h"
#include "services/network/public/cpp/initiator_lock_compatibility.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "services/network/public/cpp/url_loader_completion_status.h"
#include "services/network/public/cpp/wrapper_shared_url_loader_factory.h"
@ -57,6 +59,47 @@ void UpdateRequestResponseStartTime(
response_head->load_timing.request_start = now_ticks;
}
bool IsValidRequestInitiator(const network::ResourceRequest& request,
const url::Origin& request_initiator_origin_lock) {
// TODO(lukasza): Deduplicate the check below by reusing parts of
// CorsURLLoaderFactory::IsValidRequest (potentially also reusing the parts
// that validate non-initiator-related parts of a ResourceRequest)..
network::InitiatorLockCompatibility initiator_lock_compatibility =
network::VerifyRequestInitiatorLock(request_initiator_origin_lock,
request.request_initiator);
switch (initiator_lock_compatibility) {
case network::InitiatorLockCompatibility::kBrowserProcess:
case network::InitiatorLockCompatibility::kAllowedRequestInitiatorForPlugin:
// kBrowserProcess and kAllowedRequestInitiatorForPlugin cannot happen
// outside of NetworkService.
NOTREACHED();
return false;
case network::InitiatorLockCompatibility::kNoLock:
case network::InitiatorLockCompatibility::kNoInitiator:
// Only browser-initiated navigations can specify no initiator and we only
// expect subresource requests (i.e. non-navigations) to go through
// SubresourceSignedExchangeURLLoaderFactory::CreateLoaderAndStart.
NOTREACHED();
return false;
case network::InitiatorLockCompatibility::kCompatibleLock:
return true;
case network::InitiatorLockCompatibility::kIncorrectLock:
// This branch indicates that either 1) the CreateLoaderAndStart IPC was
// forged by a malicious/compromised renderer process or 2) there are
// renderer-side bugs.
NOTREACHED();
return false;
}
// Failing safely for an unrecognied `network::InitiatorLockCompatibility`
// enum value.
NOTREACHED();
return false;
}
// A utility subclass of MojoBlobReader::Delegate that calls the passed callback
// in OnComplete().
class MojoBlobReaderDelegate : public storage::MojoBlobReader::Delegate {
@ -143,6 +186,14 @@ class InnerResponseURLLoader : public network::mojom::URLLoader {
completion_status_(completion_status),
client_(std::move(client)) {
DCHECK(response_->headers);
// The `request.request_initiator` is assumed to be present and trustworthy
// - it comes either from:
// 1, The trustworthy navigation stack (via
// PrefetchedNavigationLoaderInterceptor::StartInnerResponse).
// or
// 2. SubresourceSignedExchangeURLLoaderFactory::CreateLoaderAndStart which
// validates the untrustworthy IPC payload as its very first action.
DCHECK(request.request_initiator);
// Keep the SSLInfo only when the request is for main frame main resource,
@ -178,6 +229,11 @@ class InnerResponseURLLoader : public network::mojom::URLLoader {
}
}
// TODO(https://crbug.com/1217825): Stop passing
// `request_initiator_origin_lock` below (it is only required for the
// deprecated GetTrustworthyInitiator function used by
// CrossOriginReadBlockingChecker + the request_initiator is already
// trustworthy as documented in a comment above).
corb_checker_ = std::make_unique<CrossOriginReadBlockingChecker>(
request, *response_, request_initiator_origin_lock, *blob_data_handle_,
base::BindOnce(
@ -357,6 +413,21 @@ class SubresourceSignedExchangeURLLoaderFactory
mojo::PendingRemote<network::mojom::URLLoaderClient> client,
const net::MutableNetworkTrafficAnnotationTag& traffic_annotation)
override {
if (!IsValidRequestInitiator(request, request_initiator_origin_lock_)) {
NOTREACHED();
network::debug::ScopedResourceRequestCrashKeys request_crash_keys(
request);
network::debug::ScopedRequestInitiatorOriginLockCrashKey lock_crash_keys(
request_initiator_origin_lock_);
mojo::ReportBadMessage(
"SubresourceSignedExchangeURLLoaderFactory: "
"lock VS initiator mismatch");
mojo::Remote<network::mojom::URLLoaderClient>(std::move(client))
->OnComplete(
network::URLLoaderCompletionStatus(net::ERR_INVALID_ARGUMENT));
return;
}
DCHECK_EQ(request.url, entry_->inner_url());
mojo::MakeSelfOwnedReceiver(
std::make_unique<InnerResponseURLLoader>(
@ -458,10 +529,27 @@ class PrefetchedNavigationLoaderInterceptor
const network::ResourceRequest& resource_request,
mojo::PendingReceiver<network::mojom::URLLoader> receiver,
mojo::PendingRemote<network::mojom::URLLoaderClient> client) {
// `resource_request.request_initiator()` is trustworthy, because:
// 1) StartInnerResponse is only used from the navigation stack (via
// MaybeCreateLoader override of NavigationLoaderInterceptor)
// 2) navigation initiator is validated in IPCs from the renderer (e.g. see
// VerifyBeginNavigationCommonParams).
// Note that `request_initiator_origin_lock` below might be different from
// `url::Origin::Create(exchange_->inner_url())` - for example in the
// All/SignedExchangeRequestHandlerBrowserTest.Simple/3 testcase.
CHECK_EQ(network::mojom::RequestMode::kNavigate, resource_request.mode);
// PrefetchedNavigationLoaderInterceptor is only created for
// renderer-initiated navigations - therefore `request_initiator` is
// guaranteed to have a value here.
CHECK(resource_request.request_initiator.has_value());
url::Origin request_initiator_origin_lock =
resource_request.request_initiator.value();
mojo::MakeSelfOwnedReceiver(
std::make_unique<InnerResponseURLLoader>(
resource_request, exchange_->inner_response().Clone(),
url::Origin::Create(exchange_->inner_url()),
request_initiator_origin_lock,
std::make_unique<const storage::BlobDataHandle>(
*exchange_->blob_data_handle()),
*exchange_->completion_status(), std::move(client),
@ -470,7 +558,7 @@ class PrefetchedNavigationLoaderInterceptor
}
State state_ = State::kInitial;
std::unique_ptr<const PrefetchedSignedExchangeCacheEntry> exchange_;
const std::unique_ptr<const PrefetchedSignedExchangeCacheEntry> exchange_;
std::vector<mojom::PrefetchedSignedExchangeInfoPtr> info_list_;
base::WeakPtrFactory<PrefetchedNavigationLoaderInterceptor> weak_factory_{

@ -30,8 +30,6 @@ component("network_service") {
"cors/preflight_controller.h",
"cors/preflight_result.cc",
"cors/preflight_result.h",
"crash_keys.cc",
"crash_keys.h",
"crl_set_distributor.cc",
"crl_set_distributor.h",
"data_pipe_element_reader.cc",

@ -7,12 +7,9 @@
#include <utility>
#include "base/bind.h"
#include "base/debug/crash_logging.h"
#include "base/debug/dump_without_crashing.h"
#include "base/logging.h"
#include "base/metrics/histogram_macros.h"
#include "base/stl_util.h"
#include "base/types/strong_alias.h"
#include "mojo/public/cpp/bindings/message.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "net/base/load_flags.h"
@ -20,7 +17,6 @@
#include "net/http/http_util.h"
#include "services/network/cors/cors_url_loader.h"
#include "services/network/cors/preflight_controller.h"
#include "services/network/crash_keys.h"
#include "services/network/network_context.h"
#include "services/network/network_service.h"
#include "services/network/public/cpp/cors/cors.h"
@ -245,7 +241,7 @@ void CorsURLLoaderFactory::CreateLoaderAndStart(
const ResourceRequest& resource_request,
mojo::PendingRemote<mojom::URLLoaderClient> client,
const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
debug::ScopedRequestCrashKeys request_crash_keys(resource_request);
debug::ScopedResourceRequestCrashKeys request_crash_keys(resource_request);
if (!IsValidRequest(resource_request, options)) {
mojo::Remote<mojom::URLLoaderClient>(std::move(client))

@ -1,54 +0,0 @@
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "services/network/crash_keys.h"
#include "base/stl_util.h"
#include "base/strings/string_number_conversions.h"
#include "services/network/public/cpp/resource_request.h"
#include "url/gurl.h"
namespace network {
namespace debug {
namespace {
base::debug::CrashKeyString* GetRequestUrlCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_url", base::debug::CrashKeySize::Size256);
return crash_key;
}
base::debug::CrashKeyString* GetRequestInitiatorCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_initiator", base::debug::CrashKeySize::Size64);
return crash_key;
}
base::debug::CrashKeyString* GetRequestResourceTypeCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_resource_type", base::debug::CrashKeySize::Size32);
return crash_key;
}
} // namespace
base::debug::CrashKeyString* GetRequestInitiatorOriginLockCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_initiator_origin_lock", base::debug::CrashKeySize::Size64);
return crash_key;
}
ScopedRequestCrashKeys::ScopedRequestCrashKeys(
const network::ResourceRequest& request)
: url_(GetRequestUrlCrashKey(), request.url.possibly_invalid_spec()),
request_initiator_(GetRequestInitiatorCrashKey(),
base::OptionalOrNullptr(request.request_initiator)),
resource_type_(GetRequestResourceTypeCrashKey(),
base::NumberToString(request.resource_type)) {}
ScopedRequestCrashKeys::~ScopedRequestCrashKeys() = default;
} // namespace debug
} // namespace network

@ -1,38 +0,0 @@
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef SERVICES_NETWORK_CRASH_KEYS_H_
#define SERVICES_NETWORK_CRASH_KEYS_H_
#include "base/debug/crash_logging.h"
#include "url/origin.h"
namespace network {
struct ResourceRequest;
namespace debug {
// TODO(lukasza): Move to //services/network/public/cpp to enable reuse outside
// of //services/network (e.g. see https://crrev.com/c/2923326).
base::debug::CrashKeyString* GetRequestInitiatorOriginLockCrashKey();
class ScopedRequestCrashKeys {
public:
ScopedRequestCrashKeys(const network::ResourceRequest& request);
~ScopedRequestCrashKeys();
ScopedRequestCrashKeys(const ScopedRequestCrashKeys&) = delete;
ScopedRequestCrashKeys& operator=(const ScopedRequestCrashKeys&) = delete;
private:
base::debug::ScopedCrashKeyString url_;
url::debug::ScopedOriginCrashKey request_initiator_;
base::debug::ScopedCrashKeyString resource_type_;
};
} // namespace debug
} // namespace network
#endif // SERVICES_NETWORK_CRASH_KEYS_H_

@ -17,6 +17,16 @@
namespace network {
namespace {
base::debug::CrashKeyString* GetRequestInitiatorOriginLockCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_initiator_origin_lock", base::debug::CrashKeySize::Size64);
return crash_key;
}
} // namespace
InitiatorLockCompatibility VerifyRequestInitiatorLock(
const absl::optional<url::Origin>& request_initiator_origin_lock,
const absl::optional<url::Origin>& request_initiator) {
@ -64,4 +74,17 @@ url::Origin GetTrustworthyInitiator(
return request_initiator.value();
}
namespace debug {
ScopedRequestInitiatorOriginLockCrashKey::
ScopedRequestInitiatorOriginLockCrashKey(
const absl::optional<url::Origin>& request_initiator_origin_lock)
: ScopedOriginCrashKey(
GetRequestInitiatorOriginLockCrashKey(),
base::OptionalOrNullptr(request_initiator_origin_lock)) {}
ScopedRequestInitiatorOriginLockCrashKey::
~ScopedRequestInitiatorOriginLockCrashKey() = default;
} // namespace debug
} // namespace network

@ -73,15 +73,31 @@ InitiatorLockCompatibility VerifyRequestInitiatorLock(
// network::ResourceRequest::request_initiator which may be initially set in an
// untrustworthy process (eg: renderer process).
//
// TODO(lukasza): Remove this function if https://crrev.com/c/1661114 sticks
// (i.e. if ResourceRequest::request_initiator is sanitized and made trustworthy
// by CorsURLLoaderFactory::CreateLoaderAndStart and IsValidRequest). Once we
// remove this, this header can be moved to non-public directory.
// TODO(https://crbug.com/920634): Remove this function, because
// ResourceRequest::request_initator should now be trustworthy *within the
// NetworkService* (see r888724).
COMPONENT_EXPORT(NETWORK_CPP)
url::Origin GetTrustworthyInitiator(
const absl::optional<url::Origin>& request_initiator_origin_lock,
const absl::optional<url::Origin>& request_initiator);
namespace debug {
class COMPONENT_EXPORT(NETWORK_CPP) ScopedRequestInitiatorOriginLockCrashKey
: public url::debug::ScopedOriginCrashKey {
public:
explicit ScopedRequestInitiatorOriginLockCrashKey(
const absl::optional<url::Origin>& request_initiator_origin_lock);
~ScopedRequestInitiatorOriginLockCrashKey();
ScopedRequestInitiatorOriginLockCrashKey(
const ScopedRequestInitiatorOriginLockCrashKey&) = delete;
ScopedRequestInitiatorOriginLockCrashKey& operator=(
const ScopedRequestInitiatorOriginLockCrashKey&) = delete;
};
} // namespace debug
} // namespace network
#endif // SERVICES_NETWORK_PUBLIC_CPP_INITIATOR_LOCK_COMPATIBILITY_H_

@ -4,6 +4,7 @@
#include "services/network/public/cpp/resource_request.h"
#include "base/strings/string_number_conversions.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "net/base/load_flags.h"
#include "services/network/public/mojom/cookie_access_observer.mojom.h"
@ -76,6 +77,24 @@ bool OptionalWebBundleTokenParamsEqualsForTesting( // IN-TEST
(lhs && rhs && lhs->EqualsForTesting(*rhs)); // IN-TEST
}
base::debug::CrashKeyString* GetRequestUrlCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_url", base::debug::CrashKeySize::Size256);
return crash_key;
}
base::debug::CrashKeyString* GetRequestInitiatorCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_initiator", base::debug::CrashKeySize::Size64);
return crash_key;
}
base::debug::CrashKeyString* GetRequestResourceTypeCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"request_resource_type", base::debug::CrashKeySize::Size32);
return crash_key;
}
} // namespace
ResourceRequest::TrustedParams::TrustedParams() = default;
@ -269,4 +288,17 @@ net::ReferrerPolicy ReferrerPolicyForUrlRequest(
return net::ReferrerPolicy::CLEAR_ON_TRANSITION_FROM_SECURE_TO_INSECURE;
}
namespace debug {
ScopedResourceRequestCrashKeys::ScopedResourceRequestCrashKeys(
const network::ResourceRequest& request)
: url_(GetRequestUrlCrashKey(), request.url.possibly_invalid_spec()),
request_initiator_(GetRequestInitiatorCrashKey(),
base::OptionalOrNullptr(request.request_initiator)),
resource_type_(GetRequestResourceTypeCrashKey(),
base::NumberToString(request.resource_type)) {}
ScopedResourceRequestCrashKeys::~ScopedResourceRequestCrashKeys() = default;
} // namespace debug
} // namespace network

@ -9,6 +9,7 @@
#include <string>
#include "base/component_export.h"
#include "base/debug/crash_logging.h"
#include "base/memory/ref_counted.h"
#include "base/unguessable_token.h"
#include "mojo/public/cpp/bindings/remote.h"
@ -178,6 +179,26 @@ COMPONENT_EXPORT(NETWORK_CPP_BASE)
net::ReferrerPolicy ReferrerPolicyForUrlRequest(
mojom::ReferrerPolicy referrer_policy);
namespace debug {
class COMPONENT_EXPORT(NETWORK_CPP_BASE) ScopedResourceRequestCrashKeys {
public:
explicit ScopedResourceRequestCrashKeys(
const network::ResourceRequest& request);
~ScopedResourceRequestCrashKeys();
ScopedResourceRequestCrashKeys(const ScopedResourceRequestCrashKeys&) =
delete;
ScopedResourceRequestCrashKeys& operator=(
const ScopedResourceRequestCrashKeys&) = delete;
private:
base::debug::ScopedCrashKeyString url_;
url::debug::ScopedOriginCrashKey request_initiator_;
base::debug::ScopedCrashKeyString resource_type_;
};
} // namespace debug
} // namespace network
#endif // SERVICES_NETWORK_PUBLIC_CPP_RESOURCE_REQUEST_H_