From 25fb658fd8af8ccbf56b1c0c3d972960884414d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Wed, 22 Apr 2026 16:50:59 -0400 Subject: [PATCH 1/3] Resolve ACL performance PR conflicts against master --- src/AppInstallerCLITests/Runtime.cpp | 298 ++++++++++++++++++ src/AppInstallerCommonCore/Runtime.cpp | 1 + src/AppInstallerSharedLib/Filesystem.cpp | 269 ++++++++++++++-- .../Public/winget/Filesystem.h | 2 +- 4 files changed, 549 insertions(+), 21 deletions(-) diff --git a/src/AppInstallerCLITests/Runtime.cpp b/src/AppInstallerCLITests/Runtime.cpp index e1003a22d1..b7d7bd8558 100644 --- a/src/AppInstallerCLITests/Runtime.cpp +++ b/src/AppInstallerCLITests/Runtime.cpp @@ -28,6 +28,204 @@ void RequireAdminOwner(const std::filesystem::path& directory) REQUIRE(EqualSid(adminSID.get(), ownerSID)); } +DWORD AccessMaskFrom(ACEPermissions permissions) +{ + DWORD result = 0; + + if (permissions == ACEPermissions::All) + { + result |= GENERIC_ALL; + } + else + { + if (WI_IsFlagSet(permissions, ACEPermissions::Read)) + { + result |= GENERIC_READ; + } + + if (WI_IsFlagSet(permissions, ACEPermissions::Write)) + { + result |= GENERIC_WRITE | FILE_DELETE_CHILD; + } + + if (WI_IsFlagSet(permissions, ACEPermissions::Execute)) + { + result |= GENERIC_EXECUTE; + } + } + + return result; +} + +DWORD NormalizedAccessMaskFrom(ACEPermissions permissions) +{ + DWORD result = AccessMaskFrom(permissions); + GENERIC_MAPPING genericMapping + { + FILE_GENERIC_READ, + FILE_GENERIC_WRITE, + FILE_GENERIC_EXECUTE, + FILE_ALL_ACCESS, + }; + + MapGenericMask(&result, &genericMapping); + return result; +} + +void ApplyAclForTest(const std::filesystem::path& directory, const std::optional& owner, const std::map& acl, bool protectDacl = true) +{ + auto userToken = wil::get_token_information(); + auto adminSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); + auto systemSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_LOCAL_SYSTEM_RID); + PSID ownerSID = nullptr; + + struct ACEDetails + { + ACEPrincipal Principal; + PSID SID; + TRUSTEE_TYPE TrusteeType; + }; + + ACEDetails aceDetails[] = + { + { ACEPrincipal::CurrentUser, userToken->User.Sid, TRUSTEE_IS_USER }, + { ACEPrincipal::Admins, adminSID.get(), TRUSTEE_IS_WELL_KNOWN_GROUP }, + { ACEPrincipal::System, systemSID.get(), TRUSTEE_IS_USER }, + }; + + ULONG entriesCount = 0; + std::array explicitAccess; + + for (const auto& ace : aceDetails) + { + if (owner && owner.value() == ace.Principal) + { + ownerSID = ace.SID; + } + + auto itr = acl.find(ace.Principal); + if (itr != acl.end()) + { + EXPLICIT_ACCESS_W& entry = explicitAccess[entriesCount++]; + entry = {}; + entry.grfAccessPermissions = AccessMaskFrom(itr->second); + entry.grfAccessMode = SET_ACCESS; + entry.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + entry.Trustee.TrusteeForm = TRUSTEE_IS_SID; + entry.Trustee.TrusteeType = ace.TrusteeType; + entry.Trustee.ptstrName = reinterpret_cast(ace.SID); + } + } + + wil::unique_any appliedAcl; + THROW_IF_WIN32_ERROR(SetEntriesInAclW(entriesCount, explicitAccess.data(), nullptr, &appliedAcl)); + + SECURITY_INFORMATION securityInformation = DACL_SECURITY_INFORMATION; + if (protectDacl) + { + securityInformation |= PROTECTED_DACL_SECURITY_INFORMATION; + } + + if (ownerSID) + { + securityInformation |= OWNER_SECURITY_INFORMATION; + } + + std::wstring path = directory.wstring(); + THROW_IF_WIN32_ERROR(SetNamedSecurityInfoW(&path[0], SE_FILE_OBJECT, securityInformation, ownerSID, nullptr, appliedAcl.get(), nullptr)); +} + +void AddUnexpectedUsersAce(const std::filesystem::path& directory) +{ + wil::unique_hlocal_security_descriptor securityDescriptor; + PACL existingDacl = nullptr; + THROW_IF_WIN32_ERROR(GetNamedSecurityInfoW(directory.c_str(), SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &existingDacl, nullptr, &securityDescriptor)); + + auto usersSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_USERS); + EXPLICIT_ACCESS_W entry = {}; + entry.grfAccessPermissions = GENERIC_READ; + entry.grfAccessMode = GRANT_ACCESS; + entry.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + entry.Trustee.TrusteeForm = TRUSTEE_IS_SID; + entry.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; + entry.Trustee.ptstrName = reinterpret_cast(usersSID.get()); + + wil::unique_any updatedDacl; + THROW_IF_WIN32_ERROR(SetEntriesInAclW(1, &entry, existingDacl, &updatedDacl)); + + std::wstring path = directory.wstring(); + THROW_IF_WIN32_ERROR(SetNamedSecurityInfoW(&path[0], SE_FILE_OBJECT, DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, nullptr, nullptr, updatedDacl.get(), nullptr)); +} + +void ApplyCombinedAceAclForTest(const std::filesystem::path& directory, const std::optional& owner, const std::map& acl) +{ + auto userToken = wil::get_token_information(); + auto adminSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); + auto systemSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_LOCAL_SYSTEM_RID); + PSID ownerSID = nullptr; + + struct ACEDetails + { + ACEPrincipal Principal; + PSID SID; + }; + + ACEDetails aceDetails[] = + { + { ACEPrincipal::CurrentUser, userToken->User.Sid }, + { ACEPrincipal::Admins, adminSID.get() }, + { ACEPrincipal::System, systemSID.get() }, + }; + + DWORD aclSize = sizeof(ACL); + for (const auto& ace : aceDetails) + { + if (owner && owner.value() == ace.Principal) + { + ownerSID = ace.SID; + } + + if (acl.count(ace.Principal) != 0) + { + aclSize += sizeof(ACCESS_ALLOWED_ACE) - sizeof(DWORD) + GetLengthSid(ace.SID); + } + } + + std::vector aclBuffer(aclSize); + PACL appliedAcl = reinterpret_cast(aclBuffer.data()); + THROW_IF_WIN32_BOOL_FALSE(InitializeAcl(appliedAcl, aclSize, ACL_REVISION)); + + for (const auto& ace : aceDetails) + { + auto itr = acl.find(ace.Principal); + if (itr != acl.end()) + { + THROW_IF_WIN32_BOOL_FALSE(AddAccessAllowedAceEx( + appliedAcl, + ACL_REVISION, + CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, + NormalizedAccessMaskFrom(itr->second), + ace.SID)); + } + } + + SECURITY_INFORMATION securityInformation = DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION; + if (ownerSID) + { + securityInformation |= OWNER_SECURITY_INFORMATION; + } + + std::wstring path = directory.wstring(); + THROW_IF_WIN32_ERROR(SetNamedSecurityInfoW( + &path[0], + SE_FILE_OBJECT, + securityInformation, + ownerSID, + nullptr, + appliedAcl, + nullptr)); +} + TEST_CASE("ApplyACL_CurrentUserOwner", "[runtime]") { TempDirectory directory("CurrentUserOwner"); @@ -104,6 +302,106 @@ TEST_CASE("ApplyACL_CurrentUserOwner_SystemAll", "[runtime]") REQUIRE(CanWriteToPath(directory)); } +TEST_CASE("ShouldApplyACL_FalseWhenSecurityAlreadyMatches", "[runtime]") +{ + TempDirectory directory("ShouldApplyACLExactMatch"); + PathDetails details; + details.Path = directory; + details.SetOwner(ACEPrincipal::CurrentUser); + details.ACL[ACEPrincipal::System] = ACEPermissions::All; + details.ACL[ACEPrincipal::Admins] = ACEPermissions::All; + + details.ApplyACL(); + + REQUIRE_FALSE(details.ShouldApplyACL()); +} + +TEST_CASE("ShouldApplyACL_FalseWhenSecurityIsSemanticallyEquivalent", "[runtime]") +{ + TempDirectory directory("ShouldApplyACLSemanticMatch"); + PathDetails details; + details.Path = directory; + details.SetOwner(ACEPrincipal::CurrentUser); + details.ACL[ACEPrincipal::System] = ACEPermissions::All; + details.ACL[ACEPrincipal::Admins] = ACEPermissions::All; + + ApplyCombinedAceAclForTest(directory, details.Owner, details.ACL); + + REQUIRE_FALSE(details.ShouldApplyACL()); +} + +TEST_CASE("ShouldApplyACL_TrueWhenOwnerMismatched", "[runtime]") +{ + TempDirectory directory("ShouldApplyACLMismatchedOwner"); + PathDetails actualDetails; + actualDetails.Path = directory; + actualDetails.SetOwner(ACEPrincipal::CurrentUser); + actualDetails.ACL[ACEPrincipal::System] = ACEPermissions::All; + actualDetails.ACL[ACEPrincipal::Admins] = ACEPermissions::All; + actualDetails.ApplyACL(); + + PathDetails expectedDetails = actualDetails; + expectedDetails.Owner = ACEPrincipal::Admins; + + REQUIRE(expectedDetails.ShouldApplyACL()); +} + +TEST_CASE("ShouldApplyACL_TrueWhenPermissionsMismatched", "[runtime]") +{ + TempDirectory directory("ShouldApplyACLMismatchedPermissions"); + PathDetails actualDetails; + actualDetails.Path = directory; + actualDetails.SetOwner(ACEPrincipal::CurrentUser); + actualDetails.ACL[ACEPrincipal::System] = ACEPermissions::All; + actualDetails.ACL[ACEPrincipal::Admins] = ACEPermissions::All; + actualDetails.ApplyACL(); + + PathDetails expectedDetails = actualDetails; + expectedDetails.ACL[ACEPrincipal::CurrentUser] = ACEPermissions::ReadExecute; + + REQUIRE(expectedDetails.ShouldApplyACL()); +} + +TEST_CASE("ShouldApplyACL_TrueWhenDaclIsNotProtected", "[runtime]") +{ + TempDirectory directory("ShouldApplyACLUnprotectedDacl"); + PathDetails details; + details.Path = directory; + details.SetOwner(ACEPrincipal::CurrentUser); + details.ACL[ACEPrincipal::System] = ACEPermissions::All; + details.ACL[ACEPrincipal::Admins] = ACEPermissions::All; + + ApplyAclForTest(directory, details.Owner, details.ACL, false); + + REQUIRE(details.ShouldApplyACL()); +} + +TEST_CASE("ShouldApplyACL_TrueWhenUnexpectedAceExists", "[runtime]") +{ + TempDirectory directory("ShouldApplyACLUnexpectedAce"); + PathDetails details; + details.Path = directory; + details.SetOwner(ACEPrincipal::CurrentUser); + details.ACL[ACEPrincipal::System] = ACEPermissions::All; + details.ACL[ACEPrincipal::Admins] = ACEPermissions::All; + details.ApplyACL(); + AddUnexpectedUsersAce(directory); + + REQUIRE(details.ShouldApplyACL()); +} + +TEST_CASE("SetRuntimePathStateName_AffectsTempPath", "[runtime]") +{ + constexpr std::string_view stateName = "Issue6162State"; + auto restoreStateName = wil::scope_exit([]() { Runtime::SetRuntimePathStateName("defaultState"); }); + + Runtime::SetRuntimePathStateName(std::string{ stateName }); + + PathDetails details = Runtime::GetPathDetailsFor(Runtime::PathName::Temp, true); + + REQUIRE(details.Path.filename() == Utility::ConvertToUTF16(stateName)); +} + TEST_CASE("VerifyDevModeEnabledCheck", "[runtime]") { if (!Runtime::IsRunningAsAdmin()) diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index 1618776842..a4820790d2 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -280,6 +280,7 @@ namespace AppInstaller::Runtime { case PathName::Temp: result.Path = GetPathToUserTemp(forDisplay) / s_DefaultTempDirectory; + result.Path /= GetRuntimePathStateName(); result.SetOwner(ACEPrincipal::CurrentUser); result.ACL[ACEPrincipal::System] = ACEPermissions::All; result.ACL[ACEPrincipal::Admins] = ACEPermissions::All; diff --git a/src/AppInstallerSharedLib/Filesystem.cpp b/src/AppInstallerSharedLib/Filesystem.cpp index 8d20374c80..a1bf1c9e95 100644 --- a/src/AppInstallerSharedLib/Filesystem.cpp +++ b/src/AppInstallerSharedLib/Filesystem.cpp @@ -27,6 +27,22 @@ namespace AppInstaller::Filesystem TRUSTEE_TYPE TrusteeType; }; + constexpr BYTE s_InheritableAceFlags = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + + struct PrincipalPermissions + { + DWORD DirectAccessMask = 0; + DWORD ObjectChildAccessMask = 0; + DWORD ContainerChildAccessMask = 0; + + bool operator==(const PrincipalPermissions& other) const + { + return DirectAccessMask == other.DirectAccessMask && + ObjectChildAccessMask == other.ObjectChildAccessMask && + ContainerChildAccessMask == other.ContainerChildAccessMask; + } + }; + DWORD AccessPermissionsFrom(ACEPermissions permissions) { DWORD result = 0; @@ -56,6 +72,223 @@ namespace AppInstaller::Filesystem return result; } + std::wstring GetSidString(PSID sid) + { + wil::unique_hlocal_string sidString; + THROW_IF_WIN32_BOOL_FALSE(ConvertSidToStringSidW(sid, &sidString)); + return sidString.get(); + } + + DWORD NormalizeAccessMask(DWORD accessMask) + { + GENERIC_MAPPING genericMapping + { + FILE_GENERIC_READ, + FILE_GENERIC_WRITE, + FILE_GENERIC_EXECUTE, + FILE_ALL_ACCESS, + }; + + MapGenericMask(&accessMask, &genericMapping); + return accessMask; + } + + std::map GetExpectedPermissions( + const PathDetails& details, + const ACEDetails(&aceDetails)[3], + const std::optional& principalToIgnore, + PSID& expectedOwnerSID) + { + std::map result; + + for (const auto& ace : aceDetails) + { + if (principalToIgnore && principalToIgnore.value() == ace.Principal) + { + continue; + } + + if (details.Owner && details.Owner.value() == ace.Principal) + { + expectedOwnerSID = ace.SID; + } + + auto itr = details.ACL.find(ace.Principal); + if (itr != details.ACL.end()) + { + DWORD normalizedAccessMask = NormalizeAccessMask(AccessPermissionsFrom(itr->second)); + result.emplace(ace.Principal, PrincipalPermissions + { + normalizedAccessMask, + normalizedAccessMask, + normalizedAccessMask, + }); + } + } + + return result; + } + + std::optional> GetActualPermissions( + PACL acl, + const std::map& sidToPrincipal) + { + constexpr BYTE s_AllowedAceFlags = OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE | INHERIT_ONLY_ACE; + + ACL_SIZE_INFORMATION sizeInformation{}; + THROW_IF_WIN32_BOOL_FALSE(GetAclInformation(acl, &sizeInformation, sizeof(sizeInformation), AclSizeInformation)); + + std::map result; + + for (DWORD i = 0; i < sizeInformation.AceCount; ++i) + { + void* ace = nullptr; + THROW_IF_WIN32_BOOL_FALSE(GetAce(acl, i, &ace)); + + const ACE_HEADER* aceHeader = static_cast(ace); + if (aceHeader->AceType != ACCESS_ALLOWED_ACE_TYPE || (aceHeader->AceFlags & ~s_AllowedAceFlags) != 0) + { + return std::nullopt; + } + + const ACCESS_ALLOWED_ACE* accessAllowedAce = static_cast(ace); + std::wstring sid = GetSidString(reinterpret_cast(const_cast(&accessAllowedAce->SidStart))); + auto principalItr = sidToPrincipal.find(sid); + if (principalItr == sidToPrincipal.end()) + { + return std::nullopt; + } + + if (WI_IsFlagSet(aceHeader->AceFlags, INHERIT_ONLY_ACE) && + (aceHeader->AceFlags & (OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE)) == 0) + { + return std::nullopt; + } + + PrincipalPermissions& principalPermissions = result[principalItr->second]; + DWORD normalizedAccessMask = NormalizeAccessMask(accessAllowedAce->Mask); + + if (!WI_IsFlagSet(aceHeader->AceFlags, INHERIT_ONLY_ACE)) + { + principalPermissions.DirectAccessMask |= normalizedAccessMask; + } + + if (WI_IsFlagSet(aceHeader->AceFlags, OBJECT_INHERIT_ACE)) + { + principalPermissions.ObjectChildAccessMask |= normalizedAccessMask; + } + + if (WI_IsFlagSet(aceHeader->AceFlags, CONTAINER_INHERIT_ACE)) + { + principalPermissions.ContainerChildAccessMask |= normalizedAccessMask; + } + } + + return result; + } + + std::optional GetPrincipalToIgnore(const PathDetails& details, const TOKEN_USER* userToken, PSID systemSID) + { + bool hasCurrentUser = details.ACL.count(ACEPrincipal::CurrentUser) != 0; + bool hasSystem = details.ACL.count(ACEPrincipal::System) != 0; + + if ((hasCurrentUser && hasSystem) && + IsRunningAsSystem() && + (!details.Owner || (details.Owner.value() != ACEPrincipal::CurrentUser && details.Owner.value() != ACEPrincipal::System))) + { + THROW_HR(HRESULT_FROM_WIN32(ERROR_INVALID_STATE)); + } + + if (hasCurrentUser && hasSystem && EqualSid(userToken->User.Sid, systemSID)) + { + return (details.Owner.value() == ACEPrincipal::CurrentUser ? ACEPrincipal::System : ACEPrincipal::CurrentUser); + } + + return std::nullopt; + } + + bool PathHasExpectedOwnerAndAcls(const PathDetails& details) + { + auto userToken = wil::get_token_information(); + auto adminSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); + auto systemSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_LOCAL_SYSTEM_RID); + auto principalToIgnore = GetPrincipalToIgnore(details, userToken.get(), systemSID.get()); + + ACEDetails aceDetails[] = + { + { ACEPrincipal::CurrentUser, userToken->User.Sid, TRUSTEE_IS_USER }, + { ACEPrincipal::Admins, adminSID.get(), TRUSTEE_IS_WELL_KNOWN_GROUP }, + { ACEPrincipal::System, systemSID.get(), TRUSTEE_IS_USER }, + }; + + PSID expectedOwnerSID = nullptr; + std::map expectedPermissions = GetExpectedPermissions(details, aceDetails, principalToIgnore, expectedOwnerSID); + + SECURITY_INFORMATION securityInformation = DACL_SECURITY_INFORMATION; + PSID ownerSID = nullptr; + + if (details.Owner) + { + securityInformation |= OWNER_SECURITY_INFORMATION; + ownerSID = expectedOwnerSID; + } + + std::wstring path = details.Path.wstring(); + wil::unique_hlocal_security_descriptor securityDescriptor; + PSID actualOwnerSID = nullptr; + DWORD result = GetNamedSecurityInfoW( + &path[0], + SE_FILE_OBJECT, + securityInformation, + details.Owner ? &actualOwnerSID : nullptr, + nullptr, + nullptr, + nullptr, + &securityDescriptor); + + if (result != ERROR_SUCCESS) + { + return false; + } + + SECURITY_DESCRIPTOR_CONTROL control = 0; + DWORD revision = 0; + if (!GetSecurityDescriptorControl(securityDescriptor.get(), &control, &revision) || !WI_IsFlagSet(control, SE_DACL_PROTECTED)) + { + return false; + } + + BOOL daclPresent = FALSE; + BOOL daclDefaulted = FALSE; + PACL currentDacl = nullptr; + if (!GetSecurityDescriptorDacl(securityDescriptor.get(), &daclPresent, ¤tDacl, &daclDefaulted) || !daclPresent || !currentDacl) + { + return false; + } + + if (ownerSID && (!actualOwnerSID || !EqualSid(actualOwnerSID, ownerSID))) + { + return false; + } + + std::map sidToPrincipal; + for (const auto& ace : aceDetails) + { + if (principalToIgnore && principalToIgnore.value() == ace.Principal) + { + continue; + } + + if (details.ACL.count(ace.Principal) != 0) + { + sidToPrincipal.emplace(GetSidString(ace.SID), ace.Principal); + } + } + + auto actualPermissions = GetActualPermissions(currentDacl, sidToPrincipal); + return actualPermissions && actualPermissions.value() == expectedPermissions; + } + // Gets the path to the appdata root. // *Only used by non packaged version!* std::filesystem::path GetPathToAppDataRoot(bool anonymize) @@ -340,24 +573,24 @@ namespace AppInstaller::Filesystem bool PathDetails::ShouldApplyACL() const { - // Could be expanded to actually check the current owner/ACL on the path, but isn't worth it currently - return !ACL.empty(); - } - - void PathDetails::ApplyACL() const - { - bool hasCurrentUser = ACL.count(ACEPrincipal::CurrentUser) != 0; - bool hasSystem = ACL.count(ACEPrincipal::System) != 0; + if (ACL.empty()) + { + return false; + } - // Configuring permissions for both CurrentUser and SYSTEM while not having owner set as one of them is not valid because - // below we use only the owner permissions in the case of running as SYSTEM. - if ((hasCurrentUser && hasSystem) && - IsRunningAsSystem() && - (!Owner || (Owner.value() != ACEPrincipal::CurrentUser && Owner.value() != ACEPrincipal::System))) + try { - THROW_HR(HRESULT_FROM_WIN32(ERROR_INVALID_STATE)); + return !anon::PathHasExpectedOwnerAndAcls(*this); } + catch (...) + { + LOG_CAUGHT_EXCEPTION_MSG("Failed to inspect ACL state; reapplying ACLs to preserve security"); + return true; + } + } + void PathDetails::ApplyACL() const + { auto userToken = wil::get_token_information(); auto adminSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); auto systemSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_LOCAL_SYSTEM_RID); @@ -375,11 +608,7 @@ namespace AppInstaller::Filesystem // If the current user is SYSTEM, we want to take either the owner or the only configured set of permissions. // The check above should prevent us from getting into situations outside of the ones below. - std::optional principalToIgnore; - if (hasCurrentUser && hasSystem && EqualSid(userToken->User.Sid, systemSID.get())) - { - principalToIgnore = (Owner.value() == ACEPrincipal::CurrentUser ? ACEPrincipal::System : ACEPrincipal::CurrentUser); - } + std::optional principalToIgnore = anon::GetPrincipalToIgnore(*this, userToken.get(), systemSID.get()); for (const auto& ace : aceDetails) { @@ -401,7 +630,7 @@ namespace AppInstaller::Filesystem entry.grfAccessPermissions = anon::AccessPermissionsFrom(itr->second); entry.grfAccessMode = SET_ACCESS; - entry.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + entry.grfInheritance = anon::s_InheritableAceFlags; entry.Trustee.pMultipleTrustee = nullptr; entry.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; diff --git a/src/AppInstallerSharedLib/Public/winget/Filesystem.h b/src/AppInstallerSharedLib/Public/winget/Filesystem.h index 50f2a9b6b6..c833e8e921 100644 --- a/src/AppInstallerSharedLib/Public/winget/Filesystem.h +++ b/src/AppInstallerSharedLib/Public/winget/Filesystem.h @@ -93,7 +93,7 @@ namespace AppInstaller::Filesystem // Shorthand for setting Owner and giving them ACEPermissions::All void SetOwner(ACEPrincipal owner); - // Determines if the ACL should be applied. + // Determines if the ACL needs to be applied. bool ShouldApplyACL() const; // Applies the ACL unconditionally. From 65158e5ecde6a2a99355a6dcc358663cc153d3e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Wed, 22 Apr 2026 21:22:17 -0400 Subject: [PATCH 2/3] Address PR review feedback for ACL handling --- src/AppInstallerCommonCore/Runtime.cpp | 1 - src/AppInstallerSharedLib/Filesystem.cpp | 37 +++++++++++++----------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index a4820790d2..1618776842 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -280,7 +280,6 @@ namespace AppInstaller::Runtime { case PathName::Temp: result.Path = GetPathToUserTemp(forDisplay) / s_DefaultTempDirectory; - result.Path /= GetRuntimePathStateName(); result.SetOwner(ACEPrincipal::CurrentUser); result.ACL[ACEPrincipal::System] = ACEPermissions::All; result.ACL[ACEPrincipal::Admins] = ACEPermissions::All; diff --git a/src/AppInstallerSharedLib/Filesystem.cpp b/src/AppInstallerSharedLib/Filesystem.cpp index a1bf1c9e95..c0bf01daf2 100644 --- a/src/AppInstallerSharedLib/Filesystem.cpp +++ b/src/AppInstallerSharedLib/Filesystem.cpp @@ -43,6 +43,12 @@ namespace AppInstaller::Filesystem } }; + struct ExpectedACE + { + ACEPrincipal Principal; + PSID SID; + }; + DWORD AccessPermissionsFrom(ACEPermissions permissions) { DWORD result = 0; @@ -72,13 +78,6 @@ namespace AppInstaller::Filesystem return result; } - std::wstring GetSidString(PSID sid) - { - wil::unique_hlocal_string sidString; - THROW_IF_WIN32_BOOL_FALSE(ConvertSidToStringSidW(sid, &sidString)); - return sidString.get(); - } - DWORD NormalizeAccessMask(DWORD accessMask) { GENERIC_MAPPING genericMapping @@ -131,7 +130,7 @@ namespace AppInstaller::Filesystem std::optional> GetActualPermissions( PACL acl, - const std::map& sidToPrincipal) + const std::vector& expectedAces) { constexpr BYTE s_AllowedAceFlags = OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE | INHERIT_ONLY_ACE; @@ -152,9 +151,13 @@ namespace AppInstaller::Filesystem } const ACCESS_ALLOWED_ACE* accessAllowedAce = static_cast(ace); - std::wstring sid = GetSidString(reinterpret_cast(const_cast(&accessAllowedAce->SidStart))); - auto principalItr = sidToPrincipal.find(sid); - if (principalItr == sidToPrincipal.end()) + PSID sid = reinterpret_cast(const_cast(&accessAllowedAce->SidStart)); + auto expectedAceItr = std::find_if(expectedAces.begin(), expectedAces.end(), + [&](const auto& expectedAce) + { + return EqualSid(expectedAce.SID, sid); + }); + if (expectedAceItr == expectedAces.end()) { return std::nullopt; } @@ -165,7 +168,7 @@ namespace AppInstaller::Filesystem return std::nullopt; } - PrincipalPermissions& principalPermissions = result[principalItr->second]; + PrincipalPermissions& principalPermissions = result[expectedAceItr->Principal]; DWORD normalizedAccessMask = NormalizeAccessMask(accessAllowedAce->Mask); if (!WI_IsFlagSet(aceHeader->AceFlags, INHERIT_ONLY_ACE)) @@ -207,7 +210,7 @@ namespace AppInstaller::Filesystem return std::nullopt; } - bool PathHasExpectedOwnerAndAcls(const PathDetails& details) + bool PathHasExpectedOwnerAndACLs(const PathDetails& details) { auto userToken = wil::get_token_information(); auto adminSID = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); @@ -271,7 +274,7 @@ namespace AppInstaller::Filesystem return false; } - std::map sidToPrincipal; + std::vector expectedAces; for (const auto& ace : aceDetails) { if (principalToIgnore && principalToIgnore.value() == ace.Principal) @@ -281,11 +284,11 @@ namespace AppInstaller::Filesystem if (details.ACL.count(ace.Principal) != 0) { - sidToPrincipal.emplace(GetSidString(ace.SID), ace.Principal); + expectedAces.push_back({ ace.Principal, ace.SID }); } } - auto actualPermissions = GetActualPermissions(currentDacl, sidToPrincipal); + auto actualPermissions = GetActualPermissions(currentDacl, expectedAces); return actualPermissions && actualPermissions.value() == expectedPermissions; } @@ -580,7 +583,7 @@ namespace AppInstaller::Filesystem try { - return !anon::PathHasExpectedOwnerAndAcls(*this); + return !anon::PathHasExpectedOwnerAndACLs(*this); } catch (...) { From f5ce2899b4e3e59b01e174851229972f773dcfbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Fri, 24 Apr 2026 08:46:01 -0400 Subject: [PATCH 3/3] Remove obsolete packaged temp-path test --- src/AppInstallerCLITests/Runtime.cpp | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/AppInstallerCLITests/Runtime.cpp b/src/AppInstallerCLITests/Runtime.cpp index b7d7bd8558..e986122b32 100644 --- a/src/AppInstallerCLITests/Runtime.cpp +++ b/src/AppInstallerCLITests/Runtime.cpp @@ -390,18 +390,6 @@ TEST_CASE("ShouldApplyACL_TrueWhenUnexpectedAceExists", "[runtime]") REQUIRE(details.ShouldApplyACL()); } -TEST_CASE("SetRuntimePathStateName_AffectsTempPath", "[runtime]") -{ - constexpr std::string_view stateName = "Issue6162State"; - auto restoreStateName = wil::scope_exit([]() { Runtime::SetRuntimePathStateName("defaultState"); }); - - Runtime::SetRuntimePathStateName(std::string{ stateName }); - - PathDetails details = Runtime::GetPathDetailsFor(Runtime::PathName::Temp, true); - - REQUIRE(details.Path.filename() == Utility::ConvertToUTF16(stateName)); -} - TEST_CASE("VerifyDevModeEnabledCheck", "[runtime]") { if (!Runtime::IsRunningAsAdmin())