DragEnter grants RequestURL to entire file:// scheme.

DragEnter can't know if the end action of a dragged file will be to assign
it to the value of a file input element, or to navigate to the file itself,
so it grants the permissions required for both. The RequestURL permission,
however, currently implies access to all of file:// even though we intend to
request only one file.  This change adds a method to ChildProcessSecurityPolicy
for more granular permissions for file:// URLs which is applied to the existing
renderer.  A second change causes file:// navigations to be browser-navigations,
so that the existing renderer will fork a new "file-privileged" renderer.  The
old renderer, having permissions for this one URL, will pass the checks
required to lauch the new renderer for the URL, but will not have permission
to fork renderers for other file:// URLs.

This is a second attempt at resolving the issue, see also:
http://codereview.chromium.org/10397002/

BUG=127525

Review URL: https://chromiumcodereview.appspot.com/10517009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@141124 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 128 insertions, 24 deletions

diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
index d8d4415..1f40966 100644
--- a/content/browser/child_process_security_policy_impl.cc
+++ b/content/browser/child_process_security_policy_impl.cc
@@ -15,6 +15,7 @@
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/url_constants.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/net_util.h"
 #include "net/url_request/url_request.h"
 #include "webkit/fileapi/isolated_context.h"
 
@@ -81,9 +82,16 @@ class ChildProcessSecurityPolicyImpl::SecurityState {
                          stripped.value().size());
   }
 
+  // Grant navigation to a file but not the file:// scheme in general.
+  void GrantRequestOfSpecificFile(const FilePath &file) {
+    request_file_set_.insert(file.StripTrailingSeparators());
+  }
+
   // Revokes all permissions granted to a file.
   void RevokeAllPermissionsForFile(const FilePath& file) {
-    file_permissions_.erase(file.StripTrailingSeparators());
+    FilePath stripped = file.StripTrailingSeparators();
+    file_permissions_.erase(stripped);
+    request_file_set_.erase(stripped);
   }
 
   // Grant certain permissions to a file.
@@ -113,15 +121,22 @@ class ChildProcessSecurityPolicyImpl::SecurityState {
     can_read_raw_cookies_ = false;
   }
 
-  // Determine whether permission has been granted to request url.
-  // Schemes that have not been granted default to being denied.
+  // Determine whether permission has been granted to request |url|.
   bool CanRequestURL(const GURL& url) {
+    // Having permission to a scheme implies permssion to all of its URLs.
     SchemeMap::const_iterator judgment(scheme_policy_.find(url.scheme()));
+    if (judgment != scheme_policy_.end())
+      return judgment->second;
+
+    // file:// URLs are more granular.  The child may have been given
+    // permission to a specific file but not the file:// scheme in general.
+    if (url.SchemeIs(chrome::kFileScheme)) {
+      FilePath path;
+      if (net::FileURLToFilePath(url, &path))
+        return request_file_set_.find(path) != request_file_set_.end();
+    }
 
-    if (judgment == scheme_policy_.end())
-      return false;  // Unmentioned schemes are disallowed.
-
-    return judgment->second;
+    return false;  // Unmentioned schemes are disallowed.
   }
 
   // Determine if the certain permissions have been granted to a file.
@@ -163,6 +178,7 @@ class ChildProcessSecurityPolicyImpl::SecurityState {
   typedef int FilePermissionFlags;  // bit-set of PlatformFileFlags
   typedef std::map<FilePath, FilePermissionFlags> FileMap;
   typedef std::map<std::string, FilePermissionFlags> FileSystemMap;
+  typedef std::set<FilePath> FileSet;
 
   // Maps URL schemes to whether permission has been granted or revoked:
   //   |true| means the scheme has been granted.
@@ -174,6 +190,9 @@ class ChildProcessSecurityPolicyImpl::SecurityState {
   // The set of files the child process is permited to upload to the web.
   FileMap file_permissions_;
 
+  // The set of files the child process is permitted to load.
+  FileSet request_file_set_;
+
   int enabled_bindings_;
 
   bool can_read_raw_cookies_;
@@ -315,12 +334,32 @@ void ChildProcessSecurityPolicyImpl::GrantRequestURL(
     if (state == security_state_.end())
       return;
 
-    // If the child process has been commanded to request a scheme, then we
-    // grant it the capability to request URLs of that scheme.
+    // When the child process has been commanded to request this scheme,
+    // we grant it the capability to request all URLs of that scheme.
     state->second->GrantScheme(url.scheme());
   }
 }
 
+void ChildProcessSecurityPolicyImpl::GrantRequestSpecificFileURL(
+    int child_id,
+    const GURL& url) {
+  if (!url.SchemeIs(chrome::kFileScheme))
+    return;
+
+  {
+    base::AutoLock lock(lock_);
+    SecurityStateMap::iterator state = security_state_.find(child_id);
+    if (state == security_state_.end())
+      return;
+
+    // When the child process has been commanded to request a file:// URL,
+    // then we grant it the capability for that URL only.
+    FilePath path;
+    if (net::FileURLToFilePath(url, &path))
+        state->second->GrantRequestOfSpecificFile(path);
+  }
+}
+
 void ChildProcessSecurityPolicyImpl::GrantReadFile(int child_id,
                                                    const FilePath& file) {
   GrantPermissionsForFile(child_id, file, kReadFilePermissions);
diff --git a/content/browser/child_process_security_policy_impl.h b/content/browser/child_process_security_policy_impl.h
index 330efb3..ce0a609 100644
--- a/content/browser/child_process_security_policy_impl.h
+++ b/content/browser/child_process_security_policy_impl.h
@@ -76,9 +76,15 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
 
   // Whenever the browser processes commands the child process to request a URL,
   // it should call this method to grant the child process the capability to
-  // request the URL.
+  // request the URL, along with permission to request all URLs of the same
+  // scheme.
   void GrantRequestURL(int child_id, const GURL& url);
 
+  // Whenever the browser process drops a file icon on a tab, it should call
+  // this method to grant the child process the capability to request this one
+  // file:// URL, but not all urls of the file:// scheme.
+  void GrantRequestSpecificFileURL(int child_id, const GURL& url);
+
   // Grants the child process permission to enumerate all the files in
   // this directory and read those files.
   void GrantReadDirectory(int child_id, const FilePath& directory);
diff --git a/content/browser/child_process_security_policy_unittest.cc b/content/browser/child_process_security_policy_unittest.cc
index 77be482..eaacda8 100644
--- a/content/browser/child_process_security_policy_unittest.cc
+++ b/content/browser/child_process_security_policy_unittest.cc
@@ -271,6 +271,28 @@ TEST_F(ChildProcessSecurityPolicyTest, ViewSource) {
   p->Remove(kRendererID);
 }
 
+TEST_F(ChildProcessSecurityPolicyTest, SpecificFile) {
+  ChildProcessSecurityPolicyImpl* p =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+
+  p->Add(kRendererID);
+
+  GURL icon_url("file:///tmp/foo.png");
+  GURL sensitive_url("file:///etc/passwd");
+  EXPECT_FALSE(p->CanRequestURL(kRendererID, icon_url));
+  EXPECT_FALSE(p->CanRequestURL(kRendererID, sensitive_url));
+
+  p->GrantRequestSpecificFileURL(kRendererID, icon_url);
+  EXPECT_TRUE(p->CanRequestURL(kRendererID, icon_url));
+  EXPECT_FALSE(p->CanRequestURL(kRendererID, sensitive_url));
+
+  p->GrantRequestURL(kRendererID, icon_url);
+  EXPECT_TRUE(p->CanRequestURL(kRendererID, icon_url));
+  EXPECT_TRUE(p->CanRequestURL(kRendererID, sensitive_url));
+
+  p->Remove(kRendererID);
+}
+
 TEST_F(ChildProcessSecurityPolicyTest, CanReadFiles) {
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();
diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc
index 8cce2a7..eba176f 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -531,8 +531,14 @@ void RenderViewHostImpl::DragTargetDragEnter(
   for (std::vector<WebDropData::FileInfo>::const_iterator iter(
            filtered_data.filenames.begin());
        iter != filtered_data.filenames.end(); ++iter) {
+    // A dragged file may wind up as the value of an input element, or it
+    // may be used as the target of a navigation instead.  We don't know
+    // which will happen at this point, so generously grant both access
+    // and request permissions to the specific file to cover both cases.
+    // We do not give it the permission to request all file:// URLs.
     FilePath path = FilePath::FromUTF8Unsafe(UTF16ToUTF8(iter->path));
-    policy->GrantRequestURL(renderer_id, net::FilePathToFileURL(path));
+    policy->GrantRequestSpecificFileURL(renderer_id,
+                                        net::FilePathToFileURL(path));
 
     // If the renderer already has permission to read these paths, we don't need
     // to re-grant them. This prevents problems with DnD for files in the CrOS
diff --git a/content/browser/renderer_host/render_view_host_unittest.cc b/content/browser/renderer_host/render_view_host_unittest.cc
index 655cbef..c29e8f0 100644
--- a/content/browser/renderer_host/render_view_host_unittest.cc
+++ b/content/browser/renderer_host/render_view_host_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/utf_string_conversions.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/renderer_host/test_render_view_host.h"
 #include "content/browser/web_contents/navigation_controller_impl.h"
@@ -154,12 +155,29 @@ TEST_F(RenderViewHostTest, DragEnteredFileURLsStillBlocked) {
   WebDropData dropped_data;
   gfx::Point client_point;
   gfx::Point screen_point;
-  GURL file_url = GURL("file:///etc/passwd");
-  dropped_data.url = file_url;
+  FilePath highlighted_file_path(FILE_PATH_LITERAL("/tmp/foo.html"));
+  FilePath dragged_file_path(FILE_PATH_LITERAL("/tmp/image.jpg"));
+  FilePath sensitive_file_path(FILE_PATH_LITERAL("/etc/passwd"));
+  GURL highlighted_file_url = net::FilePathToFileURL(highlighted_file_path);
+  GURL dragged_file_url = net::FilePathToFileURL(dragged_file_path);
+  GURL sensitive_file_url = net::FilePathToFileURL(sensitive_file_path);
+  dropped_data.url = highlighted_file_url;
+  dropped_data.filenames.push_back(WebDropData::FileInfo(
+      UTF8ToUTF16(dragged_file_path.AsUTF8Unsafe()), string16()));
+
   rvh()->DragTargetDragEnter(dropped_data, client_point, screen_point,
-                             WebKit::WebDragOperationNone, 0);
-  EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
-      process()->GetID(), file_url));
+                              WebKit::WebDragOperationNone, 0);
+
+  int id = process()->GetID();
+  ChildProcessSecurityPolicyImpl* policy =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+
+  EXPECT_FALSE(policy->CanRequestURL(id, highlighted_file_url));
+  EXPECT_FALSE(policy->CanReadFile(id, highlighted_file_path));
+  EXPECT_TRUE(policy->CanRequestURL(id, dragged_file_url));
+  EXPECT_TRUE(policy->CanReadFile(id, dragged_file_path));
+  EXPECT_FALSE(policy->CanRequestURL(id, sensitive_file_url));
+  EXPECT_FALSE(policy->CanReadFile(id, sensitive_file_path));
 }
 
 // The test that follow trigger DCHECKS in debug build.
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index 6955472..36a2413 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -2517,6 +2517,12 @@ WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
     }
   }
 
+  // Use the frame's original request's URL rather than the document's URL for
+  // subsequent checks.  For a popup, the document's URL may become the opener
+  // window's URL if the opener has called document.write().
+  // See http://crbug.com/93517.
+  GURL old_url(frame->dataSource()->request().url());
+
   // Detect when we're crossing a permission-based boundary (e.g. into or out of
   // an extension or app origin, leaving a WebUI page, etc). We only care about
   // top-level navigations (not iframes). But we sometimes navigate to
@@ -2539,14 +2545,27 @@ WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
     // data sources can be registered.
     // Similarly, navigations to view-source URLs or within ViewSource mode
     // must be handled by the browser process.
-    int cumulative_bindings =
-        RenderProcess::current()->GetEnabledBindings();
+    // Lastly, access to file:// URLs from non-file:// URL pages must be
+    // handled by the browser so that ordinary renderer processes don't get
+    // blessed with file permissions.
+    int cumulative_bindings = RenderProcess::current()->GetEnabledBindings();
+    bool is_initial_navigation = page_id_ == -1;
     bool should_fork =
         content::GetContentClient()->HasWebUIScheme(url) ||
         (cumulative_bindings & content::BINDINGS_POLICY_WEB_UI) ||
         url.SchemeIs(chrome::kViewSourceScheme) ||
         frame->isViewSourceModeEnabled();
 
+    if (!should_fork && url.SchemeIs(chrome::kFileScheme)) {
+      // Fork non-file to file opens.  Check the opener URL if this is the
+      // initial navigation in a newly opened window.
+      GURL source_url(old_url);
+      if (is_initial_navigation && source_url.is_empty() && frame->opener())
+        source_url = frame->opener()->top()->document().url();
+      DCHECK(!source_url.is_empty());
+      should_fork = !source_url.SchemeIs(chrome::kFileScheme);
+    }
+
     if (!should_fork) {
       // Give the embedder a chance.
       // For now, we skip this for POST submissions.  This is because
@@ -2554,7 +2573,6 @@ WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
       // with hosted apps and extensions than WebUI pages.  We will remove this
       // check when cross-process POST submissions are supported.
       if (request.httpMethod() == "GET") {
-        bool is_initial_navigation = page_id_ == -1;
         should_fork = content::GetContentClient()->renderer()->ShouldFork(
             frame, url, is_initial_navigation, &send_referrer);
       }
@@ -2567,11 +2585,6 @@ WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
     }
   }
 
-  // Use the frame's original request's URL rather than the document's URL for
-  // this check.  For a popup, the document's URL may become the opener window's
-  // URL if the opener has called document.write.  See http://crbug.com/93517.
-  GURL old_url(frame->dataSource()->request().url());
-
   // Detect when a page is "forking" a new tab that can be safely rendered in
   // its own process.  This is done by sites like Gmail that try to open links
   // in new windows without script connections back to the original page.  We