content/browser/frame_host/frame_tree_node.h - Issue 2655463006: PlzNavigate: Enforce 'frame-src' CSP on the browser.

Unified Diff: content/browser/frame_host/frame_tree_node.h

Issue 2655463006: PlzNavigate: Enforce 'frame-src' CSP on the browser. (Closed)

Patch Set: Fix tests. Created 3 years, 10 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

« content/browser/frame_host/csp_context_impl.cc ('K') | « content/browser/frame_host/csp_context_impl.cc ('k') | content/browser/frame_host/frame_tree_node.cc » ('j') | content/browser/frame_host/render_frame_host_impl.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/browser/frame_host/frame_tree_node.h

diff --git a/content/browser/frame_host/frame_tree_node.h b/content/browser/frame_host/frame_tree_node.h

index edb1033140aeb2b5cc1bd089bf67661f0a3e7a27..0c13684f6f31b6e6a37c59bb476f3be98e0732ef 100644

--- a/content/browser/frame_host/frame_tree_node.h

+++ b/content/browser/frame_host/frame_tree_node.h

@@ -17,7 +17,7 @@

#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"

@@ -178,6 +178,14 @@ class CONTENT_EXPORT FrameTreeNode {

// 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 ac

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);

@@ -406,6 +414,10 @@ class CONTENT_EXPORT FrameTreeNode {

// 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

clamy 2017/02/13 13:23:28 Can the frame navigate while the RFH is in pending

alexmos 2017/02/14 05:44:28 Yes, I wasn't sure how much was disallowed in unlo

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

On 2017/02/14 05:44:28, alexmos (OOO until 2-21) wrote: > On 2017/02/13 13:23:28, clamy wrote: > > On 2017/02/10 22:59:53, alexmos wrote: > > > 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). > > > > 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? > > 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?

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);

};