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}

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/run_loop.h"
+#include "base/test/bind.h"
+#include "base/test/task_environment.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.lnk", "My Computer.{a}",
+  };
+  for (const char* name : kSafeNames) {
+    ASSERT_TRUE(base::WriteFile(dir_.GetPath().AppendASCII(name), "data"))
+        << name;
+  }
+  for (const char* name : kUnsafeNames) {
+    ASSERT_TRUE(base::WriteFile(dir_.GetPath().AppendASCII(name), "data"))
+        << name;
+  }
+
+  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

@@ -1799,6 +1799,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",