diff options
| author | kinuko@chromium.org <kinuko@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-09 02:42:24 +0000 |
|---|---|---|
| committer | kinuko@chromium.org <kinuko@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-09 02:42:24 +0000 |
| commit | ed7be3bf80811a41e967177aa40cd78dca96c7e0 (patch) | |
| tree | 04ebf3bb82905ff525c8b507a809b37c8fcab191 /chrome/browser/file_system | |
| parent | ba4f1137a6f051c668857f850b00373a7adc0253 (diff) | |
| download | chromium_src-ed7be3bf80811a41e967177aa40cd78dca96c7e0.zip chromium_src-ed7be3bf80811a41e967177aa40cd78dca96c7e0.tar.gz chromium_src-ed7be3bf80811a41e967177aa40cd78dca96c7e0.tar.bz2 | |
Check restricted names/chars for FileSystem API
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#naming-restrictions
(We do the same check in the renderer, but would be better to do this in the browser (too))
Also addressed the memory leak issue.
BUG=56433,58474
TEST=FileSystemHostContextTest.IsRestrictedName
Review URL: http://codereview.chromium.org/3525018
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62063 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/file_system')
5 files changed, 198 insertions, 23 deletions
diff --git a/chrome/browser/file_system/file_system_dispatcher_host.cc b/chrome/browser/file_system/file_system_dispatcher_host.cc index 28ffe03..503e85b 100644 --- a/chrome/browser/file_system/file_system_dispatcher_host.cc +++ b/chrome/browser/file_system/file_system_dispatcher_host.cc @@ -161,6 +161,7 @@ void FileSystemDispatcherHost::OnMove( int request_id, const FilePath& src_path, const FilePath& dest_path) { if (!CheckValidFileSystemPath(src_path, request_id) || !CheckValidFileSystemPath(dest_path, request_id) || + !CheckIfFilePathIsSafe(dest_path, request_id) || !CheckQuotaForPath(dest_path, FileSystemQuota::kUnknownSize, request_id)) return; @@ -171,6 +172,7 @@ void FileSystemDispatcherHost::OnCopy( int request_id, const FilePath& src_path, const FilePath& dest_path) { if (!CheckValidFileSystemPath(src_path, request_id) || !CheckValidFileSystemPath(dest_path, request_id) || + !CheckIfFilePathIsSafe(dest_path, request_id) || !CheckQuotaForPath(dest_path, FileSystemQuota::kUnknownSize, request_id)) return; @@ -195,6 +197,7 @@ void FileSystemDispatcherHost::OnCreate( int request_id, const FilePath& path, bool exclusive, bool is_directory, bool recursive) { if (!CheckValidFileSystemPath(path, request_id) || + !CheckIfFilePathIsSafe(path, request_id) || !CheckQuotaForPath(path, 0L, request_id)) return; if (is_directory) @@ -226,6 +229,7 @@ void FileSystemDispatcherHost::OnWrite( const GURL& blob_url, int64 offset) { if (!CheckValidFileSystemPath(path, request_id) || + !CheckIfFilePathIsSafe(path, request_id) || !CheckQuotaForPath(path, FileSystemQuota::kUnknownSize, request_id)) return; GetNewOperation(request_id)->Write( @@ -246,7 +250,8 @@ void FileSystemDispatcherHost::OnTouchFile( const FilePath& path, const base::Time& last_access_time, const base::Time& last_modified_time) { - if (!CheckValidFileSystemPath(path, request_id)) + if (!CheckValidFileSystemPath(path, request_id) || + !CheckIfFilePathIsSafe(path, request_id)) return; GetNewOperation(request_id)->TouchFile( path, last_access_time, last_modified_time); @@ -307,6 +312,16 @@ bool FileSystemDispatcherHost::CheckQuotaForPath( return true; } +bool FileSystemDispatcherHost::CheckIfFilePathIsSafe( + const FilePath& path, int request_id) { + if (context_->IsRestrictedFileName(path.BaseName())) { + Send(new ViewMsg_FileSystem_DidFail( + request_id, base::PLATFORM_FILE_ERROR_SECURITY)); + return false; + } + return true; +} + fileapi::FileSystemOperation* FileSystemDispatcherHost::GetNewOperation( int request_id) { BrowserFileSystemCallbackDispatcher* dispatcher = diff --git a/chrome/browser/file_system/file_system_dispatcher_host.h b/chrome/browser/file_system/file_system_dispatcher_host.h index 33e6011..9387ba8 100644 --- a/chrome/browser/file_system/file_system_dispatcher_host.h +++ b/chrome/browser/file_system/file_system_dispatcher_host.h @@ -91,6 +91,12 @@ class FileSystemDispatcherHost // and returns false. bool CheckQuotaForPath(const FilePath& path, int64 growth, int request_id); + // Checks if a given |path| does not contain any restricted names/chars + // for new files. Returns true if the given |path| is safe. + // Otherwise it sends back a security error code to the dispatcher and + // returns false. + bool CheckIfFilePathIsSafe(const FilePath& path, int request_id); + // The sender to be used for sending out IPC messages. IPC::Message::Sender* message_sender_; diff --git a/chrome/browser/file_system/file_system_host_context.cc b/chrome/browser/file_system/file_system_host_context.cc index e7b1f5c..37c2b8c 100644 --- a/chrome/browser/file_system/file_system_host_context.cc +++ b/chrome/browser/file_system/file_system_host_context.cc @@ -30,6 +30,19 @@ const char FileSystemHostContext::kTemporaryName[] = "Temporary"; namespace { +// Restricted names. +// http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#naming-restrictions +static const char* const kRestrictedNames[] = { + "con", "prn", "aux", "nul", + "com1", "com2", "com3", "com4", "com5", "com6", "com7", "com8", "com9", + "lpt1", "lpt2", "lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9", +}; + +// Restricted chars. +static const FilePath::CharType kRestrictedChars[] = { + '/', '\\', '<', '>', ':', '?', '*', '"', '|', +}; + static inline std::string FilePathStringToASCII( const FilePath::StringType& path_string) { #if defined(OS_WIN) @@ -46,9 +59,9 @@ FileSystemHostContext::FileSystemHostContext( : base_path_(data_path.Append(kFileSystemDirectory)), is_incognito_(is_incognito), quota_manager_(new fileapi::FileSystemQuota()) { - const CommandLine& command_line = *CommandLine::ForCurrentProcess(); - allow_file_access_from_files_ = command_line.HasSwitch( - switches::kAllowFileAccessFromFiles); + const CommandLine& command_line = *CommandLine::ForCurrentProcess(); + allow_file_access_from_files_ = command_line.HasSwitch( + switches::kAllowFileAccessFromFiles); } bool FileSystemHostContext::GetFileSystemRootPath( @@ -104,6 +117,7 @@ bool FileSystemHostContext::CheckValidFileSystemPath( // must be kPersistent or kTemporary. if (!IsStringASCII(components[1])) return false; + std::string ascii_type_component = FilePathStringToASCII(components[1]); if (ascii_type_component != kPersistentName && ascii_type_component != kTemporaryName) @@ -149,6 +163,36 @@ bool FileSystemHostContext::IsAllowedScheme(const GURL& url) const { (url.SchemeIsFile() && allow_file_access_from_files_); } +bool FileSystemHostContext::IsRestrictedFileName( + const FilePath& filename) const { + if (filename.value().size() == 0) + return false; + + if (IsWhitespace(filename.value()[filename.value().size() - 1]) || + filename.value()[filename.value().size() - 1] == '.') + return true; + + std::string filename_lower = StringToLowerASCII( + FilePathStringToASCII(filename.value())); + + for (size_t i = 0; i < arraysize(kRestrictedNames); ++i) { + // Exact match. + if (filename_lower == kRestrictedNames[i]) + return true; + // Starts with "RESTRICTED_NAME.". + if (filename_lower.find(std::string(kRestrictedNames[i]) + ".") == 0) + return true; + } + + for (size_t i = 0; i < arraysize(kRestrictedChars); ++i) { + if (filename.value().find(kRestrictedChars[i]) != + FilePath::StringType::npos) + return true; + } + + return false; +} + bool FileSystemHostContext::CheckOriginQuota(const GURL& url, int64 growth) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); // If allow-file-access-from-files flag is explicitly given and the scheme diff --git a/chrome/browser/file_system/file_system_host_context.h b/chrome/browser/file_system/file_system_host_context.h index a1141c0..be5b5bd 100644 --- a/chrome/browser/file_system/file_system_host_context.h +++ b/chrome/browser/file_system/file_system_host_context.h @@ -6,7 +6,6 @@ #define CHROME_BROWSER_FILE_SYSTEM_FILE_SYSTEM_HOST_CONTEXT_H_ #include "base/file_path.h" -#include "base/gtest_prod_util.h" #include "base/ref_counted.h" #include "base/scoped_ptr.h" #include "chrome/browser/chrome_thread.h" @@ -33,7 +32,7 @@ class FileSystemHostContext FilePath* root_path, std::string* name) const; - // Check if the given |path| is in the FileSystem base directory. + // Checks if a given |path| is in the FileSystem base directory. bool CheckValidFileSystemPath(const FilePath& path) const; // Retrieves the origin URL for the given |path| and populates @@ -45,6 +44,9 @@ class FileSystemHostContext // filesystem. bool IsAllowedScheme(const GURL& url) const; + // Checks if a given |filename| contains any restricted names/chars in it. + bool IsRestrictedFileName(const FilePath& filename) const; + // Quota related methods. bool CheckOriginQuota(const GURL& url, int64 growth); void SetOriginQuotaUnlimited(const GURL& url); @@ -66,8 +68,6 @@ class FileSystemHostContext scoped_ptr<fileapi::FileSystemQuota> quota_manager_; - FRIEND_TEST_ALL_PREFIXES(FileSystemHostContextTest, GetOriginFromPath); - DISALLOW_IMPLICIT_CONSTRUCTORS(FileSystemHostContext); }; diff --git a/chrome/browser/file_system/file_system_host_context_unittest.cc b/chrome/browser/file_system/file_system_host_context_unittest.cc index f91bb04..2d58588 100644 --- a/chrome/browser/file_system/file_system_host_context_unittest.cc +++ b/chrome/browser/file_system/file_system_host_context_unittest.cc @@ -5,7 +5,7 @@ #include "base/basictypes.h" #include "base/file_path.h" #include "base/scoped_ptr.h" -#include "chrome/browser/file_system/file_system_host_context.h" +#include "base/scoped_temp_dir.h" #include "chrome/browser/file_system/file_system_host_context.h" #include "googleurl/src/gurl.h" #include "testing/gtest/include/gtest/gtest.h" @@ -56,16 +56,108 @@ const struct CheckValidPathTest { { FILE_PATH_LITERAL("a/b/../c/.."), false, }, }; +const struct IsRestrictedNameTest { + FilePath::StringType name; + bool expected_dangerous; +} kIsRestrictedNameTestCases[] = { + // Name that has restricted names in it. + { FILE_PATH_LITERAL("con"), true, }, + { FILE_PATH_LITERAL("Con.txt"), true, }, + { FILE_PATH_LITERAL("Prn.png"), true, }, + { FILE_PATH_LITERAL("AUX"), true, }, + { FILE_PATH_LITERAL("nUl."), true, }, + { FILE_PATH_LITERAL("coM1"), true, }, + { FILE_PATH_LITERAL("COM3.com"), true, }, + { FILE_PATH_LITERAL("cOM7"), true, }, + { FILE_PATH_LITERAL("com9"), true, }, + { FILE_PATH_LITERAL("lpT1"), true, }, + { FILE_PATH_LITERAL("LPT4.com"), true, }, + { FILE_PATH_LITERAL("lPT8"), true, }, + { FILE_PATH_LITERAL("lPT9"), true, }, + // Similar but safe cases. + { FILE_PATH_LITERAL("con3"), false, }, + { FILE_PATH_LITERAL("PrnImage.png"), false, }, + { FILE_PATH_LITERAL("AUXX"), false, }, + { FILE_PATH_LITERAL("NULL"), false, }, + { FILE_PATH_LITERAL("coM0"), false, }, + { FILE_PATH_LITERAL("COM.com"), false, }, + { FILE_PATH_LITERAL("lpT0"), false, }, + { FILE_PATH_LITERAL("LPT.com"), false, }, + + // Ends with period or whitespace. + { FILE_PATH_LITERAL("b "), true, }, + { FILE_PATH_LITERAL("b\t"), true, }, + { FILE_PATH_LITERAL("b\n"), true, }, + { FILE_PATH_LITERAL("b\r\n"), true, }, + { FILE_PATH_LITERAL("b."), true, }, + { FILE_PATH_LITERAL("b.."), true, }, + // Similar but safe cases. + { FILE_PATH_LITERAL("b c"), false, }, + { FILE_PATH_LITERAL("b\tc"), false, }, + { FILE_PATH_LITERAL("b\nc"), false, }, + { FILE_PATH_LITERAL("b\r\nc"), false, }, + { FILE_PATH_LITERAL("b c d e f"), false, }, + { FILE_PATH_LITERAL("b.c"), false, }, + { FILE_PATH_LITERAL("b..c"), false, }, + + // Name that has restricted chars in it. + { FILE_PATH_LITERAL("a\\b"), true, }, + { FILE_PATH_LITERAL("a/b"), true, }, + { FILE_PATH_LITERAL("a<b"), true, }, + { FILE_PATH_LITERAL("a>b"), true, }, + { FILE_PATH_LITERAL("a:b"), true, }, + { FILE_PATH_LITERAL("a?b"), true, }, + { FILE_PATH_LITERAL("a|b"), true, }, + { FILE_PATH_LITERAL("ab\\"), true, }, + { FILE_PATH_LITERAL("ab/.txt"), true, }, + { FILE_PATH_LITERAL("ab<.txt"), true, }, + { FILE_PATH_LITERAL("ab>.txt"), true, }, + { FILE_PATH_LITERAL("ab:.txt"), true, }, + { FILE_PATH_LITERAL("ab?.txt"), true, }, + { FILE_PATH_LITERAL("ab|.txt"), true, }, + { FILE_PATH_LITERAL("\\ab"), true, }, + { FILE_PATH_LITERAL("/ab"), true, }, + { FILE_PATH_LITERAL("<ab"), true, }, + { FILE_PATH_LITERAL(">ab"), true, }, + { FILE_PATH_LITERAL(":ab"), true, }, + { FILE_PATH_LITERAL("?ab"), true, }, + { FILE_PATH_LITERAL("|ab"), true, }, +}; + } // namespace -TEST(FileSystemHostContextTest, GetRootPath) { +class FileSystemHostContextTest : public testing::Test { + public: + FileSystemHostContextTest() + : io_thread_(ChromeThread::IO, &message_loop_), + data_path_(kTestDataPath) { + } + + void TearDown() { + message_loop_.RunAllPending(); + } + + ~FileSystemHostContextTest() { + message_loop_.RunAllPending(); + } + + scoped_refptr<FileSystemHostContext> GetHostContext(bool incognite) { + return new FileSystemHostContext(data_path_, incognite); + } + + private: + MessageLoopForIO message_loop_; + BrowserThread io_thread_; + FilePath data_path_; +}; + +TEST_F(FileSystemHostContextTest, GetRootPath) { for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kRootPathTestCases); ++i) { SCOPED_TRACE(testing::Message() << "RootPath #" << i << " " << kRootPathTestCases[i].expected_path); - scoped_refptr<FileSystemHostContext> context( - new FileSystemHostContext(FilePath(kTestDataPath), - kRootPathTestCases[i].off_the_record)); + scoped_refptr<FileSystemHostContext> context = GetHostContext( + kRootPathTestCases[i].off_the_record); FilePath root_path; bool result = context->GetFileSystemRootPath( @@ -81,23 +173,41 @@ TEST(FileSystemHostContextTest, GetRootPath) { } } -TEST(FileSystemHostContextTest, CheckValidPath) { +TEST_F(FileSystemHostContextTest, CheckValidPath) { + scoped_refptr<FileSystemHostContext> context = GetHostContext(false); + FilePath root_path; + EXPECT_TRUE(context->GetFileSystemRootPath( + GURL("http://foo.com/"), fileapi::kFileSystemTypePersistent, + &root_path, NULL)); + + // The root path must be valid, but upper directories or directories + // that are not in our temporary or persistent directory must be + // evaluated invalid. + EXPECT_TRUE(context->CheckValidFileSystemPath(root_path)); + EXPECT_FALSE(context->CheckValidFileSystemPath(root_path.DirName())); + EXPECT_FALSE(context->CheckValidFileSystemPath( + root_path.DirName().DirName())); + EXPECT_FALSE(context->CheckValidFileSystemPath( + root_path.DirName().AppendASCII("ArbitraryName"))); + for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kCheckValidPathTestCases); ++i) { SCOPED_TRACE(testing::Message() << "CheckValidPath #" << i << " " << kCheckValidPathTestCases[i].path); - - scoped_refptr<FileSystemHostContext> context( - new FileSystemHostContext(FilePath(kTestDataPath), false)); - - FilePath root_path; - EXPECT_TRUE(context->GetFileSystemRootPath( - GURL("http://foo.com/"), fileapi::kFileSystemTypePersistent, - &root_path, NULL)); FilePath path(kCheckValidPathTestCases[i].path); if (!path.IsAbsolute()) path = root_path.Append(path); - EXPECT_EQ(kCheckValidPathTestCases[i].expected_valid, context->CheckValidFileSystemPath(path)); } } + +TEST_F(FileSystemHostContextTest, IsRestrictedName) { + scoped_refptr<FileSystemHostContext> context = GetHostContext(false); + for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kIsRestrictedNameTestCases); ++i) { + SCOPED_TRACE(testing::Message() << "IsRestrictedName #" << i << " " + << kIsRestrictedNameTestCases[i].name); + FilePath name(kIsRestrictedNameTestCases[i].name); + EXPECT_EQ(kIsRestrictedNameTestCases[i].expected_dangerous, + context->IsRestrictedFileName(name)); + } +} |