PlzNavigate: Enforce frame-src CSP on the browser.

content/browser/frame_host/ancestor_throttle.cc

Index: content/browser/frame_host/ancestor_throttle.cc
diff --git a/content/browser/frame_host/ancestor_throttle.cc b/content/browser/frame_host/ancestor_throttle.cc
index ca87f0a221d797c54f9304669bd016aa961c9bcc..b19720775786eb0de3778d1a103ad51a4bfab33f 100644
--- a/content/browser/frame_host/ancestor_throttle.cc
+++ b/content/browser/frame_host/ancestor_throttle.cc
@@ -11,9 +11,11 @@
 #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"
@@ -165,6 +167,45 @@ AncestorThrottle::WillProcessResponse() {
   return NavigationThrottle::BLOCK_RESPONSE;
 }
 
+NavigationThrottle::ThrottleCheckResult
+AncestorThrottle::CheckContentSecurityPolicyFrameSrc(bool is_redirect) {
+  // If PlzNavigate is enabled, "frame-src" is enforced on the browser side,
+  // else on the renderer side.
+  if (!IsBrowserSideNavigationEnabled())
+    return NavigationThrottle::PROCEED;
+
+  const GURL& url = navigation_handle()->GetURL();
+  if (url.SchemeIs(url::kAboutScheme))
+    return NavigationThrottle::PROCEED;
+
+  NavigationHandleImpl* handle =
+      static_cast<NavigationHandleImpl*>(navigation_handle());
+
+  // Allow the request when it bypasses the CSP.
+  if (handle->should_bypass_main_world_csp())
+    return NavigationThrottle::PROCEED;
+
+  FrameTreeNode* parent_ftn = handle->frame_tree_node()->parent();
+  DCHECK(parent_ftn);
+  RenderFrameHostImpl* parent = parent_ftn->current_frame_host();
+  DCHECK(parent);
+
+  if (!parent->AllowContentSecurityPolicy(CSPDirective::FrameSrc, url,
+                                          is_redirect))

[nasko, 2017/03/03 23:04:23]

nit: {} needed because of the two line if statement.

[arthursonzogni, 2017/03/06 15:10:12]

Done.

+    return NavigationThrottle::BLOCK_REQUEST;

[nasko, 2017/03/03 23:04:23]

I thought the idea was to return CANCELLED here, so we preserve the existing
behavior of not displaying an error page. Why can't we do this first, to
minimize the change of behavior in this specific CL, then in a follow up address
this specifically?

[arthursonzogni, 2017/03/06 15:10:12]

I had two options: returning BLOCK_REQUEST or CANCEL.
I used BLOCK_REQUEST instead of CANCEL because when an error page is loaded, it
fires the 'load' event and restrict some data the embedder of the iframe could
get.

This is necessary, because an evil embedded could tries to load a page and
detect if an redirect happens  (to know if the user is logged in for instance,
or to know some data based on the URL) by using the CSP source-expressions.

We could try to make return CANCEL and make another set of IPC to tell blink to
achieve the same result, but after some discussions, we decide that loading an
error page was the best option.

+
+  return NavigationThrottle::PROCEED;

[nasko, 2017/03/03 23:04:23]

I wonder if it will be better to fail closed, the return value to be BLOCK
instead of PROCEED, so we notice bugs quickly :).

[arthursonzogni, 2017/03/06 15:10:12]

Sorry, I don't understand :) Do you meant reversing the condition?

+}
+
+NavigationThrottle::ThrottleCheckResult AncestorThrottle::WillStartRequest() {
+  return CheckContentSecurityPolicyFrameSrc(false);
+}
+
+NavigationThrottle::ThrottleCheckResult
+AncestorThrottle::WillRedirectRequest() {
+  return CheckContentSecurityPolicyFrameSrc(true);
+}
+
 AncestorThrottle::AncestorThrottle(NavigationHandle* handle)
     : NavigationThrottle(handle) {}