Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

content/browser/frame_host/render_frame_proxy_host.cc

Index: content/browser/frame_host/render_frame_proxy_host.cc
diff --git a/content/browser/frame_host/render_frame_proxy_host.cc b/content/browser/frame_host/render_frame_proxy_host.cc
index 63be6ebbcc661af2c504d123b99cdc111c002216..8f1d9d5c3a663f7d69e89de73f550b445ad37950 100644
--- a/content/browser/frame_host/render_frame_proxy_host.cc
+++ b/content/browser/frame_host/render_frame_proxy_host.cc
@@ -135,6 +135,8 @@ bool RenderFrameProxyHost::OnMessageReceived(const IPC::Message& msg) {
   IPC_BEGIN_MESSAGE_MAP(RenderFrameProxyHost, msg)
     IPC_MESSAGE_HANDLER(FrameHostMsg_Detach, OnDetach)
     IPC_MESSAGE_HANDLER(FrameHostMsg_OpenURL, OnOpenURL)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_ForwardContentSecurityPolicyViolation,
+                        OnForwardContentSecurityPolicyViolation)
     IPC_MESSAGE_HANDLER(FrameHostMsg_RouteMessageEvent, OnRouteMessageEvent)
     IPC_MESSAGE_HANDLER(FrameHostMsg_DidChangeOpener, OnDidChangeOpener)
     IPC_MESSAGE_HANDLER(FrameHostMsg_AdvanceFocus, OnAdvanceFocus)
@@ -268,6 +270,18 @@ void RenderFrameProxyHost::OnOpenURL(
       params.resource_request_body);
 }
 
+void RenderFrameProxyHost::OnForwardContentSecurityPolicyViolation(
+    const ContentSecurityPolicyViolation& violation) {
+  // Verify that we are in the same BrowsingInstance as the current
+  // RenderFrameHost.
+  RenderFrameHostImpl* current_rfh = frame_tree_node_->current_frame_host();
+  if (!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))

[alexmos, 2016/08/09 18:01:19]

What's the reason for this check?

[Łukasz Anforowicz, 2016/08/09 22:23:20]

Thanks for asking this question - I blindly copied this check from
RenderFrameProxyHost::OnOpenURL.  I assumed that this check is making sure that
RFPH and RFH are in sync (and drops the message if RFH has navigated away).

Does the above make sense?  Let's chat to figure out what should be done here.

[Łukasz Anforowicz, 2016/08/10 19:50:25]

FWIW, I checked that the same condition in RenderFrameProxyHost::OnOpenURL has
been introduced in https://codereview.chromium.org/1715993002 (by alexmos@) - it
replaces a TODO(creis) that also points at NavigatorImpl::RequestOpenURL.

[alexmos, 2016/08/10 20:34:33]

Yes, I moved that check according to Charlie's TODO.  The actual check traces
back to Charlie's https://chromiumcodereview.appspot.com/10350013 and
http://crbug.com/126174.  

Two thoughts about this.  Currently, this will only be used to forward
violations to the parent frame (right?).  If the parent commits a navigation to
another BrowsingInstance, that should cause the children to be deleted at that
point, which should currently cause any subsequent messages from them to be
dropped, but that might change once we implement pending delete FTNs
(http://crbug.com/609963).  So I think it's ok to keep this check here, just to
ensure that we won't try to report a violation to a WebUI page in a race, etc.

Second, if this ever gets used to also report the violation in a popup where the
CSP lives in the opener, then it seems we'd also need this check for scenarios
like the one in http://crbug.com/126174.  So, another reason to keep it.

Charlie, does that sound about right?  Any thoughts about whether we need this
check here?

[Łukasz Anforowicz, 2016/08/11 17:32:36]

Hmmm... actually, I think that the check above might be insufficient. 
Navigation can succeed as long as RFH is in the same site instance -so this kind
of check works okay for RenderFrameProxyHost::OnOpenURL.  OTOH, we probably
should NOT report a CSP violation if a frame has been navigated to another
origin (while racing with the CSP violation message from RFP).  I am not sure
what is the best way for enforcing this - maybe the IPC can include
|csp_self_origin| and here we can verify if it is still the same as RFH's
origin?  WDYT?

FWIW, I looked at other RenderFrameProxyHost::On* methods and most seem not to
care about the current origin of RFH - I think the concern raised here only
applies to the new IPC.

[Łukasz Anforowicz, 2016/08/11 18:04:35]

I talked with Alex about this a little bit more and:

- Navigating a frame should delete all child frames, so the concern about
reporting a different origin's CSP violation in a wrong frame doesn't apply to
this scenario (this scenario = CSP violation detected via remote parent).

- OTOH, the general concern remains in case RemoteContentSecurityPolicy is not
associated with a parent frame, but (for example) with "opener".  I am not sure
if such scenario is possible today, but I have a feeling that estark@ wants to
depend on proper behavior in this scenario when fixing https://crbug.com/630332.

[Charlie Reis, 2016/08/11 18:32:14]

Sorry for chiming in late.  I'll reply to Alex's question first.

The check in RFPH::OnOpenURL is there so that a frame can't initiate a
navigation in a frame from a different BrowsingInstance.  (Note that this is
different from whether it's cross-origin or not.  We only swap BrowsingInstances
when we're doing a complete privilege level change, like the user typing in
chrome://settings after being on a web page.)

I don't think we can ever have frames from different BrowsingInstances in the
same frame tree, but this matters in the case of a popup/opener where one gets
navigated to a WebUI URL, etc.  We basically sever postMessage, navigation, and
other script relationships at that point.

It's not clear to me whether CSP reporting needs to follow this model.  If it
only happens for frames within the same frame tree, it's probably moot since
we'll never see frames from a different BrowsingInstance.  If it's possible that
we could forward a report across tabs, then it becomes a more interesting
question-- can a CSP policy affect a popup tab after it has left the
BrowsingInstance?  (Actually, can CSP policies affect popups at all, or are
there plans for that?)

I would imagine that CSP would stop applying if the user navigated the tab to a
different BrowsingInstance, in which case we wouldn't need to forward the
violation.
Not sure I follow this part.  Navigation can work across SiteInstances as well,
as long as we're in the same BrowsingInstance.  That's what RFPH::OpenURL is
for.
I do see the concern here, and it's related to Alex's pending delete FTN
question.  I agree that we don't want to deliver a report to a RFH just after
we've left the page it was intended for.

I *think* it's the case that we don't have this problem today.  For same-site
navigations, the parent RFH will detach its children before committing the new
page, so the FTN will be gone.  For cross-site navigations, we clear the
children FTNs when swapping.  In both cases, no one will be listening on the
browser side when the report IPC arrives.  Correct me if I'm wrong.

If we add pending delete FTNs, we have to worry about this.  Nick and I have
been chatting about an alternative to pending delete FTNs, though, which might
allow us to avoid races like this.  (I won't go into it in depth here, but it
basically involves a Mojo channel to NavigationController for PageState, and
ignoring most other things during unload.)  I'm inclined to say we should post
this concern to bug 609963 and not worry about it here for now.

Just for a sanity check, though, is it the case that this message only goes from
a child to its parent, or are there other cases it could be sent?

[Łukasz Anforowicz, 2016/08/11 19:51:35]

Today sending of this message is only triggered when a child violates policy of
a (remote) parent.  But... in the future the message might be sent in other
cases - specifically, https://crbug.com/630332 talks about "form-action" CSP
directive.  In case when form element targets another window by name (*) the CSP
to verify can come from an arbitrary remote frame.

So - in the latest patchset I am trying to compare origins, to make sure they
are still the same.  One thing that worries me is the "LastCommitted" part of
GetLastCommittedOrigin, but hopefully this is still correct.  Can you please
take a look?

(*) LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
shows how form.target attribute can be used to target another frame.  I assume
that this attribute can also be used to target another window.

[Charlie Reis, 2016/08/11 20:40:26]

Hmm, I have some concerns about that, and no solutions yet.  I'll leave comments
on that patch set.

+    return;
+
+  current_rfh->Send(new FrameMsg_ReportContentSecurityPolicyViolation(
+      current_rfh->GetRoutingID(), violation));
+}
+
 void RenderFrameProxyHost::OnRouteMessageEvent(
     const FrameMsg_PostMessage_Params& params) {
   RenderFrameHostImpl* target_rfh = frame_tree_node()->current_frame_host();