Skip to content

Commit

Permalink
Rewrite ApplyACL to allow for SYSTEM to participate. Grant SYSTEM ful…
Browse files Browse the repository at this point in the history 
…l 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.
  • Loading branch information
@JohnMcPMS
JohnMcPMS authored Jul 26, 2022
1 parent 0a66b93 commit 1180ae6
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 57 deletions.
23 changes: 18 additions & 5 deletions src/AppInstallerCLITests/Runtime.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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();

Expand All @@ -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())
{
Expand All @@ -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));
}
21 changes: 16 additions & 5 deletions src/AppInstallerCommonCore/Public/AppInstallerRuntime.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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);
Expand All @@ -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;
Expand Down
121 changes: 74 additions & 47 deletions src/AppInstallerCommonCore/Runtime.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ namespace AppInstaller::Runtime
{
DWORD result = 0;

if (permissions == ACEPermissions::Owner)
if (permissions == ACEPermissions::All)
{
result |= GENERIC_ALL;
}
Expand 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()
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
Expand All @@ -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:
Expand All @@ -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
{
Expand Down

0 comments on commit 1180ae6

Please sign in to comment.