Block redirects to renderer-debug urls.

content/browser/frame_host/navigation_request.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/navigation_request.h"
 
 #include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
@@ skipping 19 matching lines @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_data.h"
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/stream_handle.h"
 #include "content/public/common/appcache_info.h"
+#include "content/public/common/child_process_host.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/common/request_context_type.h"
 #include "content/public/common/resource_request_body.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/common/web_preferences.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/url_util.h"
@@ skipping 443 matching lines @@
 }
 
 void NavigationRequest::TransferNavigationHandleOwnership(
     RenderFrameHostImpl* render_frame_host) {
   render_frame_host->SetNavigationHandle(std::move(navigation_handle_));
 }
 
 void NavigationRequest::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
     const scoped_refptr<ResourceResponse>& response) {
+  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRedirectToURL(
+          redirect_info.new_url)) {
+    DVLOG(1) << "Denied unauthorized request (redirect) for "
+             << redirect_info.new_url.possibly_invalid_spec();
+    // TODO(arthursonzogni): Consider switching to net::ERR_UNSAFE_REDIRECT
+    // when PlzNavigate is launched.
+    navigation_handle_->set_net_error_code(net::ERR_ABORTED);
+    frame_tree_node_->ResetNavigationRequest(false, true);
+    return;
+  }
+
   // If a redirect occurs, the original site instance we thought is the
   // destination could change.
   dest_site_instance_ = nullptr;
 
   // If the navigation is no longer a POST, the POST data should be reset.
   if (redirect_info.new_method != "POST")
     common_params_.post_data = nullptr;
 
   // Mark time for the Navigation Timing API.
   if (request_params_.navigation_timing.redirect_start.is_null()) {
@@ skipping 18 matching lines @@
   // otherwise block.
   if (CheckContentSecurityPolicyFrameSrc(true /* is redirect */) ==
       CONTENT_SECURITY_POLICY_CHECK_FAILED) {
     OnRequestFailed(false, net::ERR_BLOCKED_BY_CLIENT);
 
     // DO NOT ADD CODE after this. The previous call to OnRequestFailed has
     // destroyed the NavigationRequest.
     return;
   }
 
-  // For non browser initiated navigations we need to check if the source has
-  // access to the URL. We always allow browser initiated requests.
-  // TODO(clamy): Kill the renderer if FilterURL fails?
-  GURL url = common_params_.url;
-  if (!browser_initiated_ && source_site_instance()) {
-    source_site_instance()->GetProcess()->FilterURL(false, &url);

[Charlie Reis, 2017/07/07 17:13:00]

I don't see how it's ok to remove the FilterURL call.  It seems indirect to
assume that everything FilterURL would catch will be caught by CanRedirectToURL,
and I'm not sure if it's true.

Is there a reason this needed to be removed?

[arthursonzogni, 2017/07/10 16:07:04]

I think we should do both:
* CanRedirectToURL(url) for every redirect.
* CanRequestURL(process, url) for non-browser initiated requests.

1) I will keep this check, but I would prefer putting it at the top of this
function. I don't want to update the NavigationRequest for a redirect that isn't
authorized. Preventing us from reusing these data by mistake is safer.

2) I think that CanRequestURL is more appropriate than FilterURL. Both are the
same, except that FilterURL will modify |url| instead of returning false.
At this location, it uses a temporary variable in order not to modify an
existing one and does the check |url == url::kAboutBlankURL| in order to get the
original result of CanRequestURL. That is weird.

3) I will remove the comment that talks about CSP. AFAIK, CSP aren't involved in
FilterURL/CanRequestURL.

[Charlie Reis, 2017/07/10 21:16:21]

Thanks!  This sounds reasonable to me.

-    // FilterURL sets the URL to about:blank if the CSP checks prevent the
-    // renderer from accessing it.
-    if ((url == url::kAboutBlankURL) && (url != common_params_.url)) {
-      navigation_handle_->set_net_error_code(net::ERR_ABORTED);
-      frame_tree_node_->ResetNavigationRequest(false, true);
-      return;
-    }
-  }
-
   // Compute the SiteInstance to use for the redirect and pass its
   // RenderProcessHost if it has a process. Keep a reference if it has a
   // process, so that the SiteInstance and its associated process aren't deleted
   // before the navigation is ready to commit.
   scoped_refptr<SiteInstance> site_instance =
       frame_tree_node_->render_manager()->GetSiteInstanceForNavigationRequest(
           *this);
   speculative_site_instance_ =
       site_instance->HasProcess() ? site_instance : nullptr;
 
@@ skipping 442 matching lines @@
           CSPDirective::FrameSrc, common_params_.url, is_redirect,
           common_params_.source_location.value_or(SourceLocation()),
           CSPContext::CHECK_ENFORCED_CSP)) {
     return CONTENT_SECURITY_POLICY_CHECK_PASSED;
   }
 
   return CONTENT_SECURITY_POLICY_CHECK_FAILED;
 }
 
 }  // namespace content