diff --git a/dom/system/tests/test_pathutils.html b/dom/system/tests/test_pathutils.html index c51281a92666..6a21f7a3417c 100644 --- a/dom/system/tests/test_pathutils.html +++ b/dom/system/tests/test_pathutils.html @@ -346,12 +346,8 @@ tmpDir, "foo", ".", - "..", - "foo", - ".", "bar", - "..", - "bar" + ".", ) ), PathUtils.join(tmpDir, "foo", "bar"), 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 6a39759db81d..a8f39c1762c4 100644 --- a/xpcom/io/nsLocalFileUnix.cpp +++ b/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('/'); } diff --git a/xpcom/tests/gtest/TestFilePreferencesUnix.cpp b/xpcom/tests/gtest/TestFilePreferencesUnix.cpp index b48f7d0a72e8..915c189b97f8 100644 --- a/xpcom/tests/gtest/TestFilePreferencesUnix.cpp +++ b/xpcom/tests/gtest/TestFilePreferencesUnix.cpp @@ -151,20 +151,14 @@ 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); rv = newPath->AppendNative("allowed"_ns); ASSERT_EQ(rv, NS_OK); rv = newPath->AppendNative(".."_ns); - ASSERT_EQ(rv, NS_OK); - rv = newPath->AppendNative("forbidden_dir"_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); nsAutoCString trickyPath(tempPath); trickyPath.AppendLiteral("/allowed/../forbidden_dir/file"); 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 ".."` + ); +});