Forwarding POST body into renderer after a cross-site transfer.

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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/navigation_handle_impl.h"
 
 #include <utility>
 
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/navigator_delegate.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_navigation_handle.h"
 #include "content/common/frame_messages.h"
+#include "content/common/resource_request_body.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
 #include "net/url_request/redirect_info.h"
 
 namespace content {
 
 namespace {
 
 void UpdateThrottleCheckResult(
@@ skipping 112 matching lines @@
   if (frame_tree_node_->IsMainFrame())
     return FrameTreeNode::kFrameTreeNodeInvalidId;
 
   return frame_tree_node_->parent()->frame_tree_node_id();
 }
 
 const base::TimeTicks& NavigationHandleImpl::NavigationStart() {
   return navigation_start_;
 }
 
-bool NavigationHandleImpl::IsPost() {
+const std::string& NavigationHandleImpl::GetMethod() {
   CHECK_NE(INITIAL, state_)
       << "This accessor should not be called before the request is started.";
-  return method_ == "POST";
+  return method_;
 }
 
 const Referrer& NavigationHandleImpl::GetReferrer() {
   CHECK_NE(INITIAL, state_)
       << "This accessor should not be called before the request is started.";
   return sanitized_referrer_;
 }
 
 bool NavigationHandleImpl::HasUserGesture() {
   CHECK_NE(INITIAL, state_)
@@ skipping 82 matching lines @@
 }
 
 NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CallWillStartRequestForTesting(
     bool is_post,
     const Referrer& sanitized_referrer,
     bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol) {
   NavigationThrottle::ThrottleCheckResult result = NavigationThrottle::DEFER;
-  WillStartRequest(is_post ? "POST" : "GET", sanitized_referrer,
+
+  scoped_refptr<ResourceRequestBody> resource_request_body;
+  std::string method;
+  if (is_post) {
+    method = "POST";
+
+    std::string body = "test=body";
+    resource_request_body = new ResourceRequestBody();
+    resource_request_body->AppendBytes(body.data(), body.size());
+  } else {
+    method = "GET";

[Charlie Reis, 2016/05/31 21:08:36]

Maybe we can just initialize |method| to "GET" and leave out the else block? 
Leaving it uninitialized above always makes me nervous when people might add
another branch to the if/else (or make it an else if) and forget to initialize
it.

[Łukasz Anforowicz, 2016/05/31 22:40:55]

Done.

+    resource_request_body = nullptr;
+  }
+
+  WillStartRequest(method, resource_request_body, sanitized_referrer,
                    has_user_gesture, transition, is_external_protocol,
                    base::Bind(&UpdateThrottleCheckResult, &result));
 
   // Reset the callback to ensure it will not be called later.
   complete_callback_.Reset();
   return result;
 }
 
 NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CallWillRedirectRequestForTesting(
@@ skipping 18 matching lines @@
 
 void NavigationHandleImpl::InitServiceWorkerHandle(
     ServiceWorkerContextWrapper* service_worker_context) {
   DCHECK(IsBrowserSideNavigationEnabled());
   service_worker_handle_.reset(
       new ServiceWorkerNavigationHandle(service_worker_context));
 }
 
 void NavigationHandleImpl::WillStartRequest(
     const std::string& method,
+    const scoped_refptr<content::ResourceRequestBody>& resource_request_body,
     const Referrer& sanitized_referrer,
     bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol,
     const ThrottleChecksFinishedCallback& callback) {
+  // |method != "POST"| should imply absence of |resource_request_body|.
+  DCHECK(method == "POST" || !resource_request_body);

[Charlie Reis, 2016/05/31 21:08:36]

Maybe we should upgrade this to a CHECK?  Seems like we should be careful not to
accidentally send a POST body to the renderer due to unrelated bugs.

[Łukasz Anforowicz, 2016/05/31 22:40:55]

Maybe we should try this in a follow-up, smaller (= more easily revertable) CL?


I also have a few questions about adding the CHECK:

1. Should we promote the CommonNavigationParams DCHECK instead (this is where we
construct the IPC payload for renderer;  above it just network stack processing
[which also covers subresources (?) or other things where http body won't get
sent to the renderer process]).

2. I agree with crashing the browser if we detect an obviously security problem
(i.e. browser is confused about origin of frames).  Is sending of POST body the
same level of a security issue (i.e. something that we want to crash upon)?


Also - when thinking about this, I've found out that only GET and POST are
supported for page navigations (so - i.e. we don't have to worry about not
CHECKing/crashing for PUT [which would contain a HTTP body]).  So - I wonder if
I should maybe prefer |bool uses_post| over |std::string method| in my other CLs
(i.e. in https://codereview.chromium.org/2022483003/).

[Charlie Reis, 2016/06/01 23:46:32]

Sure.  Any CHECKs we add can happen in a separate, revertable CL.
Yes, that seems like the right place.
Assuming that a renderer can't lie to make this happen (and that it requires a
browser process bug to trigger), then yes, I think we should crash the browser
if it's about to send a POST to a renderer that shouldn't receive it.  That
risks information disclosure (violating our Site Isolation goals) in some cases,
and in all cases indicates a logic bug that might otherwise be concerning.  I
think that's worthwhile to verify.
See my other response.  :)

[Łukasz Anforowicz, 2016/06/02 22:07:04]

Ack.
Hmmm... maybe the renderer can manage to send a body for a non-POST method.  I
am not sure if this is possible (feels like something that we should already vet
before starting the request), but I've added some checks against this in the
latest patchset in NavigationHandleImpl::WillStartRequest (symmetrical / similar
to the check already present in NavigationHandleImpl::WillRedirectRequest).

+
   // Update the navigation parameters.
   method_ = method;
+  resource_request_body_ = resource_request_body;
   sanitized_referrer_ = sanitized_referrer;
   has_user_gesture_ = has_user_gesture;
   transition_ = transition;
   is_external_protocol_ = is_external_protocol;
 
   state_ = WILL_SEND_REQUEST;
   complete_callback_ = callback;
 
   // Register the navigation throttles. The ScopedVector returned by
   // GetNavigationThrottles is not assigned to throttles_ directly because it
@@ skipping 23 matching lines @@
     scoped_refptr<net::HttpResponseHeaders> response_headers,
     const ThrottleChecksFinishedCallback& callback) {
   // Update the navigation parameters.
   url_ = new_url;
   method_ = new_method;
   sanitized_referrer_.url = new_referrer_url;
   sanitized_referrer_ = Referrer::SanitizeForRequest(url_, sanitized_referrer_);
   is_external_protocol_ = new_is_external_protocol;
   response_headers_ = response_headers;
   was_redirected_ = true;
+  if (new_method != "POST")
+    resource_request_body_ = nullptr;
 
   state_ = WILL_REDIRECT_REQUEST;
   complete_callback_ = callback;
 
   // Notify each throttle of the request.
   NavigationThrottle::ThrottleCheckResult result = CheckWillRedirectRequest();
 
   // If the navigation is not deferred, run the callback.
   if (result != NavigationThrottle::DEFER)
     RunCompleteCallback(result);
@@ skipping 153 matching lines @@
   complete_callback_.Reset();
 
   if (!callback.is_null())
     callback.Run(result);
 
   // No code after running the callback, as it might have resulted in our
   // destruction.
 }
 
 }  // namespace content