Sanitize https:// URLs before sending them to PAC scripts.

net/proxy/proxy_service.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 "net/proxy/proxy_service.h"
 
 #include <algorithm>
 #include <cmath>
 #include <utility>
 
@@ skipping 331 matching lines @@
   void AddObserver(Observer* observer) override {}
   void RemoveObserver(Observer* observer) override {}
   ConfigAvailability GetLatestProxyConfig(ProxyConfig* config) override {
     return CONFIG_UNSET;
   }
 };
 #endif
 
 }  // namespace
 
+GURL SanitizeUrlForPacScript(const GURL& url,
+                             SanitizeUrlForPacScriptPolicy policy) {
+  DCHECK(url.is_valid());
+  GURL::Replacements replacements;
+  replacements.ClearUsername();
+  replacements.ClearPassword();
+  replacements.ClearRef();
+
+  if (policy == SanitizeUrlForPacScriptPolicy::SAFE &&
+      url.SchemeIsCryptographic()) {
+    replacements.ClearPath();
+    replacements.ClearQuery();
+  }

[mmenke, 2016/05/19 22:33:23]

An alternative approach would be to do:

if (policy == SanitizeUrlForPacScriptPolicy::SAFE && 
url.SchemeIsCryptographic()) {
  return url.GetOrigin();
}

GURL::Replacements replacements;
...

Nice thing about that approach is it emphasizes this is indeed just an origin. 
Downside is it de-emphasizes just what the differences are between the two
paths.

[eroman, 2016/05/19 23:26:05]

I added a TODO to explore that, will follow-up.

There are some differences I would like to better understand between the two
approaches -- GetOrigin() short circuits on non-standard URL schemes and
completely clears the URL. I am not sure if that would come into play here, but
would like to play it safe by using the existing pattern of code.

+
+  return url.ReplaceComponents(replacements);
+}
+
 // ProxyService::InitProxyResolver --------------------------------------------
 
 // This glues together two asynchronous steps:
 //   (1) ProxyScriptDecider -- try to fetch/validate a sequence of PAC scripts
 //       to figure out what we should configure against.
 //   (2) Feed the fetched PAC script into the ProxyResolver.
 //
 // InitProxyResolver is a single-use class which encapsulates cancellation as
 // part of its destructor. Start() or StartSkipDecider() should be called just
 // once. The instance can be destroyed at any time, and the request will be
@@ skipping 681 matching lines @@
 
   // Notify our polling-based dependencies that a resolve is taking place.
   // This way they can schedule their polls in response to network activity.
   config_service_->OnLazyPoll();
   if (script_poller_.get())
      script_poller_->OnLazyPoll();
 
   if (current_state_ == STATE_NONE)
     ApplyProxyConfigIfAvailable();
 
-  // Strip away any reference fragments and the username/password, as they
-  // are not relevant to proxy resolution.
-  GURL url = SimplifyUrlForRequest(raw_url);
+  // Sanitize the URL before passing it on to the proxy resolver (i.e. PAC
+  // script). The goal is to remove sensitive data (like embedded user names
+  // and password), and local data (i.e. reference fragment).
+  GURL url =
+      SanitizeUrlForPacScript(raw_url, sanitize_url_for_pac_script_policy_);

[mmenke, 2016/05/19 22:33:23]

Should we only do this when we create the PacRequest?  Seems weird to call it
SanitizeForPac, and then send it to TryToCompleteSynchronously, which doesn't
actually run a PAC script.

[eroman, 2016/05/19 23:26:05]

I hope to merge this CL to M52 so don't want to change the existing code
organization too much.

I have ideas on improving this but will not be tackling them here.

(There are several refactoring opportunities around layering issues in this file
-- too many PAC concepts leak into ProxyService. Also the distinction between
ProxyService, ProxyConfigService, and ProxyResolver is not great.)

 
   // Check if the request can be completed right away. (This is the case when
   // using a direct connection for example).
   int rv = TryToCompleteSynchronously(url, load_flags, proxy_delegate, result);
   if (rv != ERR_IO_PENDING) {
     rv = DidFinishResolvingProxy(
         url, method, load_flags, proxy_delegate, result, rv, net_log,
         callback.is_null() ? TimeTicks() : TimeTicks::Now(), false);
     return rv;
   }
@@ skipping 589 matching lines @@
   State previous_state = ResetProxyConfig(false);
   if (previous_state != STATE_NONE)
     ApplyProxyConfigIfAvailable();
 }
 
 void ProxyService::OnDNSChanged() {
   OnIPAddressChanged();
 }
 
 }  // namespace net