From 12d58cce819134bf31fd2d70d21b65ee7033c9db Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Wed, 19 Jun 2024 16:25:04 -0700 Subject: [PATCH 1/5] Update winget server com security --- .../Package.appxmanifest | 16 ++-- src/WinGetServer/WinMain.cpp | 77 +++++++++++++++++-- 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/src/AppInstallerCLIPackage/Package.appxmanifest b/src/AppInstallerCLIPackage/Package.appxmanifest index 6d93cc4abe..0ddb6e8e15 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..1bbb80ab71 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; @@ -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 + ")"; 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()); @@ -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 + std::string securityDescriptorString = "O:SYG:SYD:(A;;3;;;WD)(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_PKT_INTEGRITY, // Authentication level. Validate client and data integrity. + RPC_C_IMP_LEVEL_IMPERSONATE, // Impersonation level. Can impersonate 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 CoInitilizeSecurity 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() From dd5b2897c4696dad151dfcd34b4b50d509df53fa Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Sun, 23 Jun 2024 23:54:46 -0700 Subject: [PATCH 2/5] spelling --- .github/actions/spelling/expect.txt | 8 +++++--- src/WinGetServer/WinMain.cpp | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 16466a04fb..90177c9b8f 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -90,7 +90,7 @@ createmanifestmetadata cswinrt ctc currentuser -DACL +dacl datetimeoffset debian dedupe @@ -318,7 +318,8 @@ NOTAPROPERTY notmatch NOTRACK NOUPDATE -npmjs +npmjs +NRNWNX nsis nuffing objbase @@ -400,7 +401,8 @@ roy runspace runtimeclass ryfu -rzkzqaqjwj +rzkzqaqjwj +sacl SARL SASURL schematab diff --git a/src/WinGetServer/WinMain.cpp b/src/WinGetServer/WinMain.cpp index 1bbb80ab71..f533b6e89d 100644 --- a/src/WinGetServer/WinMain.cpp +++ b/src/WinGetServer/WinMain.cpp @@ -178,7 +178,7 @@ int __stdcall wWinMain(_In_ HINSTANCE, _In_opt_ HINSTANCE, _In_ LPWSTR cmdLine, { // 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 CoInitilizeSecurity is triggered) + // to Windows.Storage folders, automatic CoInitializeSecurity is triggered) RETURN_IF_FAILED(InitializeComSecurity()); } From 2e6410f69880e7bb7034b661d2ffe4af8b4bfd79 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Mon, 24 Jun 2024 12:47:53 -0700 Subject: [PATCH 3/5] Comments. Set recommended acl. --- .github/actions/spelling/expect.txt | 3 +-- src/AppInstallerCLIPackage/Package.appxmanifest | 4 ++-- src/WinGetServer/WinMain.cpp | 10 +++++----- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 90177c9b8f..21a4e7ad01 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -318,8 +318,7 @@ NOTAPROPERTY notmatch NOTRACK NOUPDATE -npmjs -NRNWNX +npmjs nsis nuffing objbase diff --git a/src/AppInstallerCLIPackage/Package.appxmanifest b/src/AppInstallerCLIPackage/Package.appxmanifest index 0ddb6e8e15..e31e4794ad 100644 --- a/src/AppInstallerCLIPackage/Package.appxmanifest +++ b/src/AppInstallerCLIPackage/Package.appxmanifest @@ -61,7 +61,7 @@ - + @@ -83,7 +83,7 @@ - + diff --git a/src/WinGetServer/WinMain.cpp b/src/WinGetServer/WinMain.cpp index f533b6e89d..ea33bf37d4 100644 --- a/src/WinGetServer/WinMain.cpp +++ b/src/WinGetServer/WinMain.cpp @@ -36,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;;NRNWNX;;;HI) specifies a high mandatory integrity level (requires admin). + // (ML;;NW;;;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;;NRNWNX;;;HI)D:(A;;GA;;;" + userSID + ")"; + std::string securityDescriptorString = "S:(ML;;NW;;;HI)D:(A;;GA;;;" + userSID + ")"; 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()); @@ -105,7 +105,7 @@ HRESULT InitializeComSecurity() { wil::unique_hlocal_security_descriptor securityDescriptor; // Allow Everyone and AppContainer access. 3 is COM_RIGHTS_EXECUTE | COM_RIGHTS_EXECUTE_LOCAL - std::string securityDescriptorString = "O:SYG:SYD:(A;;3;;;WD)(A;;3;;;AC)"; + 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 @@ -134,8 +134,8 @@ HRESULT InitializeComSecurity() -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. + 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 From a2819aeffd9c7a19bb93f274597a5fec7b0e8f8e Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Mon, 24 Jun 2024 13:17:53 -0700 Subject: [PATCH 4/5] 8192 --- src/AppInstallerCLIPackage/Package.appxmanifest | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AppInstallerCLIPackage/Package.appxmanifest b/src/AppInstallerCLIPackage/Package.appxmanifest index e31e4794ad..18d9ca333e 100644 --- a/src/AppInstallerCLIPackage/Package.appxmanifest +++ b/src/AppInstallerCLIPackage/Package.appxmanifest @@ -61,7 +61,7 @@ - + From 62907a8ecd5ba2c769e080b7f07ae42c5efc1606 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Mon, 24 Jun 2024 15:21:30 -0700 Subject: [PATCH 5/5] Update comments --- src/WinGetServer/WinMain.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/WinGetServer/WinMain.cpp b/src/WinGetServer/WinMain.cpp index ea33bf37d4..43378f1fc6 100644 --- a/src/WinGetServer/WinMain.cpp +++ b/src/WinGetServer/WinMain.cpp @@ -104,7 +104,7 @@ extern "C" HRESULT CreateInstance( HRESULT InitializeComSecurity() { wil::unique_hlocal_security_descriptor securityDescriptor; - // Allow Everyone and AppContainer access. 3 is COM_RIGHTS_EXECUTE | COM_RIGHTS_EXECUTE_LOCAL + // 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));