Report invalid URLs as ERR_INVALID_URL rather than ERR_ABORTED

content/browser/loader/resource_dispatcher_host_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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 207 matching lines @@
     case RESOURCE_TYPE_PREFETCH:
     case RESOURCE_TYPE_PING:
     case RESOURCE_TYPE_CSP_REPORT:
       return true;
     default:
       return false;
   }
 }
 
 // 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.
     request_complete_data.completion_time = base::TimeTicks();
     request_complete_data.encoded_data_length = 0;
     filter->Send(new ResourceMsg_RequestComplete(
         request_id, request_complete_data));
   }
 }
 
@@ skipping 32 matching lines @@
       break;
     case blink::WebReferrerPolicyNoReferrerWhenDowngradeOriginWhenCrossOrigin:
       net_referrer_policy = net::URLRequest::
           REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN;
       break;
   }
   request->set_referrer_policy(net_referrer_policy);
 }
 
 // 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();
 
   // Check if the renderer is permitted to request the requested URL.
   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.
   std::string origin_string;
   bool has_origin = headers.GetHeader("Origin", &origin_string) &&
                     origin_string != "null";
   if (has_origin) {
     GURL origin(origin_string);
     if (!policy->CanCommitURL(child_id, origin) ||
         GetContentClient()->browser()->IsIllegalOrigin(resource_context,
                                                        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;
     }
   }
 
   // Check if the renderer is permitted to upload the requested files.
   if (request_data.request_body.get()) {
     const std::vector<ResourceRequestBody::Element>* uploads =
         request_data.request_body->elements();
     std::vector<ResourceRequestBody::Element>::const_iterator iter;
     for (iter = uploads->begin(); iter != uploads->end(); ++iter) {
       if (iter->type() == ResourceRequestBody::Element::TYPE_FILE &&
           !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 =
             filter->file_system_context()->CrackURL(iter->filesystem_url());
         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,
                                                const base::FilePath& path) {
   ChildProcessSecurityPolicyImpl::GetInstance()->RevokeAllPermissionsForFile(
       child_id, path);
 }
 
 int GetCertID(CertStore* cert_store, net::URLRequest* request, int child_id) {
   if (request->ssl_info().cert.get())
@@ skipping 1065 matching lines @@
   filter_->GetContexts(request_data.resource_type, request_data.origin_pid,
                        &resource_context, &request_context);
   // http://crbug.com/90971
   CHECK(ContainsKey(active_resource_contexts_, resource_context));
 
   // Parse the headers before calling ShouldServiceRequest, so that they are
   // available to be validated.
   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;
   }
 
   // Allow the observer to block/handle the request.
   if (delegate_ && !delegate_->ShouldBeginRequest(request_data.method,
                                                   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;
   }
 
   // Construct the request.
   std::unique_ptr<net::URLRequest> new_request = request_context->CreateRequest(
       is_navigation_stream_request ? request_data.resource_body_stream_url
                                    : request_data.url,
       request_data.priority, nullptr);
 
   // PlzNavigate: Always set the method to GET when gaining access to the
@@ skipping 1229 matching lines @@
   ssl.cert_id = GetCertStore()->StoreCert(ssl_info.cert.get(), child_id);
   response->head.security_info = SerializeSecurityInfo(ssl);
 }
 
 CertStore* ResourceDispatcherHostImpl::GetCertStore() {
   return cert_store_for_testing_ ? cert_store_for_testing_
                                  : CertStore::GetInstance();
 }
 
 }  // namespace content