Disallow fetching of file:// URLs discovered by DHCP-based WPAD.

Bug: 839647
TBR: stevenjb@chromium.org
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I9223238b5f187d632d68d54fd2ee4fd50981fc56
Reviewed-on: https://chromium-review.googlesource.com/1045610
Commit-Queue: Eric Roman <eroman@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556644}

chromeos/network/dhcp_pac_file_fetcher_chromeos.cc

@@ -35,7 +35,7 @@ DhcpPacFileFetcherChromeos::DhcpPacFileFetcherChromeos(
     net::URLRequestContext* url_request_context)
     : weak_ptr_factory_(this) {
   DCHECK(url_request_context);
-  pac_file_fetcher_.reset(new net::PacFileFetcherImpl(url_request_context));
+  pac_file_fetcher_ = net::PacFileFetcherImpl::Create(url_request_context);
   if (NetworkHandler::IsInitialized())
     network_handler_task_runner_ = NetworkHandler::Get()->task_runner();
 }

net/data/pac_file_fetcher_unittest/redirect-to-file

+-redirected-

net/data/pac_file_fetcher_unittest/redirect-to-file.mock-http-headers

+HTTP/1.1 302 Found
+Location: file://bin/cat

net/proxy_resolution/dhcp_pac_file_adapter_fetcher_win.cc

@@ -153,7 +153,7 @@ void DhcpPacFileAdapterFetcher::OnDhcpQueryDone(
     TransitionToFinish();
   } else {
     state_ = STATE_WAIT_URL;
-    script_fetcher_.reset(ImplCreateScriptFetcher());
+    script_fetcher_ = ImplCreateScriptFetcher();
     script_fetcher_->Fetch(pac_url_, &pac_script_,
                            base::Bind(&DhcpPacFileAdapterFetcher::OnFetcherDone,
                                       base::Unretained(this)),
@@ -194,8 +194,9 @@ DhcpPacFileAdapterFetcher::State DhcpPacFileAdapterFetcher::state() const {
   return state_;
 }
 
-PacFileFetcher* DhcpPacFileAdapterFetcher::ImplCreateScriptFetcher() {
-  return new PacFileFetcherImpl(url_request_context_);
+std::unique_ptr<PacFileFetcher>
+DhcpPacFileAdapterFetcher::ImplCreateScriptFetcher() {
+  return PacFileFetcherImpl::Create(url_request_context_);
 }
 
 DhcpPacFileAdapterFetcher::DhcpQuery*

net/proxy_resolution/dhcp_pac_file_adapter_fetcher_win.h

@@ -148,7 +148,7 @@ class NET_EXPORT_PRIVATE DhcpPacFileAdapterFetcher
   };
 
   // Virtual methods introduced to allow unit testing.
-  virtual PacFileFetcher* ImplCreateScriptFetcher();
+  virtual std::unique_ptr<PacFileFetcher> ImplCreateScriptFetcher();
   virtual DhcpQuery* ImplCreateDhcpQuery();
   virtual base::TimeDelta ImplGetTimeout() const;
 

net/proxy_resolution/dhcp_pac_file_adapter_fetcher_win_unittest.cc

@@ -56,7 +56,7 @@ class MockDhcpPacFileAdapterFetcher : public DhcpPacFileAdapterFetcher {
     fetcher_ = NULL;
   }
 
-  PacFileFetcher* ImplCreateScriptFetcher() override {
+  std::unique_ptr<PacFileFetcher> ImplCreateScriptFetcher() override {
     // We don't maintain ownership of the fetcher, it is transferred to
     // the caller.
     fetcher_ = new MockPacFileFetcher();
@@ -65,7 +65,7 @@ class MockDhcpPacFileAdapterFetcher : public DhcpPacFileAdapterFetcher {
           FROM_HERE, base::TimeDelta::FromMilliseconds(fetcher_delay_ms_), this,
           &MockDhcpPacFileAdapterFetcher::OnFetcherTimer);
     }
-    return fetcher_;
+    return base::WrapUnique(fetcher_);
   }
 
   class DelayingDhcpQuery : public DhcpQuery {
@@ -276,9 +276,8 @@ class MockDhcpRealFetchPacFileAdapterFetcher
         url_request_context_(context) {}
 
   // Returns a real PAC file fetcher.
-  PacFileFetcher* ImplCreateScriptFetcher() override {
-    PacFileFetcher* fetcher = new PacFileFetcherImpl(url_request_context_);
-    return fetcher;
+  std::unique_ptr<PacFileFetcher> ImplCreateScriptFetcher() override {
+    return PacFileFetcherImpl::Create(url_request_context_);
   }
 
   URLRequestContext* url_request_context_;

net/proxy_resolution/pac_file_fetcher_impl.cc

@@ -19,6 +19,7 @@
 #include "net/base/request_priority.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/http/http_response_headers.h"
+#include "net/url_request/redirect_info.h"
 #include "net/url_request/url_request_context.h"
 
 // TODO(eroman):
@@ -81,17 +82,15 @@ void ConvertResponseToUTF16(const std::string& charset,
 
 }  // namespace
 
-PacFileFetcherImpl::PacFileFetcherImpl(URLRequestContext* url_request_context)
-    : url_request_context_(url_request_context),
-      buf_(new IOBuffer(kBufSize)),
-      next_id_(0),
-      cur_request_id_(0),
-      result_code_(OK),
-      result_text_(NULL),
-      max_response_bytes_(kDefaultMaxResponseBytes),
-      max_duration_(kDefaultMaxDuration),
-      weak_factory_(this) {
-  DCHECK(url_request_context);
+std::unique_ptr<PacFileFetcherImpl> PacFileFetcherImpl::Create(
+    URLRequestContext* url_request_context) {
+  return base::WrapUnique(new PacFileFetcherImpl(url_request_context, false));
+}
+
+std::unique_ptr<PacFileFetcherImpl>
+PacFileFetcherImpl::CreateWithFileUrlSupport(
+    URLRequestContext* url_request_context) {
+  return base::WrapUnique(new PacFileFetcherImpl(url_request_context, true));
 }
 
 PacFileFetcherImpl::~PacFileFetcherImpl() {
@@ -137,6 +136,9 @@ int PacFileFetcherImpl::Fetch(
   if (!url_request_context_)
     return ERR_CONTEXT_SHUT_DOWN;
 
+  if (!IsUrlSchemeAllowed(url))
+    return ERR_DISALLOWED_URL_SCHEME;
+
   // Handle base-64 encoded data-urls that contain custom PAC scripts.
   if (url.SchemeIs("data")) {
     std::string mime_type;
@@ -211,6 +213,27 @@ void PacFileFetcherImpl::OnShutdown() {
   }
 }
 
+void PacFileFetcherImpl::OnReceivedRedirect(URLRequest* request,
+                                            const RedirectInfo& redirect_info,
+                                            bool* defer_redirect) {
+  int error = OK;
+
+  // Redirection to file:// is never OK. Ordinarily this is handled lower in the
+  // stack (|FileProtocolHandler::IsSafeRedirectTarget|), but this is reachable
+  // when built without file:// suppport. Return the same error for consistency.
+  if (redirect_info.new_url.SchemeIsFile()) {
+    error = ERR_UNSAFE_REDIRECT;
+  } else if (!IsUrlSchemeAllowed(redirect_info.new_url)) {
+    error = ERR_DISALLOWED_URL_SCHEME;
+  }
+
+  if (error != OK) {
+    // Fail the redirect.
+    request->CancelWithError(error);
+    OnResponseCompleted(request, error);
+  }
+}
+
 void PacFileFetcherImpl::OnAuthRequired(URLRequest* request,
                                         AuthChallengeInfo* auth_info) {
   DCHECK_EQ(request, cur_request_.get());
@@ -280,6 +303,36 @@ void PacFileFetcherImpl::OnReadCompleted(URLRequest* request, int num_bytes) {
   }
 }
 
+PacFileFetcherImpl::PacFileFetcherImpl(URLRequestContext* url_request_context,
+                                       bool allow_file_url)
+    : url_request_context_(url_request_context),
+      buf_(new IOBuffer(kBufSize)),
+      next_id_(0),
+      cur_request_id_(0),
+      result_code_(OK),
+      result_text_(NULL),
+      max_response_bytes_(kDefaultMaxResponseBytes),
+      max_duration_(kDefaultMaxDuration),
+      allow_file_url_(allow_file_url),
+      weak_factory_(this) {
+  DCHECK(url_request_context);
+}
+
+bool PacFileFetcherImpl::IsUrlSchemeAllowed(const GURL& url) const {
+  // Always allow http://, https://, data:, and ftp://.
+  if (url.SchemeIsHTTPOrHTTPS() || url.SchemeIs("ftp") || url.SchemeIs("data"))
+    return true;
+
+  // Only permit file:// if |allow_file_url_| was set. file:// should not be
+  // allowed for URLs that were auto-detected, or as the result of a server-side
+  // redirect.
+  if (url.SchemeIsFile())
+    return allow_file_url_;
+
+  // Disallow any other URL scheme.
+  return false;
+}
+
 void PacFileFetcherImpl::ReadBody(URLRequest* request) {
   // Read as many bytes as are available synchronously.
   while (true) {

net/proxy_resolution/pac_file_fetcher_impl.h

@@ -38,7 +38,23 @@ class NET_EXPORT PacFileFetcherImpl : public PacFileFetcher,
   // Note that while a request is in progress, we will be holding a reference
   // to |url_request_context|. Be careful not to create cycles between the
   // fetcher and the context; you can break such cycles by calling Cancel().
-  explicit PacFileFetcherImpl(URLRequestContext* url_request_context);
+  //
+  // Fetch() supports the following URL schemes, provided the underlying
+  // |url_request_context| also supports them:
+  //
+  //   * http://
+  //   * https://
+  //   * ftp://
+  //   * data:
+  static std::unique_ptr<PacFileFetcherImpl> Create(
+      URLRequestContext* url_request_context);
+
+  // Same as Create(), but additionally allows fetching PAC URLs from file://
+  // URLs (provided the URLRequestContext supports it).
+  //
+  // This should not be used in new code (see https://crbug.com/839566).
+  static std::unique_ptr<PacFileFetcherImpl> CreateWithFileUrlSupport(
+      URLRequestContext* url_request_context);
 
   ~PacFileFetcherImpl() override;
 
@@ -58,6 +74,9 @@ class NET_EXPORT PacFileFetcherImpl : public PacFileFetcher,
   void OnShutdown() override;
 
   // URLRequest::Delegate methods:
+  void OnReceivedRedirect(URLRequest* request,
+                          const RedirectInfo& redirect_info,
+                          bool* defer_redirect) override;
   void OnAuthRequired(URLRequest* request,
                       AuthChallengeInfo* auth_info) override;
   void OnSSLCertificateError(URLRequest* request,
@@ -69,6 +88,13 @@ class NET_EXPORT PacFileFetcherImpl : public PacFileFetcher,
  private:
   enum { kBufSize = 4096 };
 
+  PacFileFetcherImpl(URLRequestContext* url_request_context,
+                     bool allow_file_url);
+
+  // Returns true if |url| has an acceptable URL scheme (i.e. http://, https://,
+  // etc).
+  bool IsUrlSchemeAllowed(const GURL& url) const;
+
   // Read more bytes from the response.
   void ReadBody(URLRequest* request);
 
@@ -129,6 +155,8 @@ class NET_EXPORT PacFileFetcherImpl : public PacFileFetcher,
   // The time that the first byte was received.
   base::TimeTicks fetch_time_to_first_byte_;
 
+  const bool allow_file_url_;
+
   // Factory for creating the time-out task. This takes care of revoking
   // outstanding tasks when |this| is deleted.
   base::WeakPtrFactory<PacFileFetcherImpl> weak_factory_;

services/network/url_request_context_builder_mojo.cc

@@ -56,8 +56,8 @@ URLRequestContextBuilderMojo::CreateProxyResolutionService(
   if (mojo_proxy_resolver_factory_) {
     std::unique_ptr<net::DhcpPacFileFetcher> dhcp_pac_file_fetcher =
         dhcp_fetcher_factory_->Create(url_request_context);
-    std::unique_ptr<net::PacFileFetcher> pac_file_fetcher =
-        std::make_unique<net::PacFileFetcherImpl>(url_request_context);
+    auto pac_file_fetcher =
+        net::PacFileFetcherImpl::CreateWithFileUrlSupport(url_request_context);
     return CreateProxyResolutionServiceUsingMojoFactory(
         std::move(mojo_proxy_resolver_factory_),
         std::move(proxy_config_service), std::move(pac_file_fetcher),