Bug 1769094 - nsLocalFile::Append{,RelativePath} should throw when appending .. r=xpcom-reviewers,nika

Differential Revision: https://phabricator.services.mozilla.com/D146228

dom/system/tests/test_pathutils.html

@@ -346,12 +346,8 @@
             tmpDir,
             "foo",
             ".",
-            "..",
-            "foo",
-            ".",
             "bar",
-            "..",
-            "bar"
+            ".",
           )
         ),
         PathUtils.join(tmpDir, "foo", "bar"),

xpcom/io/nsIFile.idl

@@ -69,8 +69,9 @@ interface nsIFile : nsISupports
      *
      *   @param node
      *       A string which is intended to be a child node of the nsIFile.
-     *       For the |appendNative| method, the node must be in the native
-     *       filesystem charset.
+     *       For security reasons, this cannot contain .. and cannot start with
+     *       a directory separator. For the |appendNative| method, the node must
+     *       be in the native filesystem charset.
      */
     void append(in AString node);
     [noscript] void appendNative(in ACString node);
@@ -463,7 +464,7 @@ interface nsIFile : nsISupports
      *
      *   @param relativeFilePath
      *       relativeFilePath is a native relative path. For security reasons,
-     *       this cannot contain .. or cannot start with a directory separator.
+     *       this cannot contain .. and cannot start with a directory separator.
      *       For the |appendRelativeNativePath| method, the relativeFilePath
      *       must be in the native filesystem charset.
      */

xpcom/io/nsLocalFileUnix.cpp

@@ -556,9 +556,10 @@ nsLocalFile::AppendNative(const nsACString& aFragment) {
     return NS_OK;
   }
 
-  // only one component of path can be appended
+  // only one component of path can be appended and cannot append ".."
   nsACString::const_iterator begin, end;
-  if (FindCharInReadable('/', aFragment.BeginReading(begin),
+  if (aFragment.EqualsASCII("..") ||
+      FindCharInReadable('/', aFragment.BeginReading(begin),
                          aFragment.EndReading(end))) {
     return NS_ERROR_FILE_UNRECOGNIZED_PATH;
   }
@@ -572,11 +573,35 @@ nsLocalFile::AppendRelativeNativePath(const nsACString& aFragment) {
     return NS_OK;
   }
 
-  // No leading '/'
-  if (aFragment.First() == '/') {
+  // No leading '/' and cannot be ".."
+  if (aFragment.First() == '/' || aFragment.EqualsASCII("..")) {
     return NS_ERROR_FILE_UNRECOGNIZED_PATH;
   }
 
+  if (aFragment.Contains('/')) {
+    // can't contain .. as a path component. Ensure that the valid components
+    // "foo..foo", "..foo", and "foo.." are not falsely detected,
+    // but the invalid paths "../", "foo/..", "foo/../foo",
+    // "../foo", etc are.
+    constexpr auto doubleDot = "/.."_ns;
+    nsACString::const_iterator start, end, offset;
+    aFragment.BeginReading(start);
+    aFragment.EndReading(end);
+    offset = end;
+    while (FindInReadable(doubleDot, start, offset)) {
+      if (offset == end || *offset == '/') {
+        return NS_ERROR_FILE_UNRECOGNIZED_PATH;
+      }
+      start = offset;
+      offset = end;
+    }
+
+    // catches the remaining cases of prefixes
+    if (StringBeginsWith(aFragment, "../"_ns)) {
+      return NS_ERROR_FILE_UNRECOGNIZED_PATH;
+    }
+  }
+
   if (!mPath.EqualsLiteral("/")) {
     mPath.Append('/');
   }

xpcom/tests/gtest/TestFilePreferencesUnix.cpp

@@ -151,9 +151,7 @@ TEST(TestFilePreferencesUnix, Simple)
   rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(newPath));
   ASSERT_EQ(rv, NS_OK);
   rv = newPath->AppendRelativeNativePath("allowed/../forbidden_dir/file"_ns);
-  ASSERT_EQ(rv, NS_OK);
-  rv = newPath->Exists(&exists);
-  ASSERT_EQ(rv, NS_ERROR_FILE_ACCESS_DENIED);
+  ASSERT_EQ(rv, NS_ERROR_FILE_UNRECOGNIZED_PATH);
 
   rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(newPath));
   ASSERT_EQ(rv, NS_OK);

xpcom/tests/unit/test_localfile.js

@@ -183,3 +183,27 @@ add_task(
     file.remove(true);
   }
 );
+
+add_task(function test_file_append_parent() {
+  const SEPARATOR = AppConstants.platform === "win" ? "\\" : "/";
+
+  const file = do_get_profile();
+
+  Assert.throws(
+    () => file.append(".."),
+    /NS_ERROR_FILE_UNRECOGNIZED_PATH/,
+    `nsLocalFile::Append("..") throws`
+  );
+
+  Assert.throws(
+    () => file.appendRelativePath(".."),
+    /NS_ERROR_FILE_UNRECOGNIZED_PATH/,
+    `nsLocalFile::AppendRelativePath("..") throws`
+  );
+
+  Assert.throws(
+    () => file.appendRelativePath(`foo${SEPARATOR}..${SEPARATOR}baz`),
+    /NS_ERROR_FILE_UNRECOGNIZED_PATH/,
+    `nsLocalFile::AppendRelativePath(path) fails when path contains ".."`
+  );
+});