Don't allow '\0' characters in FilePath.

BUG=169777


Review URL: https://chromiumcodereview.appspot.com/11642041

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@179141 0039d316-1c4b-4281-b951-d872f2087c98

base/file_path.cc

@@ -47,6 +47,8 @@ namespace {
 const char* kCommonDoubleExtensionSuffixes[] = { "gz", "z", "bz2" };
 const char* kCommonDoubleExtensions[] = { "user.js" };
 
+const FilePath::CharType kStringTerminator = FILE_PATH_LITERAL('\0');
+
 // If this FilePath contains a drive letter specification, returns the
 // position of the last character of the drive letter specification,
 // otherwise returns npos.  This can only be true on Windows, when a pathname
@@ -182,6 +184,9 @@ FilePath::FilePath(const FilePath& that) : path_(that.path_) {
 }
 
 FilePath::FilePath(const StringType& path) : path_(path) {
+  StringType::size_type nul_pos = path_.find(kStringTerminator);
+  if (nul_pos != StringType::npos)
+    path_.erase(nul_pos, StringType::npos);
 }
 
 FilePath::~FilePath() {
@@ -454,7 +459,17 @@ bool FilePath::MatchesExtension(const StringType& extension) const {
 }
 
 FilePath FilePath::Append(const StringType& component) const {
-  DCHECK(!IsPathAbsolute(component));
+  const StringType* appended = &component;
+  StringType without_nuls;
+
+  StringType::size_type nul_pos = component.find(kStringTerminator);
+  if (nul_pos != StringType::npos) {
+    without_nuls = component.substr(0, nul_pos);
+    appended = &without_nuls;
+  }
+
+  DCHECK(!IsPathAbsolute(*appended));
+
   if (path_.compare(kCurrentDirectory) == 0) {
     // Append normally doesn't do any normalization, but as a special case,
     // when appending to kCurrentDirectory, just return a new path for the
@@ -463,7 +478,7 @@ FilePath FilePath::Append(const StringType& component) const {
     // it's likely in practice to wind up with FilePath objects containing
     // only kCurrentDirectory when calling DirName on a single relative path
     // component.
-    return FilePath(component);
+    return FilePath(*appended);
   }
 
   FilePath new_path(path_);
@@ -472,7 +487,7 @@ FilePath FilePath::Append(const StringType& component) const {
   // Don't append a separator if the path is empty (indicating the current
   // directory) or if the path component is empty (indicating nothing to
   // append).
-  if (component.length() > 0 && new_path.path_.length() > 0) {
+  if (appended->length() > 0 && new_path.path_.length() > 0) {
     // Don't append a separator if the path still ends with a trailing
     // separator after stripping (indicating the root directory).
     if (!IsSeparator(new_path.path_[new_path.path_.length() - 1])) {
@@ -483,7 +498,7 @@ FilePath FilePath::Append(const StringType& component) const {
     }
   }
 
-  new_path.path_.append(component);
+  new_path.path_.append(*appended);
   return new_path;
 }
 
@@ -589,7 +604,7 @@ FilePath FilePath::FromUTF8Unsafe(const std::string& utf8) {
 }
 #endif
 
-void FilePath::WriteToPickle(Pickle* pickle) {
+void FilePath::WriteToPickle(Pickle* pickle) const {
 #if defined(OS_WIN)
   pickle->WriteString16(path_);
 #else
@@ -606,6 +621,9 @@ bool FilePath::ReadFromPickle(PickleIterator* iter) {
     return false;
 #endif
 
+  if (path_.find(kStringTerminator) != StringType::npos)
+    return false;
+
   return true;
 }
 

base/file_path.h

@@ -53,6 +53,8 @@
 // between char[]-based pathnames on POSIX systems and wchar_t[]-based
 // pathnames on Windows.
 //
+// Paths can't contain NULs as a precaution agaist premature truncation.
+//
 // Because a FilePath object should not be instantiated at the global scope,
 // instead, use a FilePath::CharType[] and initialize it with
 // FILE_PATH_LITERAL.  At runtime, a FilePath object can be created from the
@@ -343,7 +345,7 @@ class BASE_EXPORT FilePath {
   // AsUTF8Unsafe() for details.
   static FilePath FromUTF8Unsafe(const std::string& utf8);
 
-  void WriteToPickle(Pickle* pickle);
+  void WriteToPickle(Pickle* pickle) const;
   bool ReadFromPickle(PickleIterator* iter);
 
   // Normalize all path separators to backslash on Windows

base/file_path_unittest.cc

@@ -12,6 +12,9 @@
 // This macro helps avoid wrapped lines in the test structs.
 #define FPL(x) FILE_PATH_LITERAL(x)
 
+// This macro constructs strings which can contain NULs.
+#define FPS(x) FilePath::StringType(FPL(x), arraysize(FPL(x)) - 1)
+
 struct UnaryTestData {
   const FilePath::CharType* input;
   const FilePath::CharType* expected;
@@ -1115,6 +1118,40 @@ TEST_F(FilePathTest, FromUTF8Unsafe_And_AsUTF8Unsafe) {
   }
 }
 
+TEST_F(FilePathTest, ConstructWithNUL) {
+  // Assert FPS() works.
+  ASSERT_TRUE(FPS("a\0b").length() == 3);
+
+  // Test constructor strips '\0'
+  FilePath path(FPS("a\0b"));
+  EXPECT_TRUE(path.value().length() == 1);
+  EXPECT_EQ(path.value(), FPL("a"));
+}
+
+TEST_F(FilePathTest, AppendWithNUL) {
+  // Assert FPS() works.
+  ASSERT_TRUE(FPS("b\0b").length() == 3);
+
+  // Test Append() strips '\0'
+  FilePath path(FPL("a"));
+  path = path.Append(FPS("b\0b"));
+  EXPECT_TRUE(path.value().length() == 3);
+#if defined(FILE_PATH_USES_WIN_SEPARATORS)
+  EXPECT_EQ(path.value(), FPL("a\\b"));
+#else
+  EXPECT_EQ(path.value(), FPL("a/b"));
+#endif
+}
+
+TEST_F(FilePathTest, ReferencesParentWithNUL) {
+  // Assert FPS() works.
+  ASSERT_TRUE(FPS("..\0").length() == 3);
+
+  // Test ReferencesParent() doesn't break with "..\0"
+  FilePath path(FPS("..\0"));
+  EXPECT_TRUE(path.ReferencesParent());
+}
+
 #if defined(FILE_PATH_USES_WIN_SEPARATORS)
 TEST_F(FilePathTest, NormalizePathSeparators) {
   const struct UnaryTestData cases[] = {

content/browser/download/save_package_unittest.cc

@@ -109,7 +109,7 @@ class SavePackageTest : public RenderViewHostImplTestHarness {
         temp_dir_.path().AppendASCII("testfile_files"));
 
     // We need to construct a path that is *almost* kMaxFilePathLength long
-    long_file_name.resize(kMaxFilePathLength + long_file_name.length());
+    long_file_name.reserve(kMaxFilePathLength + long_file_name.length());
     while (long_file_name.length() < kMaxFilePathLength)
       long_file_name += long_file_name;
     long_file_name.resize(

ipc/ipc_message_utils.cc

@@ -489,20 +489,13 @@ void ParamTraits<base::FileDescriptor>::Log(const param_type& p,
 #endif  // defined(OS_POSIX)
 
 void ParamTraits<FilePath>::Write(Message* m, const param_type& p) {
-  ParamTraits<FilePath::StringType>::Write(m, p.value());
+  p.WriteToPickle(m);
 }
 
 bool ParamTraits<FilePath>::Read(const Message* m,
                                  PickleIterator* iter,
                                  param_type* r) {
-  FilePath::StringType value;
+  return r->ReadFromPickle(iter);
-  if (!ParamTraits<FilePath::StringType>::Read(m, iter, &value))
-    return false;
-  // Reject embedded NULs as they can cause security checks to go awry.
-  if (value.find(FILE_PATH_LITERAL('\0')) != FilePath::StringType::npos)
-    return false;
-  *r = FilePath(value);
-  return true;
 }
 
 void ParamTraits<FilePath>::Log(const param_type& p, std::string* l) {

ipc/ipc_message_utils_unittest.cc

@@ -55,17 +55,19 @@ TEST(IPCMessageUtilsTest, NestedMessages) {
 
 // Tests that detection of various bad parameters is working correctly.
 TEST(IPCMessageUtilsTest, ParameterValidation) {
+  FilePath::StringType ok_string(FILE_PATH_LITERAL("hello"), 5);
+  FilePath::StringType bad_string(FILE_PATH_LITERAL("hel\0o"), 5);
+
+  // Change this if ParamTraits<FilePath>::Write() changes.
   IPC::Message message;
-  FilePath::StringType okString(FILE_PATH_LITERAL("hello"), 5);
+  ParamTraits<FilePath::StringType>::Write(&message, ok_string);
-  FilePath::StringType badString(FILE_PATH_LITERAL("hel\0o"), 5);
+  ParamTraits<FilePath::StringType>::Write(&message, bad_string);
-  FilePath okPath(okString);
-  FilePath badPath(badString);
-  ParamTraits<FilePath>::Write(&message, okPath);
-  ParamTraits<FilePath>::Write(&message, badPath);
 
   PickleIterator iter(message);
-  ASSERT_TRUE(ParamTraits<FilePath>::Read(&message, &iter, &okPath));
+  FilePath ok_path;
-  ASSERT_FALSE(ParamTraits<FilePath>::Read(&message, &iter, &badPath));
+  FilePath bad_path;
+  ASSERT_TRUE(ParamTraits<FilePath>::Read(&message, &iter, &ok_path));
+  ASSERT_FALSE(ParamTraits<FilePath>::Read(&message, &iter, &bad_path));
 }
 
 }  // namespace