Add sanitisation for FSP ReadDirectory() results.

Filter out invalid entries in a provided filesystem's ReadDirectory()
results list. Entries are expected to have a valid type and a name that
does not contain certain invalid characters.

Bug: 992321
Change-Id: Ie7e3b47da349892537643cba239d7f88ed4f60ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1888867Reviewed-by: Naoki Fukino <fukino@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712036}

chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc

@@ -25,6 +25,13 @@ const size_t kFakeFileSize = sizeof(kFakeFileText) - 1u;
 const char kFakeFileModificationTime[] = "Fri Apr 25 01:47:53 UTC 2014";
 const char kFakeFileMimeType[] = "text/plain";
 
+constexpr base::FilePath::CharType kBadFakeEntryPath1[] =
+    FILE_PATH_LITERAL("/bad1");
+constexpr char kBadFakeEntryName1[] = "/bad1";
+constexpr base::FilePath::CharType kBadFakeEntryPath2[] =
+    FILE_PATH_LITERAL("/bad2");
+constexpr char kBadFakeEntryName2[] = "bad2";
+
 }  // namespace
 
 const base::FilePath::CharType kFakeFilePath[] =
@@ -50,6 +57,13 @@ FakeProvidedFileSystem::FakeProvidedFileSystem(
   DCHECK(base::Time::FromString(kFakeFileModificationTime, &modification_time));
   AddEntry(base::FilePath(kFakeFilePath), false, kFakeFileName, kFakeFileSize,
            modification_time, kFakeFileMimeType, kFakeFileText);
+
+  // Add a set of bad entries, in the root directory, which should be filtered
+  // out.
+  AddEntry(base::FilePath(kBadFakeEntryPath1), false, kBadFakeEntryName1,
+           kFakeFileSize, modification_time, kFakeFileMimeType, kFakeFileText);
+  AddEntry(base::FilePath(kBadFakeEntryPath2), false, kBadFakeEntryName2,
+           kFakeFileSize, modification_time, kFakeFileMimeType, kFakeFileText);
 }
 
 FakeProvidedFileSystem::~FakeProvidedFileSystem() {}
@@ -156,11 +170,13 @@ AbortCallback FakeProvidedFileSystem::ReadDirectory(
     const base::FilePath file_path = it->first;
     if (file_path == directory_path || directory_path.IsParent(file_path)) {
       const EntryMetadata* const metadata = it->second->metadata.get();
-      entry_list.emplace_back(
-          base::FilePath(*metadata->name),
-          *metadata->is_directory
-              ? filesystem::mojom::FsFileType::DIRECTORY
-              : filesystem::mojom::FsFileType::REGULAR_FILE);
+      filesystem::mojom::FsFileType entry_type =
+          *metadata->is_directory ? filesystem::mojom::FsFileType::DIRECTORY
+                                  : filesystem::mojom::FsFileType::REGULAR_FILE;
+      if (*metadata->name == kBadFakeEntryName2) {
+        entry_type = static_cast<filesystem::mojom::FsFileType>(7);
+      }
+      entry_list.emplace_back(base::FilePath(*metadata->name), entry_type);
     }
   }
 

chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util.cc

@@ -4,12 +4,15 @@
 
 #include "chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/stl_util.h"
 #include "base/task/post_task.h"
 #include "chrome/browser/chromeos/file_system_provider/mount_path_util.h"
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
@@ -108,6 +111,23 @@ void OnReadDirectory(storage::AsyncFileUtil::ReadDirectoryCallback callback,
                      base::File::Error result,
                      storage::AsyncFileUtil::EntryList entry_list,
                      bool has_more) {
+  auto new_end_it =
+      std::remove_if(entry_list.begin(), entry_list.end(),
+                     [](const filesystem::mojom::DirectoryEntry& entry) {
+                       if (!filesystem::mojom::IsKnownEnumValue(entry.type)) {
+                         return true;
+                       }
+                       if (entry.name.empty() || entry.name.value() == "." ||
+                           entry.name.value() == ".." ||
+                           base::Contains(entry.name.value(), '\0') ||
+                           base::Contains(entry.name.value(), '/') ||
+                           base::Contains(entry.name.value(), '\\')) {
+                         return true;
+                       }
+                       return false;
+                     });
+  entry_list.erase(new_end_it, entry_list.end());
+
   base::PostTask(
       FROM_HERE, {BrowserThread::IO},
       base::BindOnce(callback, result, std::move(entry_list), has_more));

chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util_unittest.cc

@@ -74,6 +74,7 @@ class EventLogger {
                        storage::AsyncFileUtil::EntryList file_list,
                        bool has_more) {
     result_.reset(new base::File::Error(error));
+    read_directory_list_ = std::move(file_list);
   }
 
   void OnCreateSnapshotFile(
@@ -88,8 +89,13 @@ class EventLogger {
 
   base::File::Error* result() { return result_.get(); }
 
+  const storage::AsyncFileUtil::EntryList& read_directory_list() {
+    return read_directory_list_;
+  }
+
  private:
   std::unique_ptr<base::File::Error> result_;
+  storage::AsyncFileUtil::EntryList read_directory_list_;
   DISALLOW_COPY_AND_ASSIGN(EventLogger);
 };
 
@@ -289,6 +295,22 @@ TEST_F(FileSystemProviderProviderAsyncFileUtilTest, ReadDirectory) {
   EXPECT_EQ(base::File::FILE_OK, *logger.result());
 }
 
+TEST_F(FileSystemProviderProviderAsyncFileUtilTest,
+       ReadDirectory_SanitiseResultsList) {
+  EventLogger logger;
+
+  async_file_util_->ReadDirectory(
+      CreateOperationContext(), root_url_,
+      base::Bind(&EventLogger::OnReadDirectory, base::Unretained(&logger)));
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_TRUE(logger.result());
+  EXPECT_EQ(base::File::FILE_OK, *logger.result());
+  EXPECT_EQ(1U, logger.read_directory_list().size());
+  EXPECT_EQ(base::FilePath(kFakeFilePath + 1 /* No leading slash. */),
+            logger.read_directory_list()[0].name);
+}
+
 TEST_F(FileSystemProviderProviderAsyncFileUtilTest, Touch) {
   EventLogger logger;