Block redirects to renderer-debug urls.

content/browser/child_process_security_policy_impl.cc

 // Copyright (c) 2012 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/child_process_security_policy_impl.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 649 matching lines @@
 
   // If the process can commit the URL, it can request it.
   if (CanCommitURL(child_id, url))
     return true;
 
   // Also allow URLs destined for ShellExecute and not the browser itself.
   return !GetContentClient()->browser()->IsHandledURL(url) &&
          !net::URLRequest::IsHandledURL(url);
 }
 
+bool ChildProcessSecurityPolicyImpl::CanRedirectToURL(const GURL& url) {

[Charlie Reis, 2017/07/07 17:12:59]

It makes me nervous to be doing a narrower version of CanRequestURL here,
especially with no comments pointing out the similarities and differences.  If
we change one in the future, we may need to change both.

I'm not sure I understand why we can't use CanRequestURL on the initiating
process?  If that process isn't allowed to request the post-redirect URL, then
we probably shouldn't be allowing it.  (The destination process seems
unimportant, but I could be wrong.)

[clamy, 2017/07/10 12:29:19]

The issue is that we need to block redirects to renderer debug urls for all kind
of navigations. This include browser-initiated navigations and
renderer-initiated navigations that went through the OpenURL path. In those two
cases, it makes no sense to call CanRequestURL with the current process, since
it's quite possible that it will not be the process that will commit them. And
in the case of browser-initiated navigations, this process did not even
initiated the navigation in the first place. This is why I wanted Arthur to
write a function that could check whether a redirect was allowed regardless of
the process instead of calling CanRequestURL.

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

I'm happy with where we ended up.  I think there's still a reason to call
CanRequestURL on renderer-initiated navigations, but I agree that we want some
additional checks that also apply to browser-initiated navigations where there's
no initiating process.

+  if (!url.is_valid())
+    return false;  // Can't redirect to invalid URLs.
+
+  const std::string& scheme = url.scheme();
+
+  if (IsPseudoScheme(scheme)) {
+    // Redirects to a pseudo scheme (about, javascript, view-source, ...) are
+    // not allowed. An exception is made for <about:blank> and its variations.
+    return url.IsAboutBlank();
+  }
+
+  // Redirects to blob-url or filesystem-url are not allowed.
+  if (url.SchemeIsBlob() || url.SchemeIsFileSystem())
+    return false;
+
+  return IsWebSafeScheme(scheme);

[Charlie Reis, 2017/07/07 17:12:59]

We're basically skipping the CanCommitURL and IsHandledURL checks from
CanRequestURL.  Are we sure this won't break things?  We tend to find
regressions after introducing restrictions like this, and it's not clear to me
if this is a safe assumption.  (e.g., Could some non-web-safe schemes redirect
to each other from the right process type?)

[clamy, 2017/07/10 12:29:19]

As explained above, we can't really use the process in that check. Would you
prefer if we just remove this check and always return true instead? This is what
we're doing in PlzNavigate currently.

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

Sure.  If we need to, we can tighten it in a separate CL.

+}
+
 bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
                                                   const GURL& url) {
   if (!url.is_valid())
     return false;  // Can't commit invalid URLs.
 
   // Of all the pseudo schemes, only about:blank and about:srcdoc are allowed to
   // commit.
   if (IsPseudoScheme(url.scheme()))
     return url == url::kAboutBlankURL || url == kAboutSrcDocURL;
 
@@ skipping 463 matching lines @@
   return found;
 }
 
 void ChildProcessSecurityPolicyImpl::RemoveIsolatedOriginForTesting(
     const url::Origin& origin) {
   base::AutoLock lock(lock_);
   isolated_origins_.erase(origin);
 }
 
 }  // namespace content