Commit e4135825 authored by arthursonzogni's avatar arthursonzogni Committed by Chromium LUCI CQ

Error page don't inherit sandbox-flags from original response.

Due to a bug, the sandox_flags computed from the browser process were
taking into account the Content-Security-Policy:sandbox of the original
response, even for error pages.

This fixes the bug and add regressions tests.

Bug: 1158370,1158306
Change-Id: Idc1c575b0a4e578542c66b3f7153c6dac5d1e43d
Fixed: 1158306
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587047
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: default avatarŁukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837035}
parent bd616559
......@@ -75,6 +75,7 @@
#include "net/test/url_request/url_request_failed_job.h"
#include "services/metrics/public/cpp/ukm_source_id.h"
#include "services/network/public/cpp/features.h"
#include "services/network/public/cpp/web_sandbox_flags.h"
#include "services/network/public/mojom/url_loader.mojom.h"
#include "third_party/blink/public/common/loader/previews_state.h"
#include "third_party/blink/public/common/loader/url_loader_throttle.h"
......@@ -220,6 +221,23 @@ const char* non_cacheable_html_response =
"\n"
"HTML content.";
// Insert a navigation throttle blocking every navigation in its
// WillProcessResponse handler.
std::unique_ptr<content::TestNavigationThrottleInserter>
BlockNavigationWillProcessResponse(WebContentsImpl* web_content) {
return std::make_unique<content::TestNavigationThrottleInserter>(
web_content,
base::BindLambdaForTesting(
[&](NavigationHandle* handle) -> std::unique_ptr<NavigationThrottle> {
auto throttle = std::make_unique<TestNavigationThrottle>(handle);
throttle->SetResponse(TestNavigationThrottle::WILL_PROCESS_RESPONSE,
TestNavigationThrottle::SYNCHRONOUS,
NavigationThrottle::BLOCK_RESPONSE);
return throttle;
}));
}
} // namespace
// Test about navigation.
......@@ -4041,6 +4059,65 @@ IN_PROC_BROWSER_TEST_F(NavigationBrowserTest,
EXPECT_NE(current_frame_host()->GetLastCommittedOrigin(), origin_committed);
}
// Regression test for https://crbug.com/1158306.
// Navigate to a response, which set Content-Security-Policy: sandbox AND block
// the response. The error page shouldn't set sandbox flags.
IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, ErrorPageFromCspSandboxResponse) {
// Block every navigation in WillProcessResponse.
std::unique_ptr<content::TestNavigationThrottleInserter> blocker =
BlockNavigationWillProcessResponse(web_contents());
// Navigate toward a document witch sets CSP:sandbox.
GURL url = embedded_test_server()->GetURL(
"a.com", "/set-header?Content-Security-Policy: sandbox");
TestNavigationManager manager(web_contents(), url);
shell()->LoadURL(url);
manager.WaitForNavigationFinished();
// An error page committed. It doesn't have any sandbox flags, despite the
// original response headers.
EXPECT_TRUE(current_frame_host()->is_error_page());
EXPECT_EQ(network::mojom::WebSandboxFlags::kNone,
current_frame_host()->active_sandbox_flags());
EXPECT_EQ(url, current_frame_host()->GetLastCommittedURL());
EXPECT_TRUE(current_frame_host()->GetLastCommittedOrigin().opaque());
EXPECT_TRUE(
current_frame_host()->GetLastCommittedOrigin().CanBeDerivedFrom(url));
}
// Do sandbox flags apply to error page in sandboxed iframes?
// Apparently yes.
// TODO(https://crbug.com/1158370): Reconsider this.
IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, ErrorPageFromInSandboxedIframe) {
GURL url = embedded_test_server()->GetURL("a.com", "/empty.html");
EXPECT_TRUE(NavigateToURL(shell(), url));
// Block every navigation in WillProcessResponse.
std::unique_ptr<content::TestNavigationThrottleInserter> blocker =
BlockNavigationWillProcessResponse(web_contents());
TestNavigationManager manager(web_contents(), url);
ExecuteScriptAsync(current_frame_host(), R"(
let iframe = document.createElement("iframe");
iframe.src = location.href;
iframe.sandbox = "allow-orientation-lock";
document.body.appendChild(iframe);
)");
manager.WaitForNavigationFinished();
RenderFrameHostImpl* child_rfh =
current_frame_host()->child_at(0)->current_frame_host();
// An error page committed. Apparently, the error page inherited sandbox flags
// from its parent.
// TODO(https://crbug.com/1158370): Reconsider this.
EXPECT_TRUE(child_rfh->is_error_page());
EXPECT_EQ(network::mojom::WebSandboxFlags::kAll &
~network::mojom::WebSandboxFlags::kOrientationLock,
child_rfh->active_sandbox_flags());
}
IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, OriginToCommitSandboxFromFrame) {
GURL url = embedded_test_server()->GetURL("a.com", "/empty.html");
EXPECT_TRUE(NavigateToURL(shell(), url));
......
......@@ -1435,7 +1435,7 @@ void NavigationRequest::BeginNavigation() {
EnterChildTraceEvent("ResponseStarted", this);
is_loaded_from_mhtml_archive_ = IsForMhtmlSubframe();
ComputeSandboxFlagsToCommit();
ComputeSandboxFlagsToCommit(/*response_head=*/nullptr);
// Same-document navigations occur in the currently loaded document. See
// also RenderFrameHostManager::DidCreateNavigationRequest() which will
......@@ -2222,7 +2222,7 @@ void NavigationRequest::OnResponseStarted(
is_loaded_from_mhtml_archive_ = is_mhtml_archive || IsForMhtmlSubframe();
ComputeSandboxFlagsToCommit();
ComputeSandboxFlagsToCommit(response_head_.get());
// The navigation may have encountered an origin policy or Origin-Isolation
// header that requests isolation for the url's origin. Before we pick the
......@@ -3209,7 +3209,12 @@ void NavigationRequest::CommitErrorPage(
is_loaded_from_mhtml_archive_ = false;
sandbox_flags_to_commit_.reset();
ComputeSandboxFlagsToCommit();
// TODO(https://crbug.com/1158370): Apparently, error pages inherit sandbox
// flags from their parent/opener. Document loaded from the network
// shouldn't have any influence over Chrome's internal error page. We should
// define our own flags, preferably the strictest ones instead.
ComputeSandboxFlagsToCommit(/*response_head=*/nullptr);
ReadyToCommitNavigation(true);
// Use a separate cache shard, and no cookies, for error pages.
isolation_info_for_subresources_ = net::IsolationInfo::CreateTransient();
......@@ -5281,7 +5286,8 @@ NavigationRequest::TakeCookieObservers() {
return cookie_observers_.TakeReceivers();
}
void NavigationRequest::ComputeSandboxFlagsToCommit() {
void NavigationRequest::ComputeSandboxFlagsToCommit(
const network::mojom::URLResponseHead* response_head) {
DCHECK(commit_params_);
DCHECK(!HasCommitted());
DCHECK(!IsErrorPage());
......@@ -5290,9 +5296,9 @@ void NavigationRequest::ComputeSandboxFlagsToCommit() {
sandbox_flags_to_commit_ = commit_params_->frame_policy.sandbox_flags;
// The response can also restrict the policy further.
if (response_head_) {
if (response_head) {
for (const auto& csp :
response_head_->parsed_headers->content_security_policy) {
response_head->parsed_headers->content_security_policy) {
*sandbox_flags_to_commit_ |= csp->sandbox;
}
}
......
......@@ -1130,7 +1130,8 @@ class CONTENT_EXPORT NavigationRequest
// Compute the sandbox policy of the document to be loaded. This is called
// once the final response is known. It is based on the current FramePolicy
// and the response's CSP.
void ComputeSandboxFlagsToCommit();
void ComputeSandboxFlagsToCommit(
const network::mojom::URLResponseHead* response_head);
// DCHECK that tranistioning from the current state to |state| valid. This
// does nothing in non-debug builds.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment