commit ac562564b2c8f351d0eade43b2833faa33c3c683
parent 21c8e2c40719b4b84aa8160a03271f476cc238d8
Author: Bob Owen <bobowencode@gmail.com>
Date: Mon, 6 Oct 2025 08:04:25 +0000
Bug 1980886 p3 - Ensure no old inherited ACEs remain when moving files on Windows. r=yjuglaret
Differential Revision: https://phabricator.services.mozilla.com/D265007
Diffstat:
4 files changed, 209 insertions(+), 29 deletions(-)
diff --git a/xpcom/base/nsWindowsHelpers.h b/xpcom/base/nsWindowsHelpers.h
@@ -328,6 +328,19 @@ struct CloseHandleDeleter {
}
};
+using AutoFreeSecurityDescriptor =
+ mozilla::UniquePtr<SECURITY_DESCRIPTOR, LocalFreeDeleter>;
+
+struct DestroyPrivateObjectSecurityDeleter {
+ void operator()(PSECURITY_DESCRIPTOR aSecDescPtr) {
+ ::DestroyPrivateObjectSecurity(&aSecDescPtr);
+ }
+};
+
+using AutoDestroySecurityDescriptor =
+ mozilla::UniquePtr<SECURITY_DESCRIPTOR,
+ DestroyPrivateObjectSecurityDeleter>;
+
// One caller of this function is early in startup and several others are not,
// so they have different ways of determining the two parameters. This function
// exists just so any future code that needs to determine whether the dynamic
diff --git a/xpcom/io/nsLocalFileWin.cpp b/xpcom/io/nsLocalFileWin.cpp
@@ -5,6 +5,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
#include "mozilla/ArrayUtils.h"
+#include "mozilla/Assertions.h"
#include "mozilla/DebugOnly.h"
#include "mozilla/ProfilerLabels.h"
#include "mozilla/TextUtils.h"
@@ -96,7 +97,6 @@ nsresult NewLocalFile(const nsAString& aPath, bool aUseDOSDevicePathSyntax,
file.forget(aResult);
return NS_OK;
}
-
} // anonymous namespace
static HWND GetMostRecentNavigatorHWND() {
@@ -203,6 +203,73 @@ bool nsLocalFile::CheckForReservedFileName(const nsString& aFileName) {
return false;
}
+/* static */
+bool nsLocalFile::ChildAclMatchesAclInheritedFromParent(
+ const NotNull<ACL*> aChildDacl, bool aIsChildDir,
+ const AutoFreeSecurityDescriptor& aChildSecDesc, nsIFile* aParentDir) {
+ // If we fail at any point return false.
+ ACL* parentDacl = nullptr;
+ AutoFreeSecurityDescriptor parentSecDesc;
+ nsAutoString parentPath;
+ MOZ_ALWAYS_SUCCEEDS(aParentDir->GetTarget(parentPath));
+ DWORD errCode = ::GetNamedSecurityInfoW(
+ parentPath.getW(), SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr,
+ nullptr, &parentDacl, nullptr, getter_Transfers(parentSecDesc));
+ if (errCode != ERROR_SUCCESS || !parentDacl) {
+ NS_ERROR(nsPrintfCString(
+ "Failed to get parent dir DACL for comparison: %lx", errCode)
+ .get());
+ return false;
+ }
+
+ // Create a new security descriptor with a DACL that just inherits from the
+ // parent. We can then compare the current childs DACL to make sure the counts
+ // of inherited ACEs match. We pass the child security descriptor as the
+ // creator to get the owner and group information.
+ AutoDestroySecurityDescriptor newSecDesc;
+ GENERIC_MAPPING mapping;
+ mapping.GenericRead = FILE_GENERIC_READ;
+ mapping.GenericWrite = FILE_GENERIC_WRITE;
+ mapping.GenericExecute = FILE_GENERIC_EXECUTE;
+ mapping.GenericAll = FILE_ALL_ACCESS;
+ if (!::CreatePrivateObjectSecurityEx(
+ parentSecDesc.get(), aChildSecDesc.get(),
+ getter_Transfers(newSecDesc), nullptr, aIsChildDir,
+ SEF_DACL_AUTO_INHERIT | SEF_AVOID_OWNER_CHECK |
+ SEF_AVOID_PRIVILEGE_CHECK,
+ nullptr, &mapping)) {
+ // There may be legitimate reasons for this to fail, so we might have to
+ // remove this if it causes problems.
+ NS_ERROR(nsPrintfCString(
+ "Failed to create new inherited DACL for comparison: %lx",
+ ::GetLastError())
+ .get());
+ return false;
+ }
+
+ BOOL daclPresent;
+ ACL* newDacl = nullptr;
+ BOOL daclDefaulted;
+ if (!::GetSecurityDescriptorDacl(newSecDesc.get(), &daclPresent, &newDacl,
+ &daclDefaulted) ||
+ !daclPresent || !newDacl) {
+ NS_ERROR(
+ nsPrintfCString("Failed to get new DACL from security descriptor: %lx",
+ ::GetLastError())
+ .get());
+ return false;
+ }
+
+ auto getInheritedAceCount = [](const ACL* aAcl) {
+ AclAceRange aclAceRange(WrapNotNull(aAcl));
+ return std::count_if(
+ aclAceRange.begin(), aclAceRange.end(),
+ [](const auto& hdr) { return hdr.AceFlags & INHERITED_ACE; });
+ };
+
+ return getInheritedAceCount(aChildDacl) == getInheritedAceCount(newDacl);
+}
+
class nsDriveEnumerator : public nsSimpleEnumerator,
public nsIDirectoryEnumerator {
public:
@@ -1800,6 +1867,10 @@ nsresult nsLocalFile::MoveOrCopyAsSingleFileOrDir(nsIFile* aDestParent,
return NS_ERROR_FILE_ACCESS_DENIED;
}
+ // Determine if we are a directory before any move/copy.
+ bool isDir = false;
+ MOZ_ALWAYS_SUCCEEDS(IsDirectory(&isDir));
+
int copyOK = 0;
if (move) {
copyOK = ::MoveFileExW(filePath.get(), destPath.get(),
@@ -1850,31 +1921,30 @@ nsresult nsLocalFile::MoveOrCopyAsSingleFileOrDir(nsIFile* aDestParent,
} else if (move && !(aOptions & SkipNtfsAclReset)) {
// Set security permissions to inherit from parent.
// Note: propagates to all children: slow for big file trees
- PACL pOldDACL = nullptr;
- PSECURITY_DESCRIPTOR pSD = nullptr;
- ::GetNamedSecurityInfoW((LPWSTR)destPath.get(), SE_FILE_OBJECT,
- DACL_SECURITY_INFORMATION, nullptr, nullptr,
- &pOldDACL, nullptr, &pSD);
- UniquePtr<VOID, LocalFreeDeleter> autoFreeSecDesc(pSD);
- if (pOldDACL) {
- // Test the current DACL, if we find one that is inherited then we can
- // skip the reset. This avoids a request for SeTcbPrivilege, which can
- // cause a lot of audit events if enabled (Bug 1816694).
- bool inherited = false;
- for (DWORD i = 0; i < pOldDACL->AceCount; ++i) {
- VOID* pAce = nullptr;
- if (::GetAce(pOldDACL, i, &pAce) &&
- static_cast<PACE_HEADER>(pAce)->AceFlags & INHERITED_ACE) {
- inherited = true;
- break;
- }
- }
-
- if (!inherited) {
- ::SetNamedSecurityInfoW(
- (LPWSTR)destPath.get(), SE_FILE_OBJECT,
- DACL_SECURITY_INFORMATION | UNPROTECTED_DACL_SECURITY_INFORMATION,
- nullptr, nullptr, pOldDACL, nullptr);
+ ACL* childDacl = nullptr;
+ AutoFreeSecurityDescriptor childSecDesc;
+ // We need owner and group information for the parent ACL check.
+ DWORD errCode = ::GetNamedSecurityInfoW(
+ destPath.getW(), SE_FILE_OBJECT,
+ DACL_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION |
+ GROUP_SECURITY_INFORMATION,
+ nullptr, nullptr, &childDacl, nullptr, getter_Transfers(childSecDesc));
+ if (errCode == ERROR_SUCCESS && childDacl) {
+ // Compare the number of inherited ACEs on the child to the number we
+ // expect from the parent. If they don't match then we reset. This is
+ // because we can get old inherited ACEs from the previous dir if moved
+ // within the same volume. We check this to prevent unnecessary calls to
+ // SetNamedSecurityInfoW, this avoids a request for SeTcbPrivilege, which
+ // can cause a lot of audit events if enabled (Bug 1816694).
+ if (!ChildAclMatchesAclInheritedFromParent(WrapNotNull(childDacl), isDir,
+ childSecDesc, aDestParent)) {
+ // We don't expect this to fail, but it shouldn't crash in release.
+ MOZ_ALWAYS_TRUE(
+ ERROR_SUCCESS ==
+ ::SetNamedSecurityInfoW(destPath.get(), SE_FILE_OBJECT,
+ DACL_SECURITY_INFORMATION |
+ UNPROTECTED_DACL_SECURITY_INFORMATION,
+ nullptr, nullptr, childDacl, nullptr));
}
}
}
diff --git a/xpcom/io/nsLocalFileWin.h b/xpcom/io/nsLocalFileWin.h
@@ -14,6 +14,7 @@
#include "nsIFile.h"
#include "nsILocalFileWin.h"
#include "nsIClassInfoImpl.h"
+#include "nsWindowsHelpers.h"
#include "prio.h"
#include "mozilla/Attributes.h"
@@ -52,6 +53,21 @@ class nsLocalFile final : public nsILocalFileWin {
// (com1, com2, etc...) and returns true if so.
static bool CheckForReservedFileName(const nsString& aFileName);
+ /**
+ * Checks whether the inherited ACEs in aChildDacl only come from the parent.
+ *
+ * @param aChildDacl the DACL for the child file object
+ * @param aIsChildDir true if child is a directory
+ * @param aChildSecDesc the SECURITY_DESCRIPTOR for the child, this will
+ * correspond to aChildDacl
+ * @param aParentDir the parent nsIFile
+ * @return true if inherited ACEs are only from aParentDir
+ * false if child has other inherited ACEs or a failure occurs
+ */
+ static bool ChildAclMatchesAclInheritedFromParent(
+ const mozilla::NotNull<ACL*> aChildDacl, bool aIsChildDir,
+ const AutoFreeSecurityDescriptor& aChildSecDesc, nsIFile* aParentDir);
+
// PRFileInfo64 does not hvae an accessTime field;
struct FileInfo {
PRFileType type;
diff --git a/xpcom/tests/gtest/TestFile.cpp b/xpcom/tests/gtest/TestFile.cpp
@@ -7,19 +7,25 @@
#include "prsystem.h"
#include "nsIFile.h"
-#ifdef XP_WIN
-# include "nsILocalFileWin.h"
-#endif
#include "nsComponentManagerUtils.h"
#include "nsString.h"
#include "nsDirectoryServiceDefs.h"
#include "nsDirectoryServiceUtils.h"
#include "nsPrintfCString.h"
+#ifdef XP_WIN
+# include <aclapi.h>
+# include "mozilla/RandomNum.h"
+# include "nsILocalFileWin.h"
+# include "nsLocalFile.h"
+# include "nsWindowsHelpers.h"
+#endif
+
#include "gtest/gtest.h"
#include "mozilla/gtest/MozAssertions.h"
#ifdef XP_WIN
+using namespace mozilla;
bool gTestWithPrefix_Win = false;
#endif
@@ -40,6 +46,56 @@ static void SetUseDOSDevicePathSyntax(nsIFile* aFile) {
winFile->SetUseDOSDevicePathSyntax(true);
}
}
+
+static auto GetSecurityInfoStructured(nsIFile* aFile) {
+ nsAutoString pathStr;
+ MOZ_RELEASE_ASSERT(NS_SUCCEEDED(aFile->GetTarget(pathStr)));
+
+ PACL pDacl = nullptr;
+ AutoFreeSecurityDescriptor secDesc;
+ DWORD errCode = ::GetNamedSecurityInfoW(
+ pathStr.getW(), SE_FILE_OBJECT,
+ DACL_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION |
+ GROUP_SECURITY_INFORMATION,
+ nullptr, nullptr, &pDacl, nullptr, getter_Transfers(secDesc));
+ MOZ_RELEASE_ASSERT(errCode == ERROR_SUCCESS && pDacl);
+
+ return std::make_tuple(std::move(pathStr), WrapNotNull(pDacl),
+ std::move(secDesc));
+}
+
+static void AddAcesForRandomSidToDir(nsIFile* aDir) {
+ auto [dirPath, pDirDacl, secDesc] = GetSecurityInfoStructured(aDir);
+
+ constexpr BYTE kSubAuthorityCount = 4;
+ BYTE randomSidBuffer[SECURITY_SID_SIZE(4)];
+ ASSERT_TRUE(
+ GenerateRandomBytesFromOS(randomSidBuffer, sizeof(randomSidBuffer)));
+ auto* randomSid = reinterpret_cast<SID*>(randomSidBuffer);
+ randomSid->Revision = SID_REVISION;
+ randomSid->SubAuthorityCount = kSubAuthorityCount;
+ randomSid->IdentifierAuthority = SECURITY_NULL_SID_AUTHORITY;
+ ASSERT_TRUE(::IsValidSid(randomSid));
+
+ EXPLICIT_ACCESS_W newAccess[2];
+ newAccess[0].grfAccessMode = GRANT_ACCESS;
+ newAccess[0].grfAccessPermissions = GENERIC_READ;
+ newAccess[0].grfInheritance = SUB_OBJECTS_ONLY_INHERIT;
+ ::BuildTrusteeWithSidW(&newAccess[0].Trustee, randomSid);
+ newAccess[1].grfAccessMode = DENY_ACCESS;
+ newAccess[1].grfAccessPermissions = GENERIC_WRITE;
+ newAccess[1].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
+ ::BuildTrusteeWithSidW(&newAccess[1].Trustee, randomSid);
+ UniquePtr<ACL, LocalFreeDeleter> newDacl;
+ ASSERT_EQ(::SetEntriesInAclW(std::size(newAccess), newAccess, pDirDacl,
+ getter_Transfers(newDacl)),
+ (ULONG)ERROR_SUCCESS);
+
+ ASSERT_EQ(::SetNamedSecurityInfoW(dirPath.get(), SE_FILE_OBJECT,
+ DACL_SECURITY_INFORMATION, nullptr, nullptr,
+ newDacl.get(), nullptr),
+ (ULONG)ERROR_SUCCESS);
+}
#endif
static already_AddRefed<nsIFile> NewFile(nsIFile* aBase) {
@@ -284,6 +340,17 @@ static bool TestMove(nsIFile* aBase, nsIFile* aDestDir, const char* aName,
return false;
}
+#ifdef XP_WIN
+ // Ensure ACE inheritance is correct.
+ auto [childPathStr, childDacl, childSecDesc] =
+ GetSecurityInfoStructured(file);
+ bool isDir = false;
+ EXPECT_NS_SUCCEEDED(file->IsDirectory(&isDir));
+ EXPECT_TRUE(nsLocalFile::ChildAclMatchesAclInheritedFromParent(
+ childDacl, isDir, childSecDesc, aDestDir))
+ << newName.get() << " ACL, does not match destination dir";
+#endif
+
return true;
}
@@ -532,6 +599,20 @@ static void SetupAndTestFunctions(const nsAString& aDirName,
// Test moving across directories and renaming at the same time
ASSERT_TRUE(TestMove(subdir, base, "file2.txt", "file4.txt"));
+#ifdef XP_WIN
+ // On Windows if we move a file or directory to a directory on the same volume
+ // where the inherited ACLs differ between the source and target dirs, then we
+ // retain the ACEs from the original dir. Add inherited ACEs for random SIDs
+ // to subdir to ensure we reset the inheritance to just the target directory.
+ AddAcesForRandomSidToDir(subdir);
+ ASSERT_TRUE(TestCreate(subdir, "file8.txt", nsIFile::NORMAL_FILE_TYPE, 0600));
+ ASSERT_TRUE(TestMove(subdir, base, "file8.txt", "file8.txt"));
+
+ // Test that dir moves also get the ACL reset when required.
+ ASSERT_TRUE(TestCreate(subdir, "subdir2", nsIFile::DIRECTORY_TYPE, 0700));
+ ASSERT_TRUE(TestMove(subdir, base, "subdir2", "subdir2"));
+#endif
+
// Test copying across directories
ASSERT_TRUE(TestCopy(base, subdir, "file4.txt", "file5.txt"));
