Disallow navigations to blob URLs with non-canonical origins.

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 545 matching lines @@
 void ChildProcessSecurityPolicyImpl::RevokeReadRawCookies(int child_id) {
   base::AutoLock lock(lock_);
 
   SecurityStateMap::iterator state = security_state_.find(child_id);
   if (state == security_state_.end())
     return;
 
   state->second->RevokeReadRawCookies();
 }
 
 bool ChildProcessSecurityPolicyImpl::CanRequestURL(

[Charlie Reis, 2016/09/20 16:28:55]

We have pretty extensive unit tests for this in
child_process_security_policy_unittest.cc.  Can you toss one in for the spoof
case for completeness?

[ncarter (slow), 2016/09/20 23:01:43]

Thanks for suggesting this! It uncovered the fact that I meant to fix up
CanCommitURL too. I copypasted that clause there too.

[Charlie Reis, 2016/09/20 23:19:06]

Great!  I agree that having the check in CanCommitURL as well seems better.

     int child_id, const GURL& url) {
   if (!url.is_valid())
     return false;  // Can't request invalid URLs.
 
   if (IsPseudoScheme(url.scheme())) {
     // Every child process can request <about:blank>.
     if (base::LowerCaseEqualsASCII(url.spec(), url::kAboutBlankURL))
       return true;
     // URLs like <about:version>, <about:crash>, <view-source:...> shouldn't be
     // requestable by any child process.  Also, this case covers
     // <javascript:...>, which should be handled internally by the process and
     // not kicked up to the browser.
     return false;
   }
 
+  // https://crbug.com/646278 Valid blob URLs should contain canonically
+  // serialized origins.
+  if (url.SchemeIsBlob() &&

[Charlie Reis, 2016/09/20 16:28:55]

Sanity check: Do we have the same problem for filesystem URLs?

[ncarter (slow), 2016/09/20 17:46:55]

Potentially but the spoof doesn't directly translate; since for filesystem URLs,
GURL actually parses past the scheme.

The behavior I see is that authorities are preserved in inner URLs of filesystem
URLs, but the inner URL is parsed and canonicalized, so spaces get escaped to
%20. We probably should not allow authorities inside of filesystem URLs, but it
may be a spec question.

[Charlie Reis, 2016/09/20 23:19:06]

Acknowledged.

+      !base::StartsWith(url.GetContent(), url::Origin(url).Serialize() + "/",
+                        base::CompareCase::INSENSITIVE_ASCII)) {
+    return false;
+  }
+
   // 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::CanCommitURL(int child_id,
@@ skipping 259 matching lines @@
   base::AutoLock lock(lock_);
 
   SecurityStateMap::iterator state = security_state_.find(child_id);
   if (state == security_state_.end())
     return false;
 
   return state->second->can_send_midi_sysex();
 }
 
 }  // namespace content