From 9dea84af2a991ea2d20db574e1002a090d84a14e Mon Sep 17 00:00:00 2001
From: Barret Rennie <barret@brennie.ca>
Date: Fri, 13 May 2022 15:08:47 +0000
Subject: [PATCH] Bug 1769094 - nsLocalFile::Append{,RelativePath} should throw
 when appending .. r=xpcom-reviewers,nika

Differential Revision: https://phabricator.services.mozilla.com/D146228
---
 xpcom/io/nsIFile.idl               |  7 ++++---
 xpcom/io/nsLocalFileUnix.cpp       | 33 ++++++++++++++++++++++++++----
 xpcom/tests/unit/test_localfile.js | 24 ++++++++++++++++++++++
 3 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/xpcom/io/nsIFile.idl b/xpcom/io/nsIFile.idl
index e938965236b4..86b98cb2a20c 100644
--- a/xpcom/io/nsIFile.idl
+++ b/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.
      */
diff --git a/xpcom/io/nsLocalFileUnix.cpp b/xpcom/io/nsLocalFileUnix.cpp
index f3f98008ed74..92b19eebe731 100644
--- a/xpcom/io/nsLocalFileUnix.cpp
+++ b/xpcom/io/nsLocalFileUnix.cpp
@@ -560,9 +560,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;
   }
@@ -576,11 +577,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('/');
   }
diff --git a/xpcom/tests/unit/test_localfile.js b/xpcom/tests/unit/test_localfile.js
index 125d32de6334..7ea2c6c9143c 100644
--- a/xpcom/tests/unit/test_localfile.js
+++ b/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 ".."`
+  );
+});