Reland "Reland "[FSA] Add IsSafePathComponent checks to GetFile/GetDirectoryHandle.""

This is a reland of 2d41c395

The added test was modified to no longer assert that all unsafe files
were written to disk successfully. This should make the test pass (albeit
with less stringent checks) on file systems/platforms that don't allow
all unsafe file names.

Original change's description:
> Reland "[FSA] Add IsSafePathComponent checks to GetFile/GetDirectoryHandle."
>
> This is a reland of 00437792
>
> The main difference is to make sure iterating over a directory doesn't
> return files we don't want to expose either (and not CHECK failing if
> such files are found when iterating).
>
> Original change's description:
> > [FSA] Add IsSafePathComponent checks to GetFile/GetDirectoryHandle.
> >
> > This isn't directly using net::IsSafePortablePathComponent since what
> > is safe for the File System Access API is not the same as what is safe
> > for Downloads. As such currently this duplicates a lot of the
> > implementation of this method, but in a followup we should attempt to
> > unify these two implementations as much as possible.
> >
> > Bug: 1150810, 1154757
> > Change-Id: Iba4c92ef5f1cd924aa22b9dd201762d48b4bbc3b
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2568383
> > Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
> > Reviewed-by: Victor Costan <pwnall@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#833042}
>
> Bug: 1150810
> Bug: 1154757
> Change-Id: I3341b9824a1ac4cbd6f100355960ad55b01f0753
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575370
> Commit-Queue: Victor Costan <pwnall@chromium.org>
> Reviewed-by: Victor Costan <pwnall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#834118}

Bug: 1150810
Bug: 1154757
Change-Id: Ie5cad9a7b2383c89b96e8a7be6cfe75ad2555fa6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2577614
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Auto-Submit: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834598}

content/browser/file_system_access/native_file_system_directory_handle_impl.cc

@@ -4,6 +4,7 @@
 
 #include "content/browser/file_system_access/native_file_system_directory_handle_impl.h"
 
+#include "base/i18n/file_util_icu.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -12,6 +13,7 @@
 #include "content/browser/file_system_access/native_file_system_transfer_token_impl.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "net/base/escape.h"
+#include "net/base/filename_util.h"
 #include "storage/browser/file_system/file_system_context.h"
 #include "storage/browser/file_system/file_system_operation_runner.h"
 #include "storage/common/file_system/file_system_util.h"
@@ -30,27 +32,6 @@ namespace content {
 
 using HandleType = NativeFileSystemPermissionContext::HandleType;
 
-namespace {
-
-// Returns true when |name| contains a path separator like "/".
-bool ContainsPathSeparator(const std::string& name) {
-  const base::FilePath filepath_name = storage::StringToFilePath(name);
-
-  const size_t separator_position =
-      filepath_name.value().find_first_of(base::FilePath::kSeparators);
-
-  return separator_position != base::FilePath::StringType::npos;
-}
-
-// Returns true when |name| is "." or "..".
-bool IsCurrentOrParentDirectory(const std::string& name) {
-  const base::FilePath filepath_name = storage::StringToFilePath(name);
-  return filepath_name.value() == base::FilePath::kCurrentDirectory ||
-         filepath_name.value() == base::FilePath::kParentDirectory;
-}
-
-}  // namespace
-
 NativeFileSystemDirectoryHandleImpl::NativeFileSystemDirectoryHandleImpl(
     NativeFileSystemManagerImpl* manager,
     const BindingContext& context,
@@ -379,9 +360,10 @@ void NativeFileSystemDirectoryHandleImpl::DidReadDirectory(
     blink::mojom::NativeFileSystemErrorPtr get_child_url_result =
         GetChildURL(basename, &child_url);
 
-    // All entries must exist in this directory as a direct child with a valid
-    // |basename|.
-    CHECK_EQ(get_child_url_result->status, NativeFileSystemStatus::kOk);
+    // Skip any entries with names that aren't allowed to be accessed by
+    // this API, such as files with disallowed characters in their names.
+    if (get_child_url_result->status != NativeFileSystemStatus::kOk)
+      continue;
 
     entries.push_back(
         CreateEntry(basename, child_url,
@@ -412,25 +394,97 @@ void NativeFileSystemDirectoryHandleImpl::RemoveEntryImpl(
       url, recurse);
 }
 
+namespace {
+
+// Returns whether the specified extension receives special handling by the
+// Windows shell.
+bool IsShellIntegratedExtension(const base::FilePath::StringType& extension) {
+  base::FilePath::StringType extension_lower = base::ToLowerASCII(extension);
+
+  // .lnk files may be used to execute arbitrary code (see
+  // https://nvd.nist.gov/vuln/detail/CVE-2010-2568).
+  if (extension_lower == FILE_PATH_LITERAL("lnk"))
+    return true;
+
+  // Setting a file's extension to a CLSID may conceal its actual file type on
+  // some Windows versions (see https://nvd.nist.gov/vuln/detail/CVE-2004-0420).
+  if (!extension_lower.empty() &&
+      (extension_lower.front() == FILE_PATH_LITERAL('{')) &&
+      (extension_lower.back() == FILE_PATH_LITERAL('}')))
+    return true;
+  return false;
+}
+
+}  // namespace
+
+// static
+bool NativeFileSystemDirectoryHandleImpl::IsSafePathComponent(
+    const std::string& name) {
+  // This method is similar to net::IsSafePortablePathComponent, with a few
+  // notable differences where the net version does not consider names safe
+  // while here we do want to allow them. These cases are:
+  //  - Names starting with a '.'. These would be hidden files in most file
+  //    managers, but are something we explicitly want to support for the
+  //    File System Access API, for names like .git.
+  //  - Names that end in '.local'. For downloads writing to such files is
+  //    dangerous since it might modify what code is executed when an executable
+  //    is ran from the same directory. For the File System Access API this
+  //    isn't really a problem though, since if a website can write to a .local
+  //    file via a FileSystemDirectoryHandle they can also just modify the
+  //    executables in the directory directly.
+  //
+  // TODO(https://crbug.com/1154757): Unify this with
+  // net::IsSafePortablePathComponent, with the result probably ending up in
+  // base/i18n/file_util_icu.h.
+
+  const base::FilePath component = storage::StringToFilePath(name);
+  // Empty names, or names that contain path separators are invalid.
+  if (component.empty() || component != component.BaseName() ||
+      component != component.StripTrailingSeparators()) {
+    return false;
+  }
+
+  base::string16 component16;
+#if defined(OS_WIN)
+  component16.assign(component.value().begin(), component.value().end());
+#else
+  std::string component8 = component.AsUTF8Unsafe();
+  if (!base::UTF8ToUTF16(component8.c_str(), component8.size(), &component16))
+    return false;
+#endif
+  // base::i18n::IsFilenameLegal blocks names that start with '.', so strip out
+  // a leading '.' before passing it to that method.
+  // TODO(mek): Consider making IsFilenameLegal more flexible to support this
+  // use case.
+  if (component16[0] == '.')
+    component16 = component16.substr(1);
+  if (!base::i18n::IsFilenameLegal(component16))
+    return false;
+
+  base::FilePath::StringType extension = component.Extension();
+  if (!extension.empty())
+    extension.erase(extension.begin());  // Erase preceding '.'.
+  if (IsShellIntegratedExtension(extension))
+    return false;
+
+  if (base::TrimString(component.value(), FILE_PATH_LITERAL("."),
+                       base::TRIM_TRAILING) != component.value()) {
+    return false;
+  }
+
+  if (net::IsReservedNameOnWindows(component.value()))
+    return false;
+
+  return true;
+}
+
 blink::mojom::NativeFileSystemErrorPtr
 NativeFileSystemDirectoryHandleImpl::GetChildURL(
     const std::string& basename,
     storage::FileSystemURL* result) {
-  // TODO(mek): Rather than doing URL serialization and parsing we should just
-  // have a way to get a child FileSystemURL directly from its parent.
-
-  if (basename.empty()) {
-    return native_file_system_error::FromStatus(
-        NativeFileSystemStatus::kInvalidArgument,
-        "Name can't be an empty string.");
-  }
-
-  if (ContainsPathSeparator(basename) || IsCurrentOrParentDirectory(basename)) {
-    // |basename| must refer to a entry that exists in this directory as a
-    // direct child.
+  if (!IsSafePathComponent(basename)) {
     return native_file_system_error::FromStatus(
-        NativeFileSystemStatus::kInvalidArgument,
-        "Name contains invalid characters.");
+        NativeFileSystemStatus::kInvalidArgument, "Name is not allowed.");
   }
 
   const storage::FileSystemURL parent = url();

content/browser/file_system_access/native_file_system_directory_handle_impl.h

@@ -57,6 +57,14 @@ class CONTENT_EXPORT NativeFileSystemDirectoryHandleImpl
       mojo::PendingReceiver<blink::mojom::NativeFileSystemTransferToken> token)
       override;
 
+  // The File System Access API should not give access to files that might
+  // trigger special handling from the operating system. This method is used to
+  // validate that all paths passed to GetFileHandle/GetDirectoryHandle are safe
+  // to be exposed to the web.
+  // TODO(https://crbug.com/1154757): Merge this with
+  // net::IsSafePortablePathComponent.
+  static bool IsSafePathComponent(const std::string& name);
+
  private:
   // This method creates the file if it does not currently exists. I.e. it is
   // the implementation for passing create=true to GetFile.

content/browser/file_system_access/native_file_system_directory_handle_impl_unittest.cc

+// Copyright 2020 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 "content/browser/file_system_access/native_file_system_directory_handle_impl.h"
+
+#include <iterator>
+#include <string>
+
+#include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "base/test/bind.h"
+#include "base/test/task_environment.h"
+#include "build/build_config.h"
+#include "content/browser/file_system_access/fixed_native_file_system_permission_grant.h"
+#include "content/public/test/browser_task_environment.h"
+#include "storage/browser/test/test_file_system_context.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+using storage::FileSystemURL;
+
+class NativeFileSystemDirectoryHandleImplTest : public testing::Test {
+ public:
+  NativeFileSystemDirectoryHandleImplTest()
+      : task_environment_(base::test::TaskEnvironment::MainThreadType::IO) {}
+
+  void SetUp() override {
+    ASSERT_TRUE(dir_.CreateUniqueTempDir());
+
+    file_system_context_ = storage::CreateFileSystemContextForTesting(
+        /*quota_manager_proxy=*/nullptr, dir_.GetPath());
+
+    chrome_blob_context_ = base::MakeRefCounted<ChromeBlobStorageContext>();
+    chrome_blob_context_->InitializeOnIOThread(base::FilePath(),
+                                               base::FilePath(), nullptr);
+
+    manager_ = base::MakeRefCounted<NativeFileSystemManagerImpl>(
+        file_system_context_, chrome_blob_context_,
+        /*permission_context=*/nullptr,
+        /*off_the_record=*/false);
+
+    auto url_and_fs = manager_->CreateFileSystemURLFromPath(
+        test_src_origin_, NativeFileSystemEntryFactory::PathType::kLocal,
+        dir_.GetPath());
+
+    handle_ = std::make_unique<NativeFileSystemDirectoryHandleImpl>(
+        manager_.get(),
+        NativeFileSystemManagerImpl::BindingContext(
+            test_src_origin_, test_src_url_, /*worker_process_id=*/1),
+        url_and_fs.url,
+        NativeFileSystemManagerImpl::SharedHandleState(
+            allow_grant_, allow_grant_, std::move(url_and_fs.file_system)));
+  }
+
+  void TearDown() override { task_environment_.RunUntilIdle(); }
+
+ protected:
+  const GURL test_src_url_ = GURL("http://example.com/foo");
+  const url::Origin test_src_origin_ = url::Origin::Create(test_src_url_);
+
+  BrowserTaskEnvironment task_environment_;
+
+  base::ScopedTempDir dir_;
+  scoped_refptr<storage::FileSystemContext> file_system_context_;
+  scoped_refptr<ChromeBlobStorageContext> chrome_blob_context_;
+  scoped_refptr<NativeFileSystemManagerImpl> manager_;
+
+  scoped_refptr<FixedNativeFileSystemPermissionGrant> allow_grant_ =
+      base::MakeRefCounted<FixedNativeFileSystemPermissionGrant>(
+          FixedNativeFileSystemPermissionGrant::PermissionStatus::GRANTED,
+          base::FilePath());
+  std::unique_ptr<NativeFileSystemDirectoryHandleImpl> handle_;
+};
+
+TEST_F(NativeFileSystemDirectoryHandleImplTest, IsSafePathComponent) {
+  constexpr const char* kSafePathComponents[] = {
+      "a", "a.txt", "a b.txt", "My Computer", ".a", "lnk.zip", "lnk", "a.local",
+  };
+
+  constexpr const char* kUnsafePathComponents[] = {
+      "",
+      ".",
+      "..",
+      "...",
+      "con",
+      "con.zip",
+      "NUL",
+      "NUL.zip",
+      "a.",
+      "a\"a",
+      "a<a",
+      "a>a",
+      "a?a",
+      "a/",
+      "a\\",
+      "a ",
+      "a . .",
+      " Computer",
+      "My Computer.{a}",
+      "My Computer.{20D04FE0-3AEA-1069-A2D8-08002B30309D}",
+      "a\\a",
+      "a.lnk",
+      "a/a",
+      "C:\\",
+      "C:/",
+      "C:",
+  };
+
+  for (const char* component : kSafePathComponents) {
+    EXPECT_TRUE(
+        NativeFileSystemDirectoryHandleImpl::IsSafePathComponent(component))
+        << component;
+  }
+  for (const char* component : kUnsafePathComponents) {
+    EXPECT_FALSE(
+        NativeFileSystemDirectoryHandleImpl::IsSafePathComponent(component))
+        << component;
+  }
+}
+
+namespace {
+class TestNativeFileSystemDirectoryEntriesListener
+    : public blink::mojom::NativeFileSystemDirectoryEntriesListener {
+ public:
+  TestNativeFileSystemDirectoryEntriesListener(
+      std::vector<blink::mojom::NativeFileSystemEntryPtr>* entries,
+      base::OnceClosure done)
+      : entries_(entries), done_(std::move(done)) {}
+
+  void DidReadDirectory(
+      blink::mojom::NativeFileSystemErrorPtr result,
+      std::vector<blink::mojom::NativeFileSystemEntryPtr> entries,
+      bool has_more_entries) override {
+    EXPECT_EQ(result->status, blink::mojom::NativeFileSystemStatus::kOk);
+    entries_->insert(entries_->end(), std::make_move_iterator(entries.begin()),
+                     std::make_move_iterator(entries.end()));
+    if (!has_more_entries) {
+      std::move(done_).Run();
+    }
+  }
+
+ private:
+  std::vector<blink::mojom::NativeFileSystemEntryPtr>* entries_;
+  base::OnceClosure done_;
+};
+}  // namespace
+
+TEST_F(NativeFileSystemDirectoryHandleImplTest, GetEntries) {
+  constexpr const char* kSafeNames[] = {"a", "a.txt", "My Computer", "lnk.txt",
+                                        "a.local"};
+  constexpr const char* kUnsafeNames[] = {
+      "con",  "con.zip", "NUL",   "a.",
+      "a\"a", "a . .",   "a.lnk", "My Computer.{a}",
+  };
+  for (const char* name : kSafeNames) {
+    ASSERT_TRUE(base::WriteFile(dir_.GetPath().AppendASCII(name), "data"))
+        << name;
+  }
+  for (const char* name : kUnsafeNames) {
+    base::FilePath file_path = dir_.GetPath().AppendASCII(name);
+    bool success = base::WriteFile(file_path, "data");
+#if !defined(OS_WIN)
+    // Some of the unsafe names are not legal file names on Windows. This is
+    // okay, and doesn't materially effect the outcome of the test, so just
+    // ignore any failures writing these files to disk.
+    EXPECT_TRUE(success) << "Failed to create file " << file_path;
+#else
+    ignore_result(success);
+#endif
+  }
+
+  std::vector<blink::mojom::NativeFileSystemEntryPtr> entries;
+  base::RunLoop loop;
+  mojo::PendingRemote<blink::mojom::NativeFileSystemDirectoryEntriesListener>
+      listener;
+  mojo::MakeSelfOwnedReceiver(
+      std::make_unique<TestNativeFileSystemDirectoryEntriesListener>(
+          &entries, loop.QuitClosure()),
+      listener.InitWithNewPipeAndPassReceiver());
+  handle_->GetEntries(std::move(listener));
+  loop.Run();
+
+  std::vector<std::string> names;
+  for (const auto& entry : entries) {
+    names.push_back(entry->name);
+  }
+  EXPECT_THAT(names, testing::UnorderedElementsAreArray(kSafeNames));
+}
+
+}  // namespace content

content/browser/file_system_access/native_file_system_file_handle_impl_unittest.cc

@@ -28,8 +28,6 @@
 #include "storage/browser/test/test_file_system_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using storage::FileSystemURL;
-
 namespace content {
 
 using blink::mojom::PermissionStatus;

content/test/BUILD.gn

@@ -1801,6 +1801,7 @@ test("content_unittests") {
     "../browser/file_system/browser_file_system_helper_unittest.cc",
     "../browser/file_system/file_system_operation_runner_unittest.cc",
     "../browser/file_system_access/file_system_chooser_unittest.cc",
+    "../browser/file_system_access/native_file_system_directory_handle_impl_unittest.cc",
     "../browser/file_system_access/native_file_system_file_handle_impl_unittest.cc",
     "../browser/file_system_access/native_file_system_file_writer_impl_unittest.cc",
     "../browser/file_system_access/native_file_system_handle_base_unittest.cc",