PlzNavigate: Enforce 'frame-src' CSP on the browser.

content/browser/frame_host/ancestor_throttle.cc

 // Copyright 2016 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 "content/browser/frame_host/ancestor_throttle.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/browser/frame_host/navigation_request.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/navigation_throttle.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/console_message_level.h"
 #include "net/http/http_response_headers.h"
 #include "url/origin.h"
 
 namespace content {
 
 namespace {
 const char kXFrameOptionsSameOriginHistogram[] = "Security.XFrameOptions";
 
 // This enum is used for UMA metrics. Keep these enums up to date with
@@ skipping 131 matching lines @@
       RecordXFrameOptionsUsage(BYPASS);
       return NavigationThrottle::PROCEED;
     case HeaderDisposition::ALLOWALL:
       RecordXFrameOptionsUsage(ALLOWALL);
       return NavigationThrottle::PROCEED;
   }
   NOTREACHED();
   return NavigationThrottle::BLOCK_RESPONSE;
 }
 
+NavigationThrottle::ThrottleCheckResult
+AncestorThrottle::CheckContentSecurityPolicyFrameSrc(bool is_redirect) {
+  // If PlzNavigate is not enabled, "frame-src" can not be enforced on the
+  // browser-side since a NavigationRequest is needed below. It doesn't matter
+  // because it is still enforced on the renderer-side.
+  if (!IsBrowserSideNavigationEnabled())
+    return NavigationThrottle::PROCEED;
+
+  NavigationHandleImpl* handle =
+      static_cast<NavigationHandleImpl*>(navigation_handle());
+
+  const GURL& url = navigation_handle()->GetURL();
+  if (url.SchemeIs(url::kAboutScheme))
+    return NavigationThrottle::PROCEED;
+
+  // Allow the request when it bypasses the CSP of the parent frame.
+  // Note: it is possible that there is no navigation_request associated with
+  // this navigation, but it only happens when the navigation_handle was created
+  // by CreateNavigationHandleForTesting().
+  if (handle->frame_tree_node()->navigation_request() &&
+      handle->frame_tree_node()
+          ->navigation_request()
+          ->common_params()
+          .should_bypass_main_world_CSP) {
+    return NavigationThrottle::PROCEED;
+  }
+
+  auto parent = handle->frame_tree_node()->parent();
+  DCHECK(parent);
+
+  CSPContext* csp_context = parent->ContentSecurityPolicyContext();
+  if (!csp_context->Allow(parent->ContentSecurityPolicies(),
+                          CSPDirective::FrameSrc, url, is_redirect)) {
+    return NavigationThrottle::BLOCK_REQUEST;

[alexmos, 2017/02/10 22:59:53]

Will this result in loading a regular error page?  I remember we discussed
whether to load a blank page with a unique origin (data:,) temporarily until we
fix the error pages, because otherwise we can get the problems with the CWS kill
and more down the road as we tighten up CanCommitURL for OOPIFs
(https://crbug.com/622385, https://crbug.com/689733).  Can you check if you get
the kill in 622385 with this approach, if a parent page declares a CSP that
disallows the CWS URL?  Would the frame-src checks happen before XFO blocks it
using BLOCK_RESPONSE, for which you used data:, when you moved XFO over to the
browser?

[arthursonzogni, 2017/02/13 16:33:20]

Yes you are right. I forgot this.
XFO checks happens after 'frame-src'.
I will have to create a new ThrottleCheckResult for this.

[arthursonzogni, 2017/02/15 17:02:15]

I am working on a solution. See https://codereview.chromium.org/2697713005/
Now, the browser will not ask the renderer to commit an error page with an URL
that would make the browser kills the renderer if it tries to commit it.
Unfortunately, it only works with PlzNavigate. 

+  }
+
+  return NavigationThrottle::PROCEED;
+}
+
+NavigationThrottle::ThrottleCheckResult AncestorThrottle::WillStartRequest() {
+  return CheckContentSecurityPolicyFrameSrc(false);
+}
+
+NavigationThrottle::ThrottleCheckResult
+AncestorThrottle::WillRedirectRequest() {
+  return CheckContentSecurityPolicyFrameSrc(true);
+}
+
 AncestorThrottle::AncestorThrottle(NavigationHandle* handle)
     : NavigationThrottle(handle) {}
 
 void AncestorThrottle::ParseError(const std::string& value,
                                   HeaderDisposition disposition) {
   DCHECK(disposition == HeaderDisposition::CONFLICT ||
          disposition == HeaderDisposition::INVALID);
   if (!navigation_handle()->GetRenderFrameHost())
     return;  // Some responses won't have a RFH (i.e. 204/205s or downloads).
 
@@ skipping 82 matching lines @@
       HeadersContainFrameAncestorsCSP(headers)) {
     // TODO(mkwst): 'frame-ancestors' is currently handled in Blink. We should
     // handle it here instead. Until then, don't block the request, and let
     // Blink handle it. https://crbug.com/555418
     return HeaderDisposition::BYPASS;
   }
   return result;
 }
 
 }  // namespace content