Block redirects to renderer-debug urls.

content/browser/frame_host/navigation_request.cc

Index: content/browser/frame_host/navigation_request.cc
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index ba72304cea3d0d934f5d8ff0de3e8b4413035f01..b2fe441aff4228a9becef07f49b118792fb6bc6e 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -37,6 +37,7 @@
 #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"
@@ -500,6 +501,17 @@ void NavigationRequest::TransferNavigationHandleOwnership(
 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;
@@ -538,21 +550,6 @@ void NavigationRequest::OnRequestRedirected(
     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