Bug 1980886 p3 - Ensure no old inherited ACEs remain when moving files on Windows. r=yjuglaret

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

Author: bobowen

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

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));
       }
     }
   }

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;

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"));