Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SYSTEM to explicit ACLs #2370

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Add SYSTEM to explicit ACLs #2370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions src/AppInstallerCLITests/Runtime.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(hasCurrentUser && hasSystem)

If this check is only for scenarios when running as system, should we make the condition in parity with later usage

(hasCurrentUser && hasSystem && EqualSid(userToken->User.Sid, systemSID.get())

Or just throw when it's actually used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is to discover a configuration that would be illegal in that case without actually needing to be running as SYSTEM.

(!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