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

Update winget server com security #4577

Merged
merged 5 commits into from
Jun 24, 2024
Merged

Update winget server com security #4577

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12d58cc
Update winget server com security
yao-msft Jun 19, 2024
dd5b289
spelling
yao-msft Jun 24, 2024
2e6410f
Comments. Set recommended acl.
yao-msft Jun 24, 2024
a2819ae
8192
yao-msft Jun 24, 2024
62907a8
Update comments
yao-msft Jun 24, 2024
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
8 changes: 5 additions & 3 deletions .github/actions/spelling/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
bcp
BEFACEF
BFirst
bght

Check warning on line 43 in .github/actions/spelling/expect.txt

View workflow job for this annotation

GitHub Actions / Check Spelling 

entry has inconsistent line ending (unexpected-line-ending)
bigcatalog
BITMAPINFOHEADER
bitspace
Expand Down Expand Up @@ -74,7 +74,7 @@
COINIT
COMGLB
commandline
compressapi

Check warning on line 77 in .github/actions/spelling/expect.txt

View workflow job for this annotation

GitHub Actions / Check Spelling 

entry has inconsistent line ending (unexpected-line-ending)
contactsupport
contentfiles
contoso
Expand All @@ -90,7 +90,7 @@
cswinrt
ctc
currentuser
DACL
dacl
datetimeoffset
debian
dedupe
Expand Down Expand Up @@ -318,7 +318,8 @@
notmatch
NOTRACK
NOUPDATE
npmjs
npmjs
NRNWNX

Check warning on line 322 in .github/actions/spelling/expect.txt

View workflow job for this annotation

GitHub Actions / Check Spelling 

entry has inconsistent line ending (unexpected-line-ending)
nsis
nuffing
objbase
Expand Down Expand Up @@ -400,7 +401,8 @@
runspace
runtimeclass
ryfu
rzkzqaqjwj
rzkzqaqjwj
sacl

Check warning on line 405 in .github/actions/spelling/expect.txt

View workflow job for this annotation

GitHub Actions / Check Spelling 

entry has inconsistent line ending (unexpected-line-ending)
SARL
SASURL
schematab
Expand Down
16 changes: 11 additions & 5 deletions src/AppInstallerCLIPackage/Package.appxmanifest
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<Package
xmlns="http://schemas.microsoft.com/appx/manifest/foundation/windows10"
xmlns:uap="http://schemas.microsoft.com/appx/manifest/uap/windows10"
Expand Down Expand Up @@ -61,7 +61,7 @@
</uap5:Extension>
<com:Extension Category="windows.comServer">
<com:ComServer>
<com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;RC)(A;;11;;;AC)(A;;11;;;AN)S:P(ML;;NX;;;S-1-16-0)">
<com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;AC)S:P(ML;;NX;;;S-1-16-4096)">
<com:Class Id ="74CB3139-B7C5-4B9E-9388-E6616DEA288C" DisplayName="PackageManager Server">
Copy link

Choose a reason for hiding this comment

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

Sanity check: you're fine with allowing non-AppContainer low IL to activate this?

Copy link

Choose a reason for hiding this comment

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

This is needed to allow AppContainer to activate this, right? (due to COM's special handling of LaunchPermission without a SACL to achieve no execute up medium by default for >= medium IL servers)

Copy link

Choose a reason for hiding this comment

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

You can configure a SACL with a Medium mandatory label. This will block non-AppContainer low IL, but still allow AppContainer because you have a packaged SID in the DACL (ALL_APPLICATION_PACKAGES).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested setting to 8192 and calls from AC still works. I'll set to 8192 as suggested.

Copy link

Choose a reason for hiding this comment

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

Ah, I forgot we just need a SACL to tell activation "we're handling that detail", using medium (8192) to allow only medium and above non-AC processes makes sense. Note that this is a new restriction though, existing non-AC < medium IL clients will be broken.

Copy link
Member

Choose a reason for hiding this comment

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

Set to medium, discuss in person.

Copy link

Choose a reason for hiding this comment

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

WD = Everyone has always been wrong here. The default access permissions without a call to CoInitializeSecurity grant access to PS, SY, and BA for all server processes, plus AC for <= Medium IL server processes, so despite the extremely permissive LaunchAndActivationPermission the effective permissions of activation + calls only let this work for PS, SY, or BA (including AppContainers running as any account that matched one of these) for the usual case of Medium IL server process. My main concern is not to duplicate the overly permissive WD to the access permissions, but:

  • Choosing LaunchAndActivationPermission to be as narrow as possible would reduce the risk of accidentally granting access to callers who shouldn't have it if/when you find a reason the access permissions need to be loosened.
  • Setting this closer to the default would make it clearer what differences this server needs relative to the default.

My recommendation would be "O:SYG:SYD:(A;;11;;;IU)(A;;11;;;SY)(A;;11;;;BA)(A;;11;;;AC)S:P(ML;;NX;;;S-1-16-4096)". If Packaged COM supported the use of PS in these security descriptors that would be even better (matching the access permissions), but they don't, so the default LaunchAndActivationPermissions granting access to IU, SY, and BA are a reasonable and longstanding compromise given this known limitation. Defaults + allow AC would be the standard permissions for servers that allow AC, and the recommendation is what you need in LaunchAndActivationPermission to achieve this.

</com:Class>
<com:Class Id ="1BD8FF3A-EC50-4F69-AEEE-DF4C9D3BAA96" DisplayName="FindPackagesOptions Server">
Expand All @@ -74,15 +74,21 @@
</com:Class>
<com:Class Id ="AA2A5C04-1AD9-46C4-B74F-6B334AD7EB8C" DisplayName="UninstallOptions Server">
</com:Class>
<com:Class Id ="C9ED7917-66AB-4E31-A92A-F65F18EF7933" DisplayName="Configuration Statics Server">
</com:Class>
<com:Class Id ="8EF324ED-367C-4880-83E5-BB2ABD0B72F6" DisplayName="DownloadOptions Server">
</com:Class>
<com:Class Id ="6484A61D-50FA-41F0-B71E-F4370C6EB37C" DisplayName="AuthenticationArguments Server">
</com:Class>
</com:ExeServer>
</com:ComServer>
</com:Extension>
<com:Extension Category="windows.comServer">
<com:ComServer>
<com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Configuration Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)S:P(ML;;NX;;;S-1-16-8192)">
<com:Class Id ="C9ED7917-66AB-4E31-A92A-F65F18EF7933" DisplayName="Configuration Statics Server">
Copy link

Choose a reason for hiding this comment

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

My recommendation for servers that don't allow AC would be to either omit the LaunchAndActivationPermission attribute to get the default, or explicitly specify a security descriptor that is the same as the default. (This is due to the same concerns about WD I mentioned for the other one.) Default here (but restricted down to local activation permissions, which are the only possible effective permissions for Packaged COM in per-user packages anyway) would be "O:SYG:SYD:(A;;11;;;IU)(A;;11;;;SY)(A;;11;;;BA)". Adding the NX ACE in the SACL has limited effect because the binding of servers without a SACL is specially handled, but it does prevent < Medium IL non-AC clients from launching a server instance that they will never be able to talk to.

</com:Class>
</com:ExeServer>
</com:ComServer>
</com:Extension>
</Extensions>
</Application>
</Applications>
Expand Down Expand Up @@ -116,4 +122,4 @@
<rescap:Capability Name="packageManagement" />
<rescap:Capability Name="unvirtualizedResources" />
</Capabilities>
</Package>
</Package>
77 changes: 69 additions & 8 deletions src/WinGetServer/WinMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <memory>
#include <string>
#include <string_view>
#include <vector>

// Holds the wwinmain open until COM tells us there are no more server connections
wil::unique_event _comServerExitEvent;
Expand All @@ -35,10 +36,10 @@ HRESULT WindowsPackageManagerServerInitializeRPCServer()
RETURN_HR_IF(HRESULT_FROM_WIN32(status), status != RPC_S_OK);

// The goal of this security descriptor is to restrict RPC server access only to the user in admin mode.
// (ML;;NW;;;HI) specifies a high mandatory integrity level (requires admin).
// (ML;;NRNWNX;;;HI) specifies a high mandatory integrity level (requires admin).
// (A;;GA;;;UserSID) specifies access only for the user with the user SID (i.e. self).
wil::unique_hlocal_security_descriptor securityDescriptor;
std::string securityDescriptorString = "S:(ML;;NW;;;HI)D:(A;;GA;;;" + userSID + ")";
std::string securityDescriptorString = "S:(ML;;NRNWNX;;;HI)D:(A;;GA;;;" + userSID + ")";
Copy link

Choose a reason for hiding this comment

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

Can't you use PS instead of calculating an explicit userSID?

Copy link

Choose a reason for hiding this comment

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

That depends on whether/how RPC (and ALPC if this is passed through for the port object) call an AccessCheck* API that supports mapping a SID to PS. If they don't use one of these APIs and/or they fail to provide a mapping for PS, then PS isn't meaningful in the SD.

Copy link

Choose a reason for hiding this comment

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

Nit: I would check which of read/write/execute rights RPC maps to the object-specific rights flags it checks for in access checks of clients. Any of these you have added here but which are not meaningful in these access checks are just introducing confusion about how this is locked down as an RPC server.

Alternatively, can the whole thing just be "D:(A;;GA;;;BA)"? I.e. are there any clients that need to be allowed that are High IL, and also the same user, but don't have the BA group effective? That would have to be due to a very unusual use of token restriction on an admin account's full token, e.g. explicitly marking BA DenyOnly without reducing the IL (normally restriction reduces IL and gets more privileged groups automatically marked DenyOnly as a side effect).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert the added access rights. Regarding why we use specific sid, the original intention was to ensure only whoever manually activated the server can access it.

RETURN_LAST_ERROR_IF(!ConvertStringSecurityDescriptorToSecurityDescriptorA(securityDescriptorString.c_str(), SDDL_REVISION_1, &securityDescriptor, nullptr));

status = RpcServerRegisterIf3(WinGetServerManualActivation_v1_0_s_ifspec, nullptr, nullptr, RPC_IF_ALLOW_LOCAL_ONLY | RPC_IF_AUTOLISTEN, RPC_C_LISTEN_MAX_CALLS_DEFAULT, 0, nullptr, securityDescriptor.get());
Expand Down Expand Up @@ -100,6 +101,49 @@ extern "C" HRESULT CreateInstance(
return S_OK;
}

HRESULT InitializeComSecurity()
{
wil::unique_hlocal_security_descriptor securityDescriptor;
// Allow Everyone and AppContainer access. 3 is COM_RIGHTS_EXECUTE | COM_RIGHTS_EXECUTE_LOCAL
Copy link
Member

Choose a reason for hiding this comment

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

Comment is no longer correct.

Copy link

Choose a reason for hiding this comment

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

Comment is out of date

std::string securityDescriptorString = "O:SYG:SYD:(A;;3;;;WD)(A;;3;;;AC)";
Copy link

Choose a reason for hiding this comment

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

This is opening up permissions far more than is necessary to fix the bug as I understand it (High IL server processes that ran that way in a UAC-disabled account like the built-in Administrator account if enabled, rather than due to elevation, are not granting AC the way they would due to the defaults if they were running at Medium IL as usual). COM defaults for Medium IL processes grant PS, SY, BA, and AC. If you want the same permissions for both the usual Medium IL case and the less common High IL case, grant access to only those SIDs.

RETURN_LAST_ERROR_IF(!ConvertStringSecurityDescriptorToSecurityDescriptorA(securityDescriptorString.c_str(), SDDL_REVISION_1, &securityDescriptor, nullptr));

// Make absolute security descriptor as CoInitializeSecurity required
SECURITY_DESCRIPTOR absoluteSecurityDescriptor;
DWORD securityDescriptorSize = sizeof(SECURITY_DESCRIPTOR);

DWORD daclSize = 0;
DWORD saclSize = 0;
DWORD ownerSize = 0;
DWORD groupSize = 0;

// Get required size
BOOL result = MakeAbsoluteSD(securityDescriptor.get(), &absoluteSecurityDescriptor, &securityDescriptorSize, nullptr, &daclSize, nullptr, &saclSize, nullptr, &ownerSize, nullptr, &groupSize);
RETURN_HR_IF_MSG(E_FAIL, result || GetLastError() != ERROR_INSUFFICIENT_BUFFER, "MakeAbsoluteSD failed to return buffer sizes");

std::vector<BYTE> dacl(daclSize);
std::vector<BYTE> sacl(saclSize);
std::vector<BYTE> owner(ownerSize);
std::vector<BYTE> group(groupSize);

RETURN_LAST_ERROR_IF(!MakeAbsoluteSD(securityDescriptor.get(), &absoluteSecurityDescriptor, &securityDescriptorSize, (PACL)dacl.data(), &daclSize, (PACL)sacl.data(), &saclSize, (PACL)owner.data(), &ownerSize, (PACL)group.data(), &groupSize));

// Initialize com security
RETURN_IF_FAILED(CoInitializeSecurity(
&absoluteSecurityDescriptor, // Security descriptor
-1, // Authentication services count. -1 is let com choose.
nullptr, // Authentication services array
nullptr, // Reserved
RPC_C_AUTHN_LEVEL_PKT_INTEGRITY, // Authentication level. Validate client and data integrity.
RPC_C_IMP_LEVEL_IMPERSONATE, // Impersonation level. Can impersonate client.
Copy link

Choose a reason for hiding this comment

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

To be clear: this says you're fine with whoever you call through COM impersonating you. Still subject to kernel-level restrictions on who's allowed to have an impersonate-level token, but it's unusually permissive to put this on your CoInitializeSecurity.

nullptr, // Authentication list
EOAC_NONE, // Additional capabilities
nullptr // Reserved
));

return S_OK;
}

int __stdcall wWinMain(_In_ HINSTANCE, _In_opt_ HINSTANCE, _In_ LPWSTR cmdLine, _In_ int)
{
wil::SetResultLoggingCallback(&WindowsPackageManagerServerWilResultLoggingCallback);
Expand All @@ -115,8 +159,6 @@ int __stdcall wWinMain(_In_ HINSTANCE, _In_opt_ HINSTANCE, _In_ LPWSTR cmdLine,
RETURN_IF_FAILED(globalOptions->Set(COMGLB_EXCEPTION_HANDLING, COMGLB_EXCEPTION_DONOT_HANDLE_ANY));
}

RETURN_IF_FAILED(WindowsPackageManagerServerInitialize());

// Command line parsing
int argc = 0;
LPWSTR* argv = CommandLineToArgvW(cmdLine, &argc);
Expand All @@ -130,18 +172,29 @@ int __stdcall wWinMain(_In_ HINSTANCE, _In_opt_ HINSTANCE, _In_ LPWSTR cmdLine,
manualActivation = true;
}

// For packaged com activation, initialize com security.
// For manual activation, leave as default. We'll not register objects for manual activation.
if (!manualActivation)
{
// This must be called after IGlobalOptions (fast rundown setting cannot be changed after CoInitializeSecurity)
// This must be called before WindowsPackageManagerServerInitialize (when setting the logs
// to Windows.Storage folders, automatic CoInitializeSecurity is triggered)
RETURN_IF_FAILED(InitializeComSecurity());
}

RETURN_IF_FAILED(WindowsPackageManagerServerInitialize());

_comServerExitEvent.create();
RETURN_IF_FAILED(WindowsPackageManagerServerModuleCreate(&_releaseNotifier));
try
{
// Register all the CoCreatableClassWrlCreatorMapInclude classes
RETURN_IF_FAILED(WindowsPackageManagerServerModuleRegister());

// Manual reset event to notify the client that the server is available.
wil::unique_event manualResetEvent;

if (manualActivation)
{
// For manual activation, do not register com objects
// so that only RPC channel can be used.
HANDLE hMutex = NULL;
hMutex = CreateMutex(NULL, FALSE, TEXT("WinGetServerMutex"));
RETURN_LAST_ERROR_IF_NULL(hMutex);
Expand All @@ -157,6 +210,11 @@ int __stdcall wWinMain(_In_ HINSTANCE, _In_opt_ HINSTANCE, _In_ LPWSTR cmdLine,
manualResetEvent = CreateOrOpenServerStartEvent();
manualResetEvent.SetEvent();
}
else
{
// Register all the CoCreatableClassWrlCreatorMapInclude classes
RETURN_IF_FAILED(WindowsPackageManagerServerModuleRegister());
}

_comServerExitEvent.wait();

Expand All @@ -165,7 +223,10 @@ int __stdcall wWinMain(_In_ HINSTANCE, _In_opt_ HINSTANCE, _In_ LPWSTR cmdLine,
manualResetEvent.reset();
}

RETURN_IF_FAILED(WindowsPackageManagerServerModuleUnregister());
if (!manualActivation)
{
RETURN_IF_FAILED(WindowsPackageManagerServerModuleUnregister());
}
}
CATCH_RETURN()

Expand Down
Loading