Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

content/browser/frame_host/render_frame_proxy_host.cc

 // Copyright 2014 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.
 
 #include "content/browser/frame_host/render_frame_proxy_host.h"
 
 #include <utility>
 
 #include "base/lazy_instance.h"
 #include "content/browser/bad_message.h"
@@ skipping 117 matching lines @@
 
 bool RenderFrameProxyHost::OnMessageReceived(const IPC::Message& msg) {
   if (cross_process_frame_connector_.get() &&
       cross_process_frame_connector_->OnMessageReceived(msg))
     return true;
 
   bool handled = true;
   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)
     IPC_MESSAGE_HANDLER(FrameHostMsg_FrameFocused, OnFrameFocused)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 bool RenderFrameProxyHost::InitRenderFrameProxy() {
@@ skipping 113 matching lines @@
 
   // TODO(alexmos, creis): Figure out whether |params.user_gesture| needs to be
   // passed in as well.
   frame_tree_node_->navigator()->RequestTransferURL(
       current_rfh, validated_url, site_instance_.get(), std::vector<GURL>(),
       params.referrer, ui::PAGE_TRANSITION_LINK, GlobalRequestID(),
       params.should_replace_current_entry, params.uses_post ? "POST" : "GET",
       params.resource_request_body);
 }
 
+void RenderFrameProxyHost::OnForwardContentSecurityPolicyViolation(
+    const url::Origin& origin_declaring_violated_csp,
+    const ContentSecurityPolicyViolation& violation) {
+  RenderFrameHostImpl* current_rfh = frame_tree_node_->current_frame_host();
+
+  // Verify that the CSP violation will be reported in the same frame

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

"same frame": Did you mean to say "same document?"  (And I agree with the
parenthetical that we're falling back to origin, since same document is hard.)

[Łukasz Anforowicz, 2016/08/12 18:55:04]

Good point.  Done.

+  // as the one that declared the violated CSP (or at least in a frame
+  // with the same origin).  This check protects against a race when
+  // ForwardContentSecurityPolicyViolation IPC races with navigation
+  // of the frame the IPC is sent to.
+  if (!origin_declaring_violated_csp.IsSameOriginWith(
+          current_rfh->GetLastCommittedOrigin()))

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

I'm wondering if there's something better we can do here, because I'm not very
happy with treating the origin as a proxy for whether we've navigated.  It seems
like there's lots of ways this could go wrong:

1) The origin doesn't change, but we've committed a different page where the CSP
policy doesn't apply anymore.
2) The renderer lies about the origin.  (Not sure if there's a viable attack in
this case, but we have to be careful anytime we hear an origin claim from the
renderer.)
3) We have to be careful that current_rfh's origin is always correct, since
there are cases where we clear some of its navigation state (e.g., after an
ignored navigation).

From one perspective, the document sequence number is a better indication of
whether we've moved to a different page, but the renderer process won't know the
DSN of a remote frame (and I'm pretty sure we don't want to add that).

Another not perfect idea: the renderer could send up the CSP policy it was using
when it noticed the violation, and we can compare it against what the browser
has in the current replication state for the frame?  We would notice if they
differ, which would help with case (1) where the origin doesn't change. 
However, that doesn't help if you go to a different page with the same CSP
policy, which would erroneously get an error.

I'm not sure how else to tell that we've left the page that had the CSP policy.

[Łukasz Anforowicz, 2016/08/12 18:55:04]

Ack.  Things are a bit improved by your suggestion to cross-check CSP contents.
If the renderer lies, then we will ignore the IPC message.  If the renderer
sends a correct origin, then it is indistinguishable from a normal CSP violation
report - OTOH, your remark got me thinking more about potential abuse of the new
renderer->browser message (i.e. thinking about the impact of fake CSP violation
reports a compromised/hostile renderer can send):

1. Since the new message contains |report_endpoints|, a hostile renderer can
start sending CSP reports to endpoints controlled by an attacker.  This can be
seen as a way to bypass CSRF protection (because the reports will be sent with
the Origin header coming from the frame the report gets forwarded to).  If you
are curious to learn more, you can start looking at code around
PingLoader::sendViolationReport.  

Hmmm... I am not sure I understand the threat of CSRF - an attacker can simply
use telnet and send a fake Origin header, instead of coercing the browser to do
it.  I am probably missing something - can you help me learn?

I am comparing against the actual list of report endpoints in patchset #10 -
this at least means that a hostile renderer doesn't control the endpoints
(although it can still spam/forge reports send to the real endpoints).

2. Ability to spam cross-origin console is not a new threat AFAICT - there is
already a pair of FrameHostMsg_AddMessageToConsole and
FrameMsg_AddMessageToConsole.

3. A hostile renderer can raise "securitypolicyviolation" cross-origin.

I am slightly fixing #1 in the patchset #10, but I am not sure if we can protect
against #2 and #3 without moving CSP checks to the browser.
Let's follow-up on this in
https://codereview.chromium.org/2190183002/patch/160001/170004
Ack.
Okay - I am doing this in patchset #9.  The IPC already contains the CSP header
that is violated - we just need to check if this header value is still present
in FrameReplicationState (this gets reset when navigating the frame - see how
NavigatorImpl::DidNavigate calls FrameTreeNode::ResetContentSecurityPolicy).

Is this better (or maybe good enough), or is the direction of this CL doomed?
Yes, it seems to be a difficult problem.


I could possibly get rid of the checks if only I could guarantee that the
forwarding is only happening from child to parent (AFAIK this is true today). 
Unfortunately, when ContentSecurityPolicy::reportViolation is called, it doesn't
know that it was checking something for a child.

[Charlie Reis, 2016/08/12 20:47:30]

I don't see any comments there, so I'll avoid digging deeper at the moment.  :)
Sure.  CSRF attacks are a threat when the attacker can get a victim's machine to
make the request.  Attackers can't make the requests themselves (e.g., with
telnet) because they don't have the victim's cookies on their machine.  When the
victim's browser sends the POST submission, it carries the victim's cookies (or
other ambient authority, like intranet access).

I'm not up to speed on CSP reports being used for CSRF, but I suppose the
attacker could make up an endpoint like victim.com/do_state_change?  (I'd be
surprised if the CSP report that the browser sends could be interpreted as a
valid command other than reporting a CSP error, but maybe there are cases where
that's a problem.)
I suppose it's good enough if we have to defend against this.  I'm just worried
about it being interpreted as a pattern for others to follow when they're
concerned about skipping an action when the frame has been navigated away. 
Maybe we can add some clear comments about what policy we'd like to have, and
why this approach is a reasonable (and necessary) tradeoff in this case.
I would love to leave it out if we can confirm the reports are limited to the
parent/child case.  I've emailed estark@ to see if we can confirm that.

Otherwise, I think the comments I mention above might be sufficient.

+    return;
+
+  // Forward CSP violation report to the frame that declared the CSP.
+  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();
 
   // Only deliver the message if the request came from a RenderFrameHost in the
   // same BrowsingInstance or if this WebContents is dedicated to a browser
   // plugin guest.
   //
   // TODO(alexmos, lazyboy):  The check for browser plugin guest currently
   // requires going through the delegate.  It should be refactored and
@@ skipping 88 matching lines @@
   target_rfh->Send(new FrameMsg_AdvanceFocus(target_rfh->GetRoutingID(), type,
                                              source_proxy_routing_id));
 }
 
 void RenderFrameProxyHost::OnFrameFocused() {
   frame_tree_node_->current_frame_host()->delegate()->SetFocusedFrame(
       frame_tree_node_, GetSiteInstance());
 }
 
 }  // namespace content