diff --git a/src/AppInstallerCLITests/Runtime.cpp b/src/AppInstallerCLITests/Runtime.cpp index 986f786688..7a6352b57f 100644 --- a/src/AppInstallerCLITests/Runtime.cpp +++ b/src/AppInstallerCLITests/Runtime.cpp @@ -31,7 +31,7 @@ TEST_CASE("ApplyACL_CurrentUserOwner", "[runtime]") TempDirectory directory("CurrentUserOwner"); PathDetails details; details.Path = directory; - details.CurrentUser = ACEPermissions::Owner; + details.SetOwner(ACEPrincipal::CurrentUser); details.ApplyACL(); @@ -43,7 +43,7 @@ TEST_CASE("ApplyACL_RemoveWriteForUser", "[runtime]") TempDirectory directory("CurrentUserCantWrite"); PathDetails details; details.Path = directory; - details.CurrentUser = ACEPermissions::ReadExecute; + details.ACL[ACEPrincipal::CurrentUser] = ACEPermissions::ReadExecute; details.ApplyACL(); @@ -55,7 +55,7 @@ TEST_CASE("ApplyACL_AdminOwner", "[runtime]") TempDirectory directory("AdminOwner"); PathDetails details; details.Path = directory; - details.Admins = ACEPermissions::Owner; + details.SetOwner(ACEPrincipal::Admins); if (IsRunningAsAdmin()) { @@ -75,9 +75,22 @@ TEST_CASE("ApplyACL_BothOwners", "[runtime]") TempDirectory directory("AdminOwner"); PathDetails details; details.Path = directory; - details.CurrentUser = ACEPermissions::Owner; - details.Admins = ACEPermissions::Owner; + details.ACL[ACEPrincipal::CurrentUser] = ACEPermissions::ReadExecute; + details.ACL[ACEPrincipal::System] = ACEPermissions::All; // Both cannot be owners REQUIRE_THROWS_HR(details.ApplyACL(), HRESULT_FROM_WIN32(ERROR_INVALID_STATE)); } + +TEST_CASE("ApplyACL_CurrentUserOwner_SystemAll", "[runtime]") +{ + TempDirectory directory("UserAndSystem"); + PathDetails details; + details.Path = directory; + details.SetOwner(ACEPrincipal::CurrentUser); + details.ACL[ACEPrincipal::System] = ACEPermissions::All; + + details.ApplyACL(); + + REQUIRE(CanWriteToPath(directory)); +} diff --git a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h index 5ea0e22ed5..6902157115 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h @@ -64,6 +64,14 @@ namespace AppInstaller::Runtime PortableLinksMachineLocation, }; + // The principal that an ACE applies to. + enum class ACEPrincipal : uint32_t + { + CurrentUser, + Admins, + System, + }; + // The permissions granted to a specific ACE. enum class ACEPermissions : uint32_t { @@ -75,8 +83,8 @@ namespace AppInstaller::Runtime ReadWrite = Read | Write, ReadExecute = Read | Execute, ReadWriteExecute = Read | Write | Execute, - // Owner means that full control will be granted - Owner = 0xFFFFFFFF + // All means that full control will be granted + All = 0xFFFFFFFF }; DEFINE_ENUM_FLAG_OPERATORS(ACEPermissions); @@ -85,10 +93,13 @@ namespace AppInstaller::Runtime struct PathDetails { std::filesystem::path Path; - // Default to creating the directory with inherited permissions + // Default to creating the directory with inherited ownership and permissions bool Create = true; - ACEPermissions CurrentUser = ACEPermissions::None; - ACEPermissions Admins = ACEPermissions::None; + std::optional Owner; + std::map ACL; + + // Shorthand for setting Owner and giving them ACEPermissions::All + void SetOwner(ACEPrincipal owner); // Determines if the ACL should be applied. bool ShouldApplyACL() const; diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index afed6f9b22..4d781cecc3 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -198,7 +198,7 @@ namespace AppInstaller::Runtime { DWORD result = 0; - if (permissions == ACEPermissions::Owner) + if (permissions == ACEPermissions::All) { result |= GENERIC_ALL; } @@ -222,6 +222,14 @@ namespace AppInstaller::Runtime return result; } + + // Contains the information about an ACE entry for a given principal. + struct ACEDetails + { + ACEPrincipal Principal; + PSID SID; + TRUSTEE_TYPE TrusteeType; + }; } bool IsRunningInPackagedContext() @@ -329,69 +337,86 @@ namespace AppInstaller::Runtime s_runtimePathStateName.emplace(std::move(suitablePathPart)); } + void PathDetails::SetOwner(ACEPrincipal owner) + { + Owner = owner; + ACL[owner] = ACEPermissions::All; + } + bool PathDetails::ShouldApplyACL() const { // Could be expanded to actually check the current owner/ACL on the path, but isn't worth it currently - return (CurrentUser != ACEPermissions::None || Admins != ACEPermissions::None); + return !ACL.empty(); } void PathDetails::ApplyACL() const { - THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), CurrentUser == ACEPermissions::Owner && Admins == ACEPermissions::Owner); + bool hasCurrentUser = ACL.count(ACEPrincipal::CurrentUser) != 0; + bool hasSystem = ACL.count(ACEPrincipal::System) != 0; - ULONG entriesCount = 0; - EXPLICIT_ACCESS_W explicitAccess[2]; + // 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) && + (!Owner || (Owner.value() != ACEPrincipal::CurrentUser && Owner.value() != ACEPrincipal::System))) + { + THROW_HR(HRESULT_FROM_WIN32(ERROR_INVALID_STATE)); + } - decltype(wil::get_token_information()) userToken; + 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; - if (CurrentUser != ACEPermissions::None) + ACEDetails aceDetails[] = { - userToken = wil::get_token_information(); + { ACEPrincipal::CurrentUser, userToken->User.Sid, TRUSTEE_IS_USER }, + { ACEPrincipal::Admins, adminSID.get(), TRUSTEE_IS_WELL_KNOWN_GROUP}, + { ACEPrincipal::System, systemSID.get(), TRUSTEE_IS_USER}, + }; - if (CurrentUser == ACEPermissions::Owner) - { - ownerSID = userToken->User.Sid; - } - - EXPLICIT_ACCESS_W& entry = explicitAccess[entriesCount++]; - entry = {}; - - entry.grfAccessPermissions = AccessPermissionsFrom(CurrentUser); - entry.grfAccessMode = SET_ACCESS; - entry.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + ULONG entriesCount = 0; + std::array explicitAccess; - entry.Trustee.pMultipleTrustee = nullptr; - entry.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; - entry.Trustee.TrusteeForm = TRUSTEE_IS_SID; - entry.Trustee.TrusteeType = TRUSTEE_IS_USER; - entry.Trustee.ptstrName = reinterpret_cast(userToken->User.Sid); + // 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); } - if (Admins != ACEPermissions::None) + for (const auto& ace : aceDetails) { - if (Admins == ACEPermissions::Owner) + if (principalToIgnore && principalToIgnore.value() == ace.Principal) { - ownerSID = adminSID.get(); + continue; } - EXPLICIT_ACCESS_W& entry = explicitAccess[entriesCount++]; - entry = {}; - - entry.grfAccessPermissions = AccessPermissionsFrom(Admins); - entry.grfAccessMode = SET_ACCESS; - entry.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + if (Owner && Owner.value() == ace.Principal) + { + ownerSID = ace.SID; + } - entry.Trustee.pMultipleTrustee = nullptr; - entry.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; - entry.Trustee.TrusteeForm = TRUSTEE_IS_SID; - entry.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - entry.Trustee.ptstrName = reinterpret_cast(adminSID.get()); + auto itr = ACL.find(ace.Principal); + if (itr != ACL.end()) + { + EXPLICIT_ACCESS_W& entry = explicitAccess[entriesCount++]; + entry = {}; + + entry.grfAccessPermissions = AccessPermissionsFrom(itr->second); + entry.grfAccessMode = SET_ACCESS; + entry.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + + entry.Trustee.pMultipleTrustee = nullptr; + entry.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; + entry.Trustee.TrusteeForm = TRUSTEE_IS_SID; + entry.Trustee.TrusteeType = ace.TrusteeType; + entry.Trustee.ptstrName = reinterpret_cast(ace.SID); + } } wil::unique_any acl; - THROW_IF_WIN32_ERROR(SetEntriesInAclW(entriesCount, explicitAccess, nullptr, &acl)); + THROW_IF_WIN32_ERROR(SetEntriesInAclW(entriesCount, explicitAccess.data(), nullptr, &acl)); std::wstring path = Path.wstring(); SECURITY_INFORMATION securityInformation = DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION; @@ -472,7 +497,8 @@ namespace AppInstaller::Runtime { case PathName::Temp: result.Path = GetPathToUserTemp() / s_DefaultTempDirectory; - result.CurrentUser = ACEPermissions::Owner; + result.SetOwner(ACEPrincipal::CurrentUser); + result.ACL[ACEPrincipal::System] = ACEPermissions::All; break; case PathName::LocalState: case PathName::UserFileSettings: @@ -502,8 +528,8 @@ namespace AppInstaller::Runtime result.Path /= GetPackageName(); if (path == PathName::SecureSettingsForWrite) { - result.Admins = ACEPermissions::Owner; - result.CurrentUser = ACEPermissions::ReadExecute; + result.SetOwner(ACEPrincipal::Admins); + result.ACL[ACEPrincipal::CurrentUser] = ACEPermissions::ReadExecute; } else { @@ -540,7 +566,8 @@ namespace AppInstaller::Runtime result.Path /= GetRuntimePathStateName(); if (path == PathName::Temp) { - result.CurrentUser = ACEPermissions::Owner; + result.SetOwner(ACEPrincipal::CurrentUser); + result.ACL[ACEPrincipal::System] = ACEPermissions::All; } } break; @@ -553,13 +580,13 @@ namespace AppInstaller::Runtime case PathName::LocalState: result.Path = GetPathToAppDataDir(s_AppDataDir_State); result.Path /= GetRuntimePathStateName(); - result.CurrentUser = ACEPermissions::Owner; + result.SetOwner(ACEPrincipal::CurrentUser); break; case PathName::StandardSettings: case PathName::UserFileSettings: result.Path = GetPathToAppDataDir(s_AppDataDir_Settings); result.Path /= GetRuntimePathStateName(); - result.CurrentUser = ACEPermissions::Owner; + result.SetOwner(ACEPrincipal::CurrentUser); break; case PathName::SecureSettingsForRead: case PathName::SecureSettingsForWrite: @@ -571,8 +598,8 @@ namespace AppInstaller::Runtime result.Path /= GetRuntimePathStateName(); if (path == PathName::SecureSettingsForWrite) { - result.Admins = ACEPermissions::Owner; - result.CurrentUser = ACEPermissions::ReadExecute; + result.SetOwner(ACEPrincipal::Admins); + result.ACL[ACEPrincipal::CurrentUser] = ACEPermissions::ReadExecute; } else {