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

content/browser/frame_host/frame_tree_node.h

 // Copyright 2013 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.
 
 #ifndef CONTENT_BROWSER_FRAME_HOST_FRAME_TREE_NODE_H_
 #define CONTENT_BROWSER_FRAME_HOST_FRAME_TREE_NODE_H_
 
 #include <stddef.h>
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "content/browser/frame_host/frame_tree_node_blame_context.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/frame_host/render_frame_host_manager.h"
 #include "content/common/content_export.h"
-#include "content/common/content_security_policy/csp_policy.h"
+#include "content/common/content_security_policy/csp_context.h"
 #include "content/common/frame_owner_properties.h"
 #include "content/common/frame_replication_state.h"
 #include "third_party/WebKit/public/platform/WebInsecureRequestPolicy.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
 namespace content {
 
 class FrameTree;
 class NavigationRequest;
@@ skipping 140 matching lines @@
   // Add CSP header to replication state, notify proxies about the update and
   // enforce it on the browser.
   void AddContentSecurityPolicy(const ContentSecurityPolicyHeader& header,
                                 const std::vector<CSPPolicy>& policies);
 
   // Discards previous CSP headers and notifies proxies about the update.
   // Typically invoked after committing navigation to a new document (since the
   // new document comes with a fresh set of CSP http headers).
   void ResetContentSecurityPolicy();
 
+  const std::vector<CSPPolicy>& ContentSecurityPolicies() const {

[nasko, 2017/02/11 00:01:23]

This should be hacker_case(), as it is a simple accessor.

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

Okay, I didn't know about this coding style rule.

+    return csp_policies_;
+  }
+
+  // Return the Content-Security-Policy context associated to this frame.
+  // Never null.
+  CSPContext* ContentSecurityPolicyContext() { return csp_context_.get(); }

[nasko, 2017/02/11 00:01:23]

Same here, hacker_case().

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

Done.

+
   // Sets the current insecure request policy, and notifies proxies about the
   // update.
   void SetInsecureRequestPolicy(blink::WebInsecureRequestPolicy policy);
 
   // Returns the currently active sandbox flags for this frame.  This includes
   // flags inherited from parent frames and the currently active flags from the
   // <iframe> element hosting this frame.  This does not include flags that
   // have been updated in an <iframe> element but have not taken effect yet;
   // use pending_sandbox_flags() for those.
   blink::WebSandboxFlags effective_sandbox_flags() const {
@@ skipping 208 matching lines @@
   base::TimeTicks last_focus_time_;
 
   // A helper for tracing the snapshots of this FrameTreeNode and attributing
   // browser process activities to this node (when possible).  It is unrelated
   // to the core logic of FrameTreeNode.
   FrameTreeNodeBlameContext blame_context_;
 
   // A set of Content-Security-Policies to enforce on the browser-side.
   std::vector<CSPPolicy> csp_policies_;
 
+  // Used to check if a frame is allowed to navigate to an URL according to a
+  // set of content-security-policy.
+  std::unique_ptr<CSPContext> csp_context_;

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

I'm wondering whether it'd be better to associate csp_context_ and csp_policies_
with RFH instead of FTN?  I'm concerned we could end up checking the wrong
frame's CSP.  E.g., if the current RFH has a CSP, then it becomes a pending
delete RFH and adds an iframe which needs to check frame-src, this might end up
checking the wrong RFH's CSP policy (current RFH's, rather than pending delete
RFH's).  We've had this kind of issue with URLs and origins and ended up moving
both from FTN to RFH (see issues 590034, 590035, 663740).

[clamy, 2017/02/13 13:23:28]

Can the frame navigate while the RFH is in pending-delete mode? I thought we
filtered navigation IPCs coming from it while it was in that mode (and if we
don't we should). Considering we only check CSP when navigating, this should be
fine?

[alexmos, 2017/02/14 05:44:28]

Yes, I wasn't sure how much was disallowed in unload handlers currently, but
after double-checking with dcheng@, unload handlers should not be able to create
new child frames or navigate.  So I agree this isn't a concern then.

I wonder if there could be other races though.  E.g., suppose we have A-embed-B
with B in an OOPIF, and A navigates to C.  In parallel with the commit for C, B
navigates to D (before B's process knows that its parent is about to go away),
with that IPC arriving just after C commits (and before A's children are cleaned
up).  In the browser, will we try to check frame-src for D (which presumably
would need A's CSP and not C's CSP), or will we recognize that its parent RFH A
is pending deletion and abort the navigation?  Not sure whether this is actually
possible today; if the navigation to D does make it to browser process, the
right thing to do is probably to abort it if any of the ancestor RFHs are
pending deletion to match what Blink does (i.e., we should ensure that we never
need to check CSP frame-src using a pending delete RFH).  And perhaps it might
be worthwhile to add something like
CHECK(navigating_rfh->GetParent()->is_active()) when enforcing frame-src?

[nasko, 2017/02/15 21:28:44]

I also think that the CSP is better off associated with RFH instead of FTN. If
we had a document concept, that would've been even better, but RFH is the
closest we have.

+
   DISALLOW_COPY_AND_ASSIGN(FrameTreeNode);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_FRAME_TREE_NODE_H_