Download retry: Add security checks for download without renderer.

This CL starts to add security check for download without renderer in
non network service code path.

The security hook is in content, so we may need to introduce some
content dependency on network service code path as well. The in memory
download backend of background download may have the same issue. These
scenarios will be addressed in following up CLs.

Bug: 883387
Change-Id: I02001fe97a54960903f26d0e5376f2a8b46e2db6
Reviewed-on: https://chromium-review.googlesource.com/c/1318840Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605868}

content/browser/download/download_manager_impl.cc

@@ -114,18 +114,6 @@ StoragePartitionImpl* GetStoragePartition(BrowserContext* context,
       BrowserContext::GetStoragePartition(context, site_instance));
 }
 
-bool CanRequestURLFromRenderer(int render_process_id, GURL url) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  // Check if the renderer is permitted to request the requested URL.
-  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
-          render_process_id, url)) {
-    DVLOG(1) << "Denied unauthorized download request for "
-             << url.possibly_invalid_spec();
-    return false;
-  }
-  return true;
-}
-
 void OnDownloadStarted(
     download::DownloadItemImpl* download,
     const download::DownloadUrlParameters::OnStartedCallback& on_started) {
@@ -1284,8 +1272,7 @@ void DownloadManagerImpl::BeginDownloadInternal(
     const GURL& site_url) {
   // Check if the renderer is permitted to request the requested URL.
   if (params->render_process_host_id() >= 0 &&
-      !CanRequestURLFromRenderer(params->render_process_host_id(),
-                                 params->url())) {
+      !CanRequestURL(params->render_process_host_id(), params->url())) {
     CreateInterruptedDownload(
         std::move(params),
         download::DOWNLOAD_INTERRUPT_REASON_NETWORK_INVALID_REQUEST,

content/browser/download/download_utils.cc

@@ -20,6 +20,7 @@
 #include "content/browser/resource_context_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -99,4 +100,15 @@ std::unique_ptr<net::URLRequest> CreateURLRequestOnIOThread(
   return request;
 }
 
+bool CanRequestURL(int render_process_id, const GURL& url) {
+  // Check if the renderer is permitted to request the requested URL.
+  if (!ChildProcessSecurityPolicy::GetInstance()->CanRequestURL(
+          render_process_id, url)) {
+    DVLOG(1) << "Denied unauthorized download request for "
+             << url.possibly_invalid_spec();
+    return false;
+  }
+  return true;
+}
+
 }  // namespace content

content/browser/download/download_utils.h

@@ -10,6 +10,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/optional.h"
 #include "content/common/content_export.h"
+#include "url/gurl.h"
 
 namespace download {
 class DownloadUrlParameters;
@@ -37,6 +38,9 @@ std::unique_ptr<net::URLRequest> CONTENT_EXPORT CreateURLRequestOnIOThread(
 storage::BlobStorageContext* BlobStorageContextGetter(
     ResourceContext* resource_context);
 
+// Returns if the URL passes the security check and can be requested.
+bool CanRequestURL(int render_process_id, const GURL& url);
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_UTILS_H_

content/browser/download/url_downloader.cc

@@ -17,8 +17,10 @@
 #include "components/download/public/common/url_download_request_handle.h"
 #include "content/browser/byte_stream.h"
 #include "content/browser/download/byte_stream_input_stream.h"
+#include "content/browser/download/download_utils.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/common/child_process_host.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
@@ -41,6 +43,7 @@ std::unique_ptr<UrlDownloader> UrlDownloader::BeginDownload(
       Referrer::SanitizeForRequest(request->url(), referrer);
   Referrer::SetReferrerForRequest(request.get(), sanitized_referrer);
 
+  // TODO(xingliu): Figure out if we can support blob scheme.
   if (request->url().SchemeIs(url::kBlobScheme))
     return nullptr;
 
@@ -85,10 +88,13 @@ void UrlDownloader::Start() {
 void UrlDownloader::OnReceivedRedirect(net::URLRequest* request,
                                        const net::RedirectInfo& redirect_info,
                                        bool* defer_redirect) {
-  if (follow_cross_origin_redirects_)
+  DVLOG(1) << __func__ << " , request url: " << request_->url().spec()
+           << " ,redirect url:" << redirect_info.new_url;
+  if (follow_cross_origin_redirects_ &&
+      CanRequestURL(content::ChildProcessHost::kInvalidUniqueID,
+                    redirect_info.new_url)) {
     return;
-
-  DVLOG(1) << "OnReceivedRedirect: " << request_->url().spec();
+  }
 
   // Block redirects since there is no security policy being applied here.
   core_.OnWillAbort(download::DOWNLOAD_INTERRUPT_REASON_SERVER_UNREACHABLE);
@@ -105,6 +111,14 @@ void UrlDownloader::OnResponseStarted(net::URLRequest* request, int net_error) {
     return;
   }
 
+  if (!CanRequestURL(content::ChildProcessHost::kInvalidUniqueID,
+                     request_->url())) {
+    core_.OnWillAbort(
+        download::DOWNLOAD_INTERRUPT_REASON_NETWORK_INVALID_REQUEST);
+    request_->CancelWithError(net::ERR_DISALLOWED_URL_SCHEME);
+    return;
+  }
+
   if (core_.OnResponseStarted(std::string()))
     StartReading(false);  // Read the first chunk.
   else

content/browser/download/url_downloader.h

@@ -79,7 +79,7 @@ class UrlDownloader : public net::URLRequest::Delegate,
   base::WeakPtr<download::UrlDownloadHandler::Delegate> delegate_;
   DownloadRequestCore core_;
 
-  bool follow_cross_origin_redirects_;
+  const bool follow_cross_origin_redirects_;
 
   base::WeakPtrFactory<UrlDownloader> weak_ptr_factory_;
 };