Add path traversal protection to Move and CopyFile too.
These functions are used a lot in IPC receivers to manage storage.
See http://src.chromium.org/viewvc/chrome?view=rev&revision=175642
Review URL: https://codereview.chromium.org/12223014
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@181045 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: 3cd2c1c88d2646a3338cfa7888f06fe321061053
diff --git a/base/file_util.cc b/base/file_util.cc
index 7efb22b..07f2771 100644
--- a/base/file_util.cc
+++ b/base/file_util.cc
@@ -77,6 +77,18 @@
value.insert(last_dot, suffix);
}
+bool Move(const FilePath& from_path, const FilePath& to_path) {
+ if (from_path.ReferencesParent() || to_path.ReferencesParent())
+ return false;
+ return MoveUnsafe(from_path, to_path);
+}
+
+bool CopyFile(const FilePath& from_path, const FilePath& to_path) {
+ if (from_path.ReferencesParent() || to_path.ReferencesParent())
+ return false;
+ return CopyFileUnsafe(from_path, to_path);
+}
+
bool ContentsEqual(const FilePath& filename1, const FilePath& filename2) {
// We open the file in binary format even if they are text files because
// we are just comparing that bytes are exactly same in both files and not
diff --git a/base/file_util.h b/base/file_util.h
index b450087..254f326 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -120,9 +120,15 @@
// If a simple rename is not possible, such as in the case where the paths are
// on different volumes, this will attempt to copy and delete. Returns
// true for success.
+// This function fails if either path contains traversal components ('..').
BASE_EXPORT bool Move(const base::FilePath& from_path,
const base::FilePath& to_path);
+// Same as Move but allows paths with traversal components.
+// Use only with extreme care.
+BASE_EXPORT bool MoveUnsafe(const base::FilePath& from_path,
+ const base::FilePath& to_path);
+
// Renames file |from_path| to |to_path|. Both paths must be on the same
// volume, or the function will fail. Destination file will be created
// if it doesn't exist. Prefer this function over Move when dealing with
@@ -132,9 +138,15 @@
const base::FilePath& to_path);
// Copies a single file. Use CopyDirectory to copy directories.
+// This function fails if either path contains traversal components ('..').
BASE_EXPORT bool CopyFile(const base::FilePath& from_path,
const base::FilePath& to_path);
+// Same as CopyFile but allows paths with traversal components.
+// Use only with extreme care.
+BASE_EXPORT bool CopyFileUnsafe(const base::FilePath& from_path,
+ const base::FilePath& to_path);
+
// Copies the given path, and optionally all subdirectories and their contents
// as well.
// If there are files existing under to_path, always overwrite.
diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index 49cf873..ebe3fbf 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -249,7 +249,7 @@
return success;
}
-bool Move(const FilePath& from_path, const FilePath& to_path) {
+bool MoveUnsafe(const FilePath& from_path, const FilePath& to_path) {
base::ThreadRestrictions::AssertIOAllowed();
// Windows compatibility: if to_path exists, from_path and to_path
// must be the same type, either both files, or both directories.
@@ -1012,7 +1012,7 @@
return FilePath("/tmp");
}
-bool CopyFile(const FilePath& from_path, const FilePath& to_path) {
+bool CopyFileUnsafe(const FilePath& from_path, const FilePath& to_path) {
base::ThreadRestrictions::AssertIOAllowed();
int infile = HANDLE_EINTR(open(from_path.value().c_str(), O_RDONLY));
if (infile < 0)
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index d4ce43c..612f6cf 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -1149,8 +1149,8 @@
ASSERT_TRUE(file_util::PathExists(dir_name_from));
// Create a file under the directory
- FilePath file_name_from =
- dir_name_from.Append(FILE_PATH_LITERAL("Move_Test_File.txt"));
+ FilePath txt_file_name(FILE_PATH_LITERAL("Move_Test_File.txt"));
+ FilePath file_name_from = dir_name_from.Append(txt_file_name);
CreateTextFile(file_name_from, L"Gooooooooooooooooooooogle");
ASSERT_TRUE(file_util::PathExists(file_name_from));
@@ -1169,6 +1169,17 @@
EXPECT_FALSE(file_util::PathExists(file_name_from));
EXPECT_TRUE(file_util::PathExists(dir_name_to));
EXPECT_TRUE(file_util::PathExists(file_name_to));
+
+ // Test path traversal.
+ file_name_from = dir_name_to.Append(txt_file_name);
+ file_name_to = dir_name_to.Append(FILE_PATH_LITERAL(".."));
+ file_name_to = file_name_to.Append(txt_file_name);
+ EXPECT_FALSE(file_util::Move(file_name_from, file_name_to));
+ EXPECT_TRUE(file_util::PathExists(file_name_from));
+ EXPECT_FALSE(file_util::PathExists(file_name_to));
+ EXPECT_TRUE(file_util::MoveUnsafe(file_name_from, file_name_to));
+ EXPECT_FALSE(file_util::PathExists(file_name_from));
+ EXPECT_TRUE(file_util::PathExists(file_name_to));
}
TEST_F(FileUtilTest, MoveExist) {
@@ -1525,7 +1536,8 @@
FilePath dest_file2(dir_name_from);
dest_file2 = dest_file2.AppendASCII("..");
dest_file2 = dest_file2.AppendASCII("DestFile.txt");
- ASSERT_TRUE(file_util::CopyFile(file_name_from, dest_file2));
+ ASSERT_FALSE(file_util::CopyFile(file_name_from, dest_file2));
+ ASSERT_TRUE(file_util::CopyFileUnsafe(file_name_from, dest_file2));
FilePath dest_file2_test(dir_name_from);
dest_file2_test = dest_file2_test.DirName();