commit 1180ae60a6d654e37672f73720e97861b4f5db75
parent 0a66b935c728292b03d0e606483ac351cfeddb10
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Tue, 26 Jul 2022 16:41:05 -0700
Rewrite ApplyACL to allow for SYSTEM to participate. Grant SYSTEM full access to Temp as that is where we install from. (#2370)
Add support for `SYSTEM` to the explicit ACLs, and give `SYSTEM` full control on the `Temp` directory.
`ApplyACL` and the data that it uses are basically re-written to support a more dynamic list of ACEs. Also ensures that if the process is running as `SYSTEM`, it doesn't attempt to write the same ACE multiple times.
Diffstat:
3 files changed, 108 insertions(+), 57 deletions(-)
diff --git 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
@@ -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<ACEPrincipal> Owner;
+ std::map<ACEPrincipal, ACEPermissions> 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
@@ -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<TOKEN_USER>()) userToken;
+ auto userToken = wil::get_token_information<TOKEN_USER>();
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<TOKEN_USER>();
+ { 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<EXPLICIT_ACCESS_W, ARRAYSIZE(aceDetails)> 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<LPWCH>(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<ACEPrincipal> 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<LPWCH>(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<LPWCH>(ace.SID);
+ }
}
wil::unique_any<PACL, decltype(&::LocalFree), ::LocalFree> 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
{
