Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(184)

Unified Diff: third_party/WebKit/Source/core/loader/FrameLoader.cpp

Issue 2790693002: Split CSP into pre- and post-upgrade checks (Closed)
Patch Set: add mkwst TODO Created 3 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 8cd77648a4dd0116286c5cf0df7be66d44d27041..79b097f9281ca8c90899efa8dee67898f64e100a 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -139,6 +139,63 @@ static void CheckForLegacyProtocolInSubresource(
document, UseCounter::kLegacyProtocolEmbeddedAsSubresource);
}
+static NavigationPolicy MaybeCheckCSP(
+ const ResourceRequest& request,
+ NavigationType type,
+ LocalFrame* frame,
+ NavigationPolicy policy,
+ bool should_check_main_world_content_security_policy,
+ bool browser_side_navigation_enabled,
+ ContentSecurityPolicy::CheckHeaderType check_header_type) {
+ // If we're loading content into |frame| (NavigationPolicyCurrentTab), check
+ // against the parent's Content Security Policy and kill the load if that
+ // check fails, unless we should bypass the main world's CSP.
+ if (policy == kNavigationPolicyCurrentTab &&
+ should_check_main_world_content_security_policy &&
+ // TODO(arthursonzogni): 'frame-src' check is disabled on the
+ // renderer side with browser-side-navigation, but is enforced on the
+ // browser side. See http://crbug.com/692595 for understanding why it
+ // can't be enforced on both sides instead.
+ !browser_side_navigation_enabled) {
+ Frame* parent_frame = frame->Tree().Parent();
+ if (parent_frame) {
+ ContentSecurityPolicy* parent_policy =
+ parent_frame->GetSecurityContext()->GetContentSecurityPolicy();
+ if (!parent_policy->AllowFrameFromSource(
+ request.Url(), request.GetRedirectStatus(),
+ SecurityViolationReportingPolicy::kReport, check_header_type)) {
+ // Fire a load event, as timing attacks would otherwise reveal that the
+ // frame was blocked. This way, it looks like every other cross-origin
+ // page load.
+ frame->GetDocument()->EnforceSandboxFlags(kSandboxOrigin);
+ frame->Owner()->DispatchLoad();
+ return kNavigationPolicyIgnore;
+ }
+ }
+ }
+
+ bool is_form_submission = type == kNavigationTypeFormSubmitted ||
+ type == kNavigationTypeFormResubmitted;
+ if (is_form_submission &&
+ // 'form-action' check in the frame that is navigating is disabled on the
+ // renderer side when PlzNavigate is enabled, but is enforced on the
+ // browser side instead.
+ // N.B. check in the frame that initiates the navigation stills occurs in
+ // blink and is not enforced on the browser-side.
+ // TODO(arthursonzogni) The 'form-action' check should be fully disabled
+ // in blink when browser side navigation is enabled, except when the form
+ // submission doesn't trigger a navigation(i.e. javascript urls). Please
+ // see https://crbug.com/701749
+ !browser_side_navigation_enabled &&
+ !frame->GetDocument()->GetContentSecurityPolicy()->AllowFormAction(
+ request.Url(), request.GetRedirectStatus(),
+ SecurityViolationReportingPolicy::kReport, check_header_type)) {
+ return kNavigationPolicyIgnore;
+ }
+
+ return policy;
+}
+
ResourceRequest FrameLoader::ResourceRequestForReload(
FrameLoadType frame_load_type,
const KURL& override_url,
@@ -1370,51 +1427,12 @@ NavigationPolicy FrameLoader::ShouldContinueForNavigationPolicy(
return kNavigationPolicyCurrentTab;
Settings* settings = frame_->GetSettings();
- bool browser_side_navigation_enabled =
- settings && settings->GetBrowserSideNavigationEnabled();
-
- // If we're loading content into |m_frame| (NavigationPolicyCurrentTab), check
- // against the parent's Content Security Policy and kill the load if that
- // check fails, unless we should bypass the main world's CSP.
- if (policy == kNavigationPolicyCurrentTab &&
- should_check_main_world_content_security_policy ==
- kCheckContentSecurityPolicy &&
- // TODO(arthursonzogni): 'frame-src' check is disabled on the
- // renderer side with browser-side-navigation, but is enforced on the
- // browser side. See http://crbug.com/692595 for understanding why it
- // can't be enforced on both sides instead.
- !browser_side_navigation_enabled) {
- Frame* parent_frame = frame_->Tree().Parent();
- if (parent_frame) {
- ContentSecurityPolicy* parent_policy =
- parent_frame->GetSecurityContext()->GetContentSecurityPolicy();
- if (!parent_policy->AllowFrameFromSource(request.Url(),
- request.GetRedirectStatus())) {
- // Fire a load event, as timing attacks would otherwise reveal that the
- // frame was blocked. This way, it looks like every other cross-origin
- // page load.
- frame_->GetDocument()->EnforceSandboxFlags(kSandboxOrigin);
- frame_->Owner()->DispatchLoad();
- return kNavigationPolicyIgnore;
- }
- }
- }
-
- bool is_form_submission = type == kNavigationTypeFormSubmitted ||
- type == kNavigationTypeFormResubmitted;
- if (is_form_submission &&
- // 'form-action' check in the frame that is navigating is disabled on the
- // renderer side when PlzNavigate is enabled, but is enforced on the
- // browser side instead.
- // N.B. check in the frame that initiates the navigation stills occurs in
- // blink and is not enforced on the browser-side.
- // TODO(arthursonzogni) The 'form-action' check should be fully disabled
- // in blink when browser side navigation is enabled, except when the form
- // submission doesn't trigger a navigation(i.e. javascript urls). Please
- // see https://crbug.com/701749
- !browser_side_navigation_enabled &&
- !frame_->GetDocument()->GetContentSecurityPolicy()->AllowFormAction(
- request.Url(), request.GetRedirectStatus())) {
+ if (MaybeCheckCSP(request, type, frame_, policy,
+ should_check_main_world_content_security_policy ==
+ kCheckContentSecurityPolicy,
+ settings && settings->GetBrowserSideNavigationEnabled(),
+ ContentSecurityPolicy::CheckHeaderType::kCheckEnforce) ==
+ kNavigationPolicyIgnore) {
return kNavigationPolicyIgnore;
}
@@ -1439,6 +1457,31 @@ NavigationPolicy FrameLoader::ShouldContinueForNavigationPolicy(
return kNavigationPolicyIgnore;
}
+NavigationPolicy FrameLoader::ShouldContinueForRedirectNavigationPolicy(
+ const ResourceRequest& request,
+ const SubstituteData& substitute_data,
+ DocumentLoader* loader,
+ ContentSecurityPolicyDisposition
+ should_check_main_world_content_security_policy,
+ NavigationType type,
+ NavigationPolicy policy,
+ FrameLoadType frame_load_type,
+ bool is_client_redirect,
+ HTMLFormElement* form) {
+ Settings* settings = frame_->GetSettings();
+ // Check report-only CSP policies, which are not checked by
+ // ShouldContinueForNavigationPolicy.
+ MaybeCheckCSP(request, type, frame_, policy,
+ should_check_main_world_content_security_policy ==
+ kCheckContentSecurityPolicy,
+ settings && settings->GetBrowserSideNavigationEnabled(),
+ ContentSecurityPolicy::CheckHeaderType::kCheckReportOnly);
+ return ShouldContinueForNavigationPolicy(
+ request, substitute_data, loader,
+ should_check_main_world_content_security_policy, type, policy,
+ frame_load_type, is_client_redirect, form);
+}
+
NavigationPolicy FrameLoader::CheckLoadCanStart(
FrameLoadRequest& frame_load_request,
FrameLoadType type,
@@ -1453,6 +1496,20 @@ NavigationPolicy FrameLoader::CheckLoadCanStart(
// request.
ResourceRequest& resource_request = frame_load_request.GetResourceRequest();
RecordLatestRequiredCSP();
+ // Before modifying the request, check report-only CSP headers to give the
+ // site owner a chance to learn about requests that need to be modified.
+ //
+ // TODO(estark): this doesn't work with --enable-browser-side-navigation,
+ // wherein 'frame-src' is checked in the browser process. Figure out what to
+ // do; maybe with browser-side navigation the upgrade should be happening in
+ // the browser process too. See also https://crbug.com/692595
+ Settings* settings = frame_->GetSettings();
+ MaybeCheckCSP(
+ resource_request, navigation_type, frame_, navigation_policy,
+ frame_load_request.ShouldCheckMainWorldContentSecurityPolicy() ==
+ kCheckContentSecurityPolicy,
+ settings && settings->GetBrowserSideNavigationEnabled(),
+ ContentSecurityPolicy::CheckHeaderType::kCheckReportOnly);
ModifyRequestForCSP(resource_request, nullptr);
return ShouldContinueForNavigationPolicy(
« no previous file with comments | « third_party/WebKit/Source/core/loader/FrameLoader.h ('k') | third_party/WebKit/Source/platform/loader/fetch/FetchContext.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698