Skip to content

GitLab

T
tangled
Commit 455d1c44 authored by Ben Pastene's avatar Ben Pastene Committed by Chromium LUCI CQ
Browse files
Options

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

This reverts commit 00437792.

Reason for revert: suspect to be causing apps.LaunchGallery failures on CrOS
https://bugs.chromium.org/p/chromium/issues/detail?id=1155028#c21

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}

TBR=mek@chromium.org,pwnall@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1150810
Bug: 1154757
Bug: 1155028
Change-Id: I6c0fb2af7096d4f7f47d3e17a40c5a69105808cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2574931Reviewed-by: default avatarBen Pastene <bpastene@chromium.org>
Reviewed-by: default avatarMarijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Ben Pastene <bpastene@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833820}
parent 3f2085ea
Hide whitespace changes
Inline Side-by-side
Showing with 35 additions and 157 deletions
content/browser/file_system_access/native_file_system_directory_handle_impl.cc
View file @ 455d1c44
......@@ -4,7 +4,6 @@
#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"
......@@ -13,7 +12,6 @@
#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"
......@@ -32,6 +30,27 @@ 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,
......@@ -393,97 +412,25 @@ 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) {
if (!IsSafePathComponent(basename)) {
// 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.
return native_file_system_error::FromStatus(
NativeFileSystemStatus::kInvalidArgument, "Name is not allowed.");
NativeFileSystemStatus::kInvalidArgument,
"Name contains invalid characters.");
}
const storage::FileSystemURL parent = url();
......
content/browser/file_system_access/native_file_system_directory_handle_impl.h
View file @ 455d1c44
......@@ -57,14 +57,6 @@ 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 deleted 100644 → 0
View file @ 3f2085ea
// 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 <string>
#include "testing/gtest/include/gtest/gtest.h"
namespace content {
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",
"a\\a",
"C:\\",
"C:/",
"C:",
};
TEST(NativeFileSystemDirectoryHandleImplTest, IsSafePathComponent) {
for (const char* component : kSafePathComponents) {
EXPECT_TRUE(
NativeFileSystemDirectoryHandleImpl::IsSafePathComponent(component))
<< component;
}
for (const char* component : kUnsafePathComponents) {
EXPECT_FALSE(
NativeFileSystemDirectoryHandleImpl::IsSafePathComponent(component))
<< component;
}
}
} // namespace content
content/test/BUILD.gn
View file @ 455d1c44
......@@ -1799,7 +1799,6 @@ 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",
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment