From c555273a04843523cd248657caf92b22168483c5 Mon Sep 17 00:00:00 2001
From: yao-msft <50888816+yao-msft@users.noreply.github.com>
Date: Mon, 24 Jun 2024 15:39:13 -0700
Subject: [PATCH] Update winget server com security (#4577)
Change:
- Explicitly set COM access permissions for packaged com invocations.
Leave access permissions as default and do not register COM objects for
manual invocation so that only RPC channel can be used for manual
activation.
- Update LaunchAndActivationString to allow Self, System, Built-in Admin and AppContainer
only, require at least MediumIL for non-AC.
- Move Configuration to a separate COM server, use default permission.
A separate pr will be sent to update AppInstaller manifest.
Validation:
Validated manually with Microsoft Store invocation, Powershell
invocation (elevated and non elevated), test sample code and Devhome
invocation (on package management and configuration).
Also specifically validated Store invocation with Built-in Administrator
sign-in (previously not working).
---
.github/actions/spelling/expect.txt | 5 +-
.../Package.appxmanifest | 16 ++--
src/WinGetServer/WinMain.cpp | 73 +++++++++++++++++--
3 files changed, 81 insertions(+), 13 deletions(-)
diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt
index f4092ee85a..87fa38e2b4 100644
--- a/.github/actions/spelling/expect.txt
+++ b/.github/actions/spelling/expect.txt
@@ -91,7 +91,7 @@ createmanifestmetadata
cswinrt
ctc
currentuser
-DACL
+dacl
datetimeoffset
debian
dedupe
@@ -401,7 +401,8 @@ roy
runspace
runtimeclass
ryfu
-rzkzqaqjwj
+rzkzqaqjwj
+sacl
SARL
SASURL
schematab
diff --git a/src/AppInstallerCLIPackage/Package.appxmanifest b/src/AppInstallerCLIPackage/Package.appxmanifest
index 6d93cc4abe..18d9ca333e 100644
--- a/src/AppInstallerCLIPackage/Package.appxmanifest
+++ b/src/AppInstallerCLIPackage/Package.appxmanifest
@@ -1,4 +1,4 @@
-
+
-
+
@@ -74,8 +74,6 @@
-
-
@@ -83,6 +81,14 @@
+
+
+
+
+
+
+
+
@@ -116,4 +122,4 @@
-
\ No newline at end of file
+
diff --git a/src/WinGetServer/WinMain.cpp b/src/WinGetServer/WinMain.cpp
index 11fe1a3749..43378f1fc6 100644
--- a/src/WinGetServer/WinMain.cpp
+++ b/src/WinGetServer/WinMain.cpp
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
// Holds the wwinmain open until COM tells us there are no more server connections
wil::unique_event _comServerExitEvent;
@@ -100,6 +101,49 @@ extern "C" HRESULT CreateInstance(
return S_OK;
}
+HRESULT InitializeComSecurity()
+{
+ wil::unique_hlocal_security_descriptor securityDescriptor;
+ // Allow Self, System, Built-in Admin and App Container access. 3 is COM_RIGHTS_EXECUTE | COM_RIGHTS_EXECUTE_LOCAL
+ std::string securityDescriptorString = "O:SYG:SYD:(A;;3;;;PS)(A;;3;;;SY)(A;;3;;;BA)(A;;3;;;AC)";
+ 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 dacl(daclSize);
+ std::vector sacl(saclSize);
+ std::vector owner(ownerSize);
+ std::vector 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_DEFAULT, // Authentication level.
+ RPC_C_IMP_LEVEL_IDENTIFY, // Impersonation level. Identify client.
+ 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);
@@ -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);
@@ -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);
@@ -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();
@@ -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()