[CORS-RFC1918] Block insecure navigation requests.
For subresource URL requests, the renderer URLLoader uses its factory's
parameters to determine the client security state of the request
initiator. Navigations however use a central URLLoaderFactory in the
browser process.
This change propagates the client security state in the ResourceRequest
to the URLLoader as a trusted parameter, and acts on this state in
URLLoader.
It only improves the situation for non-top-level navigation requests.
The latter need some thought given to the UX first, see:
https://crbug.com/1129326.
Bug: chromium:1124346
Change-Id: I8736706ef402831375f34f9c7f2a96d3059c25be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410406
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Titouan Rigoudy <titouan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811670}
diff --git a/content/browser/loader/navigation_url_loader_impl.cc b/content/browser/loader/navigation_url_loader_impl.cc
index 394ad03..3a0f7d0 100644
--- a/content/browser/loader/navigation_url_loader_impl.cc
+++ b/content/browser/loader/navigation_url_loader_impl.cc
@@ -203,6 +203,8 @@
new_request->trusted_params = network::ResourceRequest::TrustedParams();
new_request->trusted_params->isolation_info = request_info->isolation_info;
new_request->trusted_params->cookie_observer = std::move(cookie_observer);
+ new_request->trusted_params->client_security_state =
+ request_info->client_security_state.Clone();
new_request->is_main_frame = request_info->is_main_frame;
net::RequestPriority net_priority = net::HIGHEST;
diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index 136d721c..6881841 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -2791,6 +2791,31 @@
interceptor.push_back(web_bundle_handle_->TakeInterceptor());
net::HttpRequestHeaders cors_exempt_headers;
std::swap(cors_exempt_headers, cors_exempt_request_headers_);
+
+ // For subresource requests the ClientSecurityState is passed through
+ // URLLoaderFactoryParams. That does not work for navigation requests
+ // because they all share a common factory, so each request is tagged with
+ // a ClientSecurityState to use instead.
+ //
+ // We currently define the client of the fetch as the parent frame, if any.
+ // This is probably incorrect: frames can cause others in the same browsing
+ // context group to navigate to pages, without being the parent. Additionally
+ // there is no client security state for top-level navigations, which mainly
+ // means that CORS-RFC1918 checks are skipped for such requests.
+ //
+ // TODO(https://crbug.com/1129326): Figure out the UX story for top-level
+ // navigations and spooky-action-at-a-distance navigations, then revisit this.
+ // The client security state might need to be that of the initiator of the
+ // navigation, or we might need to take into account both the parent frame and
+ // the initiator's client security states. In any case, we should probably
+ // always provide a client security state.
+ network::mojom::ClientSecurityStatePtr client_security_state = nullptr;
+ RenderFrameHostImpl* parent = GetParentFrame();
+ if (parent) {
+ client_security_state =
+ parent->last_committed_client_security_state().Clone();
+ }
+
loader_ = NavigationURLLoader::Create(
browser_context, partition,
std::make_unique<NavigationRequestInfo>(
@@ -2806,7 +2831,7 @@
: nullptr,
devtools_navigation_token(), frame_tree_node_->devtools_frame_token(),
OriginPolicyThrottle::ShouldRequestOriginPolicy(common_params_->url),
- std::move(cors_exempt_headers), nullptr /* client_security_state */),
+ std::move(cors_exempt_headers), std::move(client_security_state)),
std::move(navigation_ui_data), service_worker_handle_.get(),
appcache_handle_.get(), std::move(prefetched_signed_exchange_cache_),
this, IsServedFromBackForwardCache(), CreateCookieAccessObserver(),
diff --git a/content/browser/renderer_host/navigation_request.h b/content/browser/renderer_host/navigation_request.h
index ba4804c..b5a7d2c 100644
--- a/content/browser/renderer_host/navigation_request.h
+++ b/content/browser/renderer_host/navigation_request.h
@@ -1113,6 +1113,7 @@
// In that case, the caller must immediately return.
bool MaybeCancelFailedNavigation();
+ // Never null. The pointee node owns this navigation request instance.
FrameTreeNode* const frame_tree_node_;
// Value of |is_for_commit| supplied to the constructor.
diff --git a/content/browser/renderer_host/navigation_request_info.h b/content/browser/renderer_host/navigation_request_info.h
index ea285de..c52014a 100644
--- a/content/browser/renderer_host/navigation_request_info.h
+++ b/content/browser/renderer_host/navigation_request_info.h
@@ -95,6 +95,9 @@
// Specifies the security state applying to the navigation. For iframes, this
// is the security state of their parent. Nullptr otherwise.
+ //
+ // TODO(https://crbug.com/1129326): Set this for top-level navigation requests
+ // too once the UX story is sorted out.
const network::mojom::ClientSecurityStatePtr client_security_state;
};
diff --git a/content/browser/renderer_host/render_frame_host_impl_browsertest.cc b/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
index 712b33a..3fe3daa 100644
--- a/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
+++ b/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
@@ -87,7 +87,6 @@
namespace content {
namespace {
-using ::testing::EndsWith;
using ::testing::NotNull;
// Implementation of ContentBrowserClient that overrides
@@ -3938,11 +3937,8 @@
// Check that the child iframe failed to fetch.
ASSERT_EQ(1ul, root_frame_host()->child_count());
auto* child_frame = root_frame_host()->child_at(0)->current_frame_host();
- // TODO(crbug.com/1124346): Expect 0 once the load fails.
- EXPECT_EQ(200, child_frame->last_http_status_code());
- // TODO(crbug.com/1124346): Expect an empty URL once the load fails.
- EXPECT_THAT(child_frame->last_successful_url().spec(),
- EndsWith("/empty.html"));
+ EXPECT_EQ(0, child_frame->last_http_status_code());
+ EXPECT_EQ(GURL(), child_frame->last_successful_url());
}
namespace {
diff --git a/services/network/url_loader.cc b/services/network/url_loader.cc
index ff2ca894..e13745d 100644
--- a/services/network/url_loader.cc
+++ b/services/network/url_loader.cc
@@ -589,8 +589,24 @@
CrossOriginReadBlockingExceptionForPlugin::ShouldAllowForPlugin(
factory_params_->process_id);
request_mode_ = request.mode;
+
if (request.trusted_params) {
has_user_activation_ = request.trusted_params->has_user_activation;
+
+ if (factory_params_->client_security_state) {
+ // Enforce that only one ClientSecurityState is ever given to us, as this
+ // is an invariant in the current codebase. In case of a compromised
+ // renderer process, we might be passed both, in which case we prefer to
+ // use the factory params' value: contrary to the request params, it is
+ // always sourced from the browser process.
+ DCHECK(!request.trusted_params->client_security_state)
+ << "Must not provide a ClientSecurityState in both "
+ "URLLoaderFactoryParams and ResourceRequest::TrustedParams.";
+ } else {
+ // This might be nullptr, but that does not matter. Clone it anyways.
+ request_client_security_state_ =
+ request.trusted_params->client_security_state.Clone();
+ }
}
throttling_token_ = network::ScopedThrottlingToken::MaybeCreate(
@@ -977,8 +993,24 @@
return net::ERR_INSECURE_PRIVATE_NETWORK_REQUEST;
}
- const mojom::ClientSecurityStatePtr& security_state =
- factory_params_->client_security_state;
+ // Depending on the type of URL request, we source the client security state
+ // from either the URLRequest's trusted params (for navigations, which share
+ // a factory) or the URLLoaderFactory's params. We prefer the factory params
+ // over the request params, as the former always come from the browser
+ // process.
+ //
+ // We use a raw pointer instead of a const-ref to a StructPtr in order to be
+ // able to assign a new value to the variable. A ternary operator would let us
+ // define a const-ref but would prevent logging which branch was taken.
+ const mojom::ClientSecurityState* security_state =
+ factory_params_->client_security_state.get();
+ if (security_state) {
+ DVLOG(1) << "CORS-RFC1918 check: using factory client security state.";
+ } else {
+ DVLOG(1) << "CORS-RFC1918 check: using request client security state.";
+ security_state = request_client_security_state_.get();
+ }
+
if (!security_state) {
DVLOG(1) << "CORS-RFC1918 check: skipped, missing client security state.";
return net::OK;
diff --git a/services/network/url_loader.h b/services/network/url_loader.h
index ed6dfca8..70518f4 100644
--- a/services/network/url_loader.h
+++ b/services/network/url_loader.h
@@ -509,6 +509,13 @@
// Observer listening to all cookie reads and writes made by this request.
mojo::Remote<mojom::CookieAccessObserver> cookie_observer_;
+ // Client security state copied from the input ResourceRequest.
+ //
+ // If |factory_params_->client_security_state| is non-null, this is null.
+ // We indeed prefer the factory params over the request params as we trust the
+ // former more, given that they always come from the browser process.
+ mojom::ClientSecurityStatePtr request_client_security_state_;
+
// Indicates |url_request_| is fetch upload request and that has streaming
// body.
const bool has_fetch_streaming_upload_body_;
diff --git a/services/network/url_loader_unittest.cc b/services/network/url_loader_unittest.cc
index 914cd69f3..bd8415c7 100644
--- a/services/network/url_loader_unittest.cc
+++ b/services/network/url_loader_unittest.cc
@@ -566,6 +566,9 @@
if (request_body_)
request.request_body = request_body_;
+ request.trusted_params->client_security_state.Swap(
+ &request_client_security_state_);
+
base::RunLoop delete_run_loop;
mojo::Remote<mojom::URLLoader> loader;
std::unique_ptr<URLLoader> url_loader;
@@ -573,7 +576,7 @@
mojom::URLLoaderFactoryParams params;
params.process_id = mojom::kBrowserProcessId;
params.is_corb_enabled = false;
- params.client_security_state.Swap(&client_security_state_);
+ params.client_security_state.Swap(&factory_client_security_state_);
url::Origin origin = url::Origin::Create(url);
params.isolation_info =
@@ -749,8 +752,11 @@
DCHECK(!ran_);
expect_redirect_ = true;
}
- void set_client_security_state(mojom::ClientSecurityStatePtr state) {
- client_security_state_ = std::move(state);
+ void set_factory_client_security_state(mojom::ClientSecurityStatePtr state) {
+ factory_client_security_state_ = std::move(state);
+ }
+ void set_request_client_security_state(mojom::ClientSecurityStatePtr state) {
+ request_client_security_state_ = std::move(state);
}
void set_request_body(scoped_refptr<ResourceRequestBody> request_body) {
request_body_ = request_body;
@@ -875,7 +881,8 @@
bool send_ssl_with_response_ = false;
bool send_ssl_for_cert_error_ = false;
bool expect_redirect_ = false;
- mojom::ClientSecurityStatePtr client_security_state_;
+ mojom::ClientSecurityStatePtr factory_client_security_state_;
+ mojom::ClientSecurityStatePtr request_client_security_state_;
scoped_refptr<ResourceRequestBody> request_body_;
// Used to ensure that methods are called either before or after a request is
@@ -936,7 +943,7 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = true;
client_security_state->ip_address_space = mojom::IPAddressSpace::kUnknown;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -945,7 +952,7 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = true;
client_security_state->ip_address_space = mojom::IPAddressSpace::kPublic;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -954,7 +961,7 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = true;
client_security_state->ip_address_space = mojom::IPAddressSpace::kPrivate;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -963,7 +970,7 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = true;
client_security_state->ip_address_space = mojom::IPAddressSpace::kLocal;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -976,7 +983,7 @@
client_security_state->private_network_request_policy =
mojom::PrivateNetworkRequestPolicy::kAllow;
client_security_state->ip_address_space = mojom::IPAddressSpace::kUnknown;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -986,7 +993,7 @@
client_security_state->private_network_request_policy =
mojom::PrivateNetworkRequestPolicy::kAllow;
client_security_state->ip_address_space = mojom::IPAddressSpace::kPublic;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -996,7 +1003,7 @@
client_security_state->private_network_request_policy =
mojom::PrivateNetworkRequestPolicy::kAllow;
client_security_state->ip_address_space = mojom::IPAddressSpace::kPrivate;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -1006,7 +1013,7 @@
client_security_state->private_network_request_policy =
mojom::PrivateNetworkRequestPolicy::kAllow;
client_security_state->ip_address_space = mojom::IPAddressSpace::kLocal;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
@@ -1025,7 +1032,7 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = false;
client_security_state->ip_address_space = mojom::IPAddressSpace::kUnknown;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")),
IsError(net::ERR_INSECURE_PRIVATE_NETWORK_REQUEST));
@@ -1035,7 +1042,7 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = false;
client_security_state->ip_address_space = mojom::IPAddressSpace::kPublic;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")),
IsError(net::ERR_INSECURE_PRIVATE_NETWORK_REQUEST));
@@ -1045,7 +1052,7 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = false;
client_security_state->ip_address_space = mojom::IPAddressSpace::kPrivate;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")),
IsError(net::ERR_INSECURE_PRIVATE_NETWORK_REQUEST));
@@ -1055,11 +1062,25 @@
auto client_security_state = NewSecurityState();
client_security_state->is_web_secure_context = false;
client_security_state->ip_address_space = mojom::IPAddressSpace::kLocal;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
EXPECT_THAT(Load(test_server()->GetURL("/empty.html")), IsOk());
}
+// This test verifies that if the request's TrustedParams field carries a client
+// security state indicating that the request initiator is not a secure context
+// and came from the public IP address space, requests to local IP addresses
+// are blocked.
+TEST_F(URLLoaderTest, TrustedParamsInsecurePublicToLocalIsBlocked) {
+ auto client_security_state = NewSecurityState();
+ client_security_state->is_web_secure_context = false;
+ client_security_state->ip_address_space = mojom::IPAddressSpace::kPublic;
+ set_request_client_security_state(std::move(client_security_state));
+
+ EXPECT_THAT(Load(test_server()->GetURL("/empty.html")),
+ IsError(net::ERR_INSECURE_PRIVATE_NETWORK_REQUEST));
+}
+
// Bundles together the inputs to a parameterized private network request test.
struct URLLoaderFakeTransportInfoTestParams {
// The address space of the client.
@@ -1119,7 +1140,7 @@
auto client_security_state = NewSecurityState();
client_security_state->ip_address_space = params.client_address_space;
- set_client_security_state(std::move(client_security_state));
+ set_factory_client_security_state(std::move(client_security_state));
const GURL url("http://fake-endpoint");
