Report invalid URLs as ERR_INVALID_URL rather than ERR_ABORTED

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index e594cb76d5aef0b6545234d41d7bfa1bb0eee8c1..b8828e15670a80a69cce13fa3a9de53b973bb5ff 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -227,16 +227,17 @@ bool IsDetachableResourceType(ResourceType type) {
 // Aborts a request before an URLRequest has actually been created.
 void AbortRequestBeforeItStarts(ResourceMessageFilter* filter,

[mmenke, 2016/04/21 15:28:15]

Abort -> Fail?

[davidben, 2016/04/21 19:00:25]

Done.

                                 IPC::Message* sync_result,
-                                int request_id) {
+                                int request_id,
+                                net::Error error) {
   if (sync_result) {
     SyncLoadResult result;
-    result.error_code = net::ERR_ABORTED;
+    result.error_code = error;
     ResourceHostMsg_SyncLoad::WriteReplyParams(sync_result, result);
     filter->Send(sync_result);
   } else {
     // Tell the renderer that this request was disallowed.
     ResourceMsg_RequestCompleteData request_complete_data;
-    request_complete_data.error_code = net::ERR_ABORTED;
+    request_complete_data.error_code = error;
     request_complete_data.was_ignored_by_handler = false;
     request_complete_data.exists_in_cache = false;
     // No security info needed, connection not established.
@@ -289,15 +290,16 @@ void SetReferrerForRequest(net::URLRequest* request, const Referrer& referrer) {
 }
 
 // Consults the RendererSecurity policy to determine whether the
-// ResourceDispatcherHostImpl should service this request.  A request might be
-// disallowed if the renderer is not authorized to retrieve the request URL or
-// if the renderer is attempting to upload an unauthorized file.
-bool ShouldServiceRequest(int process_type,
-                          int child_id,
-                          const ResourceHostMsg_Request& request_data,
-                          const net::HttpRequestHeaders& headers,
-                          ResourceMessageFilter* filter,
-                          ResourceContext* resource_context) {
+// ResourceDispatcherHostImpl should service this request. Returns net::OK if
+// allowed or a net error otherwise. A request might be disallowed if the
+// renderer is not authorized to retrieve the request URL or if the renderer is
+// attempting to upload an unauthorized file.
+net::Error ShouldServiceRequest(int process_type,
+                                int child_id,
+                                const ResourceHostMsg_Request& request_data,
+                                const net::HttpRequestHeaders& headers,
+                                ResourceMessageFilter* filter,
+                                ResourceContext* resource_context) {
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
 
@@ -305,7 +307,7 @@ bool ShouldServiceRequest(int process_type,
   if (!policy->CanRequestURL(child_id, request_data.url)) {
     VLOG(1) << "Denied unauthorized request for "
             << request_data.url.possibly_invalid_spec();
-    return false;
+    return net::ERR_INVALID_URL;

[mmenke, 2016/04/21 15:28:15]

Also, I think F5 / reload is the reason not to commit an error page with the new
URL...And what about auto-reload?

If we're concerned about invalid URLs, in particular, seems like we could just
let them through, though I suppose then "smart" users could fix deliberately
malformed urls (http;//bad.com, I guess?)  Not sure what our security model is
here.

[mmenke, 2016/04/21 15:28:15]

This seem a fine error code for !url.is_valid(), but not for other errors.

[davidben, 2016/04/21 19:00:25]

It doesn't commit with the new URL, actually. Just the old one. Auto-reload's
fine I think? I don't know off-hand whether it blacklists ERR_INVALID_URL, but
auto-reload's hits tons of errors where it'd be pointless to do so. I don't see
a huge difference between this and any other URL.
Blink seems to reject invalid URLs pretty early, actually. But I think
responding with ERR_INVALID_URL instead of ERR_ABORTED should there be some
codepath that doesn't seems pretty reasonable.

I'm not a huge fan of letting them through. We'll expose more codepaths to
invalid URLs and I'm not sure what it'd do.

[davidben, 2016/04/21 19:00:25]

The other cases are things like magic schemes or special about:foo URLs. It
seems reasonable to treat those as invalid URLs I think.

[mmenke, 2016/04/21 19:10:05]

How does it know not to commit with the new URL?  If I do something like
document.location="http://0.0.0.0:7/spam" the failure URL does indeed
commit...And if it commits the old URL, the user sees "http://evil.com" with an
ERR_INVALID_URL, even if it tried to navigate to something it shouldn't.

[davidben, 2016/04/21 19:20:04]

Oh hrm, that's true. I was thinking redirects where it commits the URL right
before this one. Perhaps we should leave it as ERR_ABORTED then? Though it does
say ERR_INVALID_URL and it's not like a network attacker can't cause random
reasonable URLs to commit with network error pages. (Just trigger a network
error.)

nasko: Thoughts?

[nasko, 2016/04/21 20:30:00]

I had this comment typed up in a previous version of the patch:
---
Can we make it a bit more generic? There are various ways this check can fail?
ERR_NOT_AUTHORIZED? Or is this going to be more invasive change?
I have a similar problem I'm facing in a different context, so having a generic
error for "this renderer asked for something that it isn't allowed to access" is
going to be nice!
---

[davidben, 2016/04/29 21:54:40]

There's ERR_ACCESS_DENIED. The nuisance is that this would fire even for invalid
URLs which would look a little weird. That or we either special-case that
*again* before calling CanRequestURL or make CanRequestURL return a net::Error
(which looks like it'll be rather involved, though I can do it if you like).

I figured that ERR_INVALID_URL worked for what I expected was the common case
(GURL::is_valid failing), and it was half-plausible for the others too. If a
webpage fetches a chrome-extension:// URL it's not allowed to access, some
magical about:inducebrowsercrashforrealz URL, or something of the sort, acting
as if we couldn't parse the URL seemed pretty sound.

One could argue that those magic URLs are browser implementation details and
rather than responding with "Okay, you got me. I *do* understand that URL. But
I'm not gonna let you load it. Bye", we should stick our fingers in our ears
with "What? I don't know what about:inducebrowsercrashforrealz means. Uh. You
must have meant to dial somewhere else..."

Whereas the ResourceRequestBody::Element check is clearly not a check on the
URL, so ERR_ACCESS_DENIED seemed better.

Caveat: I'm not really sure exactly what cases hit this. I was hoping you'd know
it more globally. :-)

   }
 
   // Check if the renderer is using an illegal Origin header.  If so, kill it.
@@ -319,7 +321,7 @@ bool ShouldServiceRequest(int process_type,
                                                        child_id, origin)) {
       VLOG(1) << "Killed renderer for illegal origin: " << origin_string;
       bad_message::ReceivedBadMessage(filter, bad_message::RDH_ILLEGAL_ORIGIN);
-      return false;
+      return net::ERR_INVALID_URL;
     }
   }
 
@@ -333,7 +335,7 @@ bool ShouldServiceRequest(int process_type,
           !policy->CanReadFile(child_id, iter->path())) {
         NOTREACHED() << "Denied unauthorized upload of "
                      << iter->path().value();
-        return false;
+        return net::ERR_ACCESS_DENIED;
       }
       if (iter->type() == ResourceRequestBody::Element::TYPE_FILE_FILESYSTEM) {
         storage::FileSystemURL url =
@@ -341,13 +343,13 @@ bool ShouldServiceRequest(int process_type,
         if (!policy->CanReadFileSystemFile(child_id, url)) {
           NOTREACHED() << "Denied unauthorized upload of "
                        << iter->filesystem_url().spec();
-          return false;
+          return net::ERR_ACCESS_DENIED;
         }
       }
     }
   }
 
-  return true;
+  return net::OK;
 }
 
 void RemoveDownloadFileFromChildSecurityPolicy(int child_id,
@@ -1433,10 +1435,15 @@ void ResourceDispatcherHostImpl::BeginRequest(
   net::HttpRequestHeaders headers;
   headers.AddHeadersFromString(request_data.headers);
 
-  if (is_shutdown_ ||
-      !ShouldServiceRequest(process_type, child_id, request_data, headers,
-                            filter_, resource_context)) {
-    AbortRequestBeforeItStarts(filter_, sync_result, request_id);
+  if (is_shutdown_) {
+    AbortRequestBeforeItStarts(filter_, sync_result, request_id,
+                               net::ERR_ABORTED);
+    return;
+  }
+  net::Error error = ShouldServiceRequest(process_type, child_id, request_data,
+                                          headers, filter_, resource_context);
+  if (error != net::OK) {
+    AbortRequestBeforeItStarts(filter_, sync_result, request_id, error);
     return;
   }
 
@@ -1445,7 +1452,8 @@ void ResourceDispatcherHostImpl::BeginRequest(
                                                   request_data.url,
                                                   request_data.resource_type,
                                                   resource_context)) {
-    AbortRequestBeforeItStarts(filter_, sync_result, request_id);
+    AbortRequestBeforeItStarts(filter_, sync_result, request_id,
+                               net::ERR_ABORTED);
     return;
   }