chromeos: Check both original and absolute paths for file: scheme

net/url_request/url_request_unittest.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 <memory>
 #include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "build/build_config.h"
@@ skipping 816 matching lines @@
   }
 
  protected:
   TestNetLog net_log_;
   TestNetworkDelegate default_network_delegate_;  // Must outlive URLRequest.
   URLRequestJobFactoryImpl* job_factory_impl_;
   std::unique_ptr<URLRequestJobFactory> job_factory_;
   TestURLRequestContext default_context_;
 };
 
+// This NetworkDelegate is picky about what files are accessible. Only
+// whitelisted files are allowed.
+class PickyNetworkDelegate : public TestNetworkDelegate {

[mmenke, 2017/05/15 19:21:14]

nit:  Maybe CookieBlockingNetworkDelegate?

[satorux1, 2017/05/19 07:02:53]

Done.

+ public:
+  // Adds |directory| to the access white list.
+  void AddToWhitelist(const base::FilePath& directory) {
+    whitelist_.insert(directory);
+  }
+
+  // Returns true if |path| matches the white list.
+  bool OnCanAccessFileInternal(const base::FilePath& path) const {
+    for (const auto& directory : whitelist_) {
+      if (directory == path || directory.IsParent(path))
+        return true;
+    }
+    return false;
+  }
+
+  // Returns true only if both |original_path| and |absolute_path| match the
+  // white list.
+  bool OnCanAccessFile(const URLRequest& request,
+                       const base::FilePath& original_path,
+                       const base::FilePath& absolute_path) const override {
+    return (OnCanAccessFileInternal(original_path) &&
+            OnCanAccessFileInternal(absolute_path));
+  }
+
+ private:
+  std::set<base::FilePath> whitelist_;

[mmenke, 2017/05/15 19:21:14]

nit:  DISALLOW_COPY_AND_ASSIGN

[satorux1, 2017/05/19 07:02:53]

Done.

+};
+
 TEST_F(URLRequestTest, AboutBlankTest) {
   TestDelegate d;
   {
     std::unique_ptr<URLRequest> r(
         default_context_.CreateRequest(GURL("about:blank"), DEFAULT_PRIORITY,
                                        &d, TRAFFIC_ANNOTATION_FOR_TESTS));
 
     r->Start();
     EXPECT_TRUE(r->is_pending());
 
@@ skipping 228 matching lines @@
     base::RunLoop().Run();
     EXPECT_TRUE(d.request_failed());
   }
 
   EXPECT_TRUE(base::DeleteFile(temp_path, false));
 }
 
 TEST_F(URLRequestTest, AllowFileURLs) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  // Get an absolute path since the path can contain a symbolic link.
+  base::FilePath absolute_temp_dir =
+      base::MakeAbsoluteFilePath(temp_dir.GetPath());
   base::FilePath test_file;
-  ASSERT_TRUE(base::CreateTemporaryFileInDir(temp_dir.GetPath(), &test_file));
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(absolute_temp_dir, &test_file));
   std::string test_data("monkey");
   base::WriteFile(test_file, test_data.data(), test_data.size());
   GURL test_file_url = FilePathToFileURL(test_file);
-
   {
     TestDelegate d;
-    TestNetworkDelegate network_delegate;
-    network_delegate.set_can_access_files(true);
+    PickyNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(absolute_temp_dir);
     default_context_.set_network_delegate(&network_delegate);
     std::unique_ptr<URLRequest> r(default_context_.CreateRequest(
         test_file_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
     r->Start();
     base::RunLoop().Run();
+    // This should be allowed as the file path is white listed.
     EXPECT_FALSE(d.request_failed());
     EXPECT_EQ(test_data, d.data_received());
   }
 
   {
     TestDelegate d;
-    TestNetworkDelegate network_delegate;
-    network_delegate.set_can_access_files(false);
+    PickyNetworkDelegate network_delegate;
     default_context_.set_network_delegate(&network_delegate);
     std::unique_ptr<URLRequest> r(default_context_.CreateRequest(
         test_file_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
     r->Start();
     base::RunLoop().Run();
+    // This should be rejected as the file path is not white listed.
     EXPECT_TRUE(d.request_failed());
     EXPECT_EQ("", d.data_received());
+    EXPECT_EQ(ERR_ACCESS_DENIED, d.request_status());
   }
 }
 
+#if defined(OS_POSIX)  // Bacause of symbolic links.
+
+TEST_F(URLRequestTest, SymlinksToFiles) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  // Get an absolute path since the path can contain a symbolic link.
+  base::FilePath absolute_temp_dir =
+      base::MakeAbsoluteFilePath(temp_dir.GetPath());
+  base::FilePath test_file;
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(absolute_temp_dir, &test_file));
+  std::string test_data("monkey");
+  base::WriteFile(test_file, test_data.data(), test_data.size());
+
+  // This symlink will point to the test file. Access to the symlink will be
+  // allowed as both the symlink and the destination file are in the same
+  // directory that'll be white listed.
+  base::FilePath good_symlink = absolute_temp_dir.AppendASCII("good_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(test_file, good_symlink));
+  GURL good_file_url = FilePathToFileURL(good_symlink);
+  // This symlink will point to /dev/null. Access to the symlink will be
+  // rejected as /dev/null will not be white listed.
+  base::FilePath bad_symlink = absolute_temp_dir.AppendASCII("bad_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(
+      base::FilePath(FILE_PATH_LITERAL("/dev/null")), bad_symlink));

[mmenke, 2017/05/15 19:21:14]

Maybe use something that's windows friendly?  Not sure if we'll ever support
symlinks on Windows, but I don't think that's a reason to use Windows-hostile
code, unless there are benefits in doing so.  Can just whitelist both the good
file and the good symlink, rather than the path (Or do something else - lots of
options).

[satorux1, 2017/05/19 07:02:52]

Good point. Done

+  GURL bad_file_url = FilePathToFileURL(bad_symlink);
+
+  {
+    TestDelegate d;
+    PickyNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(absolute_temp_dir);
+    default_context_.set_network_delegate(&network_delegate);
+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(good_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // good_file_url should be allowed.
+    EXPECT_FALSE(d.request_failed());
+    EXPECT_EQ(test_data, d.data_received());
+  }
+
+  {
+    TestDelegate d;
+    PickyNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(absolute_temp_dir);
+    default_context_.set_network_delegate(&network_delegate);
+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(bad_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // bad_file_url should be rejected.
+    EXPECT_TRUE(d.request_failed());
+    EXPECT_EQ("", d.data_received());
+    EXPECT_EQ(ERR_ACCESS_DENIED, d.request_status());
+  }
+}
+
+TEST_F(URLRequestTest, SymlinksToDirs) {
+  // The temporary dir will be added to the whitelist.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  // Get an absolute path since the path can contain a symbolic link.
+  base::FilePath absolute_temp_dir =
+      base::MakeAbsoluteFilePath(temp_dir.GetPath());
+
+  // This symlink will point to the temp dir. Access to the symlink will be
+  // allowed as the symlink is in the temp dir that'll be white listed.
+  base::FilePath good_symlink = absolute_temp_dir.AppendASCII("good_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(absolute_temp_dir, good_symlink));
+  GURL good_file_url = FilePathToFileURL(good_symlink);
+  // This symlink will point to /dev. Access to the symlink will be rejected
+  // since /dev will not be white listed.
+  base::FilePath bad_symlink = absolute_temp_dir.AppendASCII("bad_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(
+      base::FilePath(FILE_PATH_LITERAL("/dev")), bad_symlink));

[mmenke, 2017/05/15 19:21:14]

Do we really want to be modifying /dev in a test?  Seems like a bad idea.  Could
we just make a non-whitelisted file in the temp directory?

[satorux1, 2017/05/19 07:02:53]

it's not modifying /dev. it's just creating a symlink to /dev, but I agree that
making a non-whitelisted file in the temp dir is better.  Fixed.

+  GURL bad_file_url = FilePathToFileURL(bad_symlink);
+
+  {
+    TestDelegate d;
+    PickyNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(absolute_temp_dir);
+    default_context_.set_network_delegate(&network_delegate);
+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(good_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // good_file_url should be allowed.
+    EXPECT_FALSE(d.request_failed());
+    ASSERT_NE(d.data_received().find("good_symlink"), std::string::npos);
+  }
+
+  {
+    TestDelegate d;
+    PickyNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(absolute_temp_dir);
+    default_context_.set_network_delegate(&network_delegate);
+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(bad_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // bad_file_url should be rejected.
+    EXPECT_TRUE(d.request_failed());
+    EXPECT_EQ("", d.data_received());
+    EXPECT_EQ(ERR_ACCESS_DENIED, d.request_status());
+  }
+}
+
+#endif  // defined(OS_POSIX)
 
 TEST_F(URLRequestTest, FileDirCancelTest) {
   // Put in mock resource provider.
   NetModule::SetResourceProvider(TestNetResourceProvider);
 
   TestDelegate d;
   {
     base::FilePath file_path;
     PathService::Get(base::DIR_SOURCE_ROOT, &file_path);
     file_path = file_path.Append(FILE_PATH_LITERAL("net"));
@@ skipping 31 matching lines @@
   req->Start();
   base::RunLoop().Run();
 
   // Generate entry for the sentinel file.
   base::FilePath sentinel_path = path.AppendASCII(sentinel_name);
   base::File::Info info;
   EXPECT_TRUE(base::GetFileInfo(sentinel_path, &info));
   EXPECT_GT(info.size, 0);
   std::string sentinel_output = GetDirectoryListingEntry(
       base::string16(sentinel_name, sentinel_name + strlen(sentinel_name)),
-      std::string(sentinel_name),
-      false /* is_dir */,
-      info.size,
+      std::string(sentinel_name), false /* is_dir */, info.size,
+
       info.last_modified);
 
   ASSERT_LT(0, d.bytes_received());
   ASSERT_FALSE(d.request_failed());
   EXPECT_EQ(OK, d.request_status());
   // Check for the entry generated for the "sentinel" file.
   const std::string& data = d.data_received();
   ASSERT_NE(data.find(sentinel_output), std::string::npos);
 }
 
@@ skipping 9876 matching lines @@
   AddTestInterceptor()->set_main_intercept_job(std::move(job));
 
   req->Start();
   req->Cancel();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(ERR_ABORTED, d.request_status());
   EXPECT_EQ(0, d.received_redirect_count());
 }
 
 }  // namespace net