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

content/common/navigation_params.h

Index: content/common/navigation_params.h
diff --git a/content/common/navigation_params.h b/content/common/navigation_params.h
index c47da6b89c797ca8ceb40b93f578effdf10bbe92..2673ed94ca5a8aa5f970e0ced4bbd7b88357888b 100644
--- a/content/common/navigation_params.h
+++ b/content/common/navigation_params.h
@@ -58,7 +58,8 @@ struct CONTENT_EXPORT CommonNavigationParams {
       PreviewsState previews_state,
       const base::TimeTicks& navigation_start,
       std::string method,
-      const scoped_refptr<ResourceRequestBodyImpl>& post_data);
+      const scoped_refptr<ResourceRequestBodyImpl>& post_data,
+      bool should_bypass_main_world_CSP);

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

for style consistency, I'd favor lowercasing the CSP, i.e.,
should_bypass_main_world_csp.

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

Done.

   CommonNavigationParams(const CommonNavigationParams& other);
   ~CommonNavigationParams();
 
@@ -119,6 +120,13 @@ struct CONTENT_EXPORT CommonNavigationParams {
 
   // Body of HTTP POST request.
   scoped_refptr<ResourceRequestBodyImpl> post_data;
+
+  // Whether or not this navigation, including each redirections, should be
+  // checked against the Content-Security-Policy(CSP) of the frames that
+  // surround it. It is actually used to bypass the 'frame-src' and 'child-src'

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

nit: drop "actually"

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

"frames that surround it" is a big vague.  Should this be just "parent frame",
if this is for frame-src only?  I'm not familiar with this bit, but is it used
to bypass other directives in Blink?

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

Done. I misunderstood what "main_world" means, it is simply the opposite of
"isolated_world". This comment is very bad, I will update it.
AFAIK, it is used for 'frame-src' and not for 'form-action'.

It is also used for others CSP, for instance 'style-src', 'script-src', ..., but
I only care about CSP when it can block a navigation.

[alexmos, 2017/02/14 06:57:20]

Interesting, thanks.  It's sad that an evil renderer can just pass this bit to
turn off the enforcement of frame-src in the browser process.  Perhaps down the
road we could add some validation in the browser process, such as whether the
renderer process really contained extension content scripts that could do this.

By the way, it doesn't seem like the isolated world's CSP is actually enforced
today (looking at how DOMWrapperWorld::setIsolatedWorldContentSecurityPolicy
ignores the passed-in policy).  It's only used to flip the bit that allows
bypassing the main world CSP.  So it doesn't seem like we need to worry about
how to enforce frame-src from the isolated world's CSP, for example.

+  // CSPs when the resource that triggers this navigation lives in an isolated
+  // world.
+  bool should_bypass_main_world_CSP;

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

Do we have tests that things work properly when this is true, with PlzNavigate
enabled?

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

Yes, I did this to make this test work:
http/tests/security/isolatedWorld/bypass-main-world-csp-iframes.html

I was able to remove the failure expectation from:
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation

[alexmos, 2017/02/14 06:57:20]

Acknowledged.

 };
 
 // Provided by the renderer ----------------------------------------------------