From 8e96a74fbc8719fb2158154a01e740273b11564d Mon Sep 17 00:00:00 2001 From: Howard Kapustein Date: Fri, 17 Jun 2022 06:42:44 -0700 Subject: [PATCH] Installation with System-Account doesn't work #2546 (#2565) * Installation with System-Account doesn't work #2546 Co-authored-by: Santosh Chintalapati --- dev/Common/Security.User.h | 25 ++++++ dev/Deployment/DeploymentActivityContext.cpp | 2 +- dev/Deployment/DeploymentActivityContext.h | 2 +- dev/Deployment/DeploymentManager.cpp | 4 +- .../MddBootstrapActivity.cpp | 2 +- .../MddBootstrapActivity.h | 2 +- .../MddBootstrapTracelogging.cpp | 4 +- installer/dev/InstallActivityContext.cpp | 17 ++++- installer/dev/InstallActivityContext.h | 6 +- installer/dev/console.cpp | 28 +++++-- installer/dev/install.cpp | 76 +++++++++++++++---- installer/dev/main.cpp | 4 +- installer/dev/pch.h | 1 + installer/dev/tracelogging.cpp | 2 +- test/Common/Common.vcxproj | 1 + test/Common/Common.vcxproj.filters | 3 + test/Common/Test_Security_User.cpp | 35 +++++++++ test/Common/pch.h | 1 + 18 files changed, 178 insertions(+), 37 deletions(-) create mode 100644 dev/Common/Security.User.h create mode 100644 test/Common/Test_Security_User.cpp diff --git a/dev/Common/Security.User.h b/dev/Common/Security.User.h new file mode 100644 index 000000000..fc2b5752a --- /dev/null +++ b/dev/Common/Security.User.h @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +#ifndef __SECURITY_USER_H +#define __SECURITY_USER_H + +namespace Security::User +{ +inline bool IsLocalSystem(HANDLE token = nullptr) +{ + BYTE localSystemSidBuffer[ SECURITY_MAX_SID_SIZE ]; + PSID localSystemSid{ reinterpret_cast(localSystemSidBuffer) }; + DWORD localSystemSidBufferSize{ ARRAYSIZE(localSystemSidBuffer) }; + THROW_IF_WIN32_BOOL_FALSE(CreateWellKnownSid(WinLocalSystemSid, nullptr, localSystemSid, &localSystemSidBufferSize)); + + wistd::unique_ptr user{ + wil::get_token_information( + !token ? GetCurrentThreadEffectiveToken() : token) }; + PSID userSid{ user->User.Sid }; + + return !!EqualSid(userSid, localSystemSid); +} +} + +#endif // __SECURITY_USER_H diff --git a/dev/Deployment/DeploymentActivityContext.cpp b/dev/Deployment/DeploymentActivityContext.cpp index 3e539bb90..d31f5db64 100644 --- a/dev/Deployment/DeploymentActivityContext.cpp +++ b/dev/Deployment/DeploymentActivityContext.cpp @@ -43,7 +43,7 @@ void WindowsAppRuntime::Deployment::Activity::Context::SetLastFailure(const wil: m_lastFailure.file.clear(); } - m_lastFailure.lineNumer = failure.uLineNumber; + m_lastFailure.lineNumber = failure.uLineNumber; if (failure.pszMessage) { diff --git a/dev/Deployment/DeploymentActivityContext.h b/dev/Deployment/DeploymentActivityContext.h index ad8430084..fdecb90d5 100644 --- a/dev/Deployment/DeploymentActivityContext.h +++ b/dev/Deployment/DeploymentActivityContext.h @@ -22,7 +22,7 @@ namespace WindowsAppRuntime::Deployment::Activity wil::FailureType type; HRESULT hr; std::string file; - unsigned int lineNumer; + unsigned int lineNumber; std::wstring message; std::string module; }; diff --git a/dev/Deployment/DeploymentManager.cpp b/dev/Deployment/DeploymentManager.cpp index bd46f276e..cc9cf5f86 100644 --- a/dev/Deployment/DeploymentManager.cpp +++ b/dev/Deployment/DeploymentManager.cpp @@ -174,7 +174,7 @@ namespace winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::implem deployPackagesResult, static_cast(initializeActivityContext.GetLastFailure().type), initializeActivityContext.GetLastFailure().file.c_str(), - initializeActivityContext.GetLastFailure().lineNumer, + initializeActivityContext.GetLastFailure().lineNumber, initializeActivityContext.GetLastFailure().message.c_str(), initializeActivityContext.GetLastFailure().module.c_str(), static_cast(status), @@ -295,7 +295,7 @@ namespace winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::implem deploymentResult.ActivityId()); } - return hrAddPackage; + return deploymentResult.ExtendedErrorCode(); } CATCH_RETURN() diff --git a/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.cpp b/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.cpp index e39d4c2f7..8ae9ab120 100644 --- a/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.cpp +++ b/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.cpp @@ -25,7 +25,7 @@ void WindowsAppRuntime::MddBootstrap::Activity::Context::SetLastFailure(const wi m_lastFailure.file.clear(); } - m_lastFailure.lineNumer = failure.uLineNumber; + m_lastFailure.lineNumber = failure.uLineNumber; if (failure.pszMessage) { diff --git a/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.h b/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.h index c216d93e7..ae91c6e9e 100644 --- a/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.h +++ b/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapActivity.h @@ -29,7 +29,7 @@ namespace WindowsAppRuntime::MddBootstrap::Activity wil::FailureType type; HRESULT hr; std::string file; - unsigned int lineNumer; + unsigned int lineNumber; std::wstring message; std::string module; }; diff --git a/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapTracelogging.cpp b/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapTracelogging.cpp index 002808ce1..8ab67b3be 100644 --- a/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapTracelogging.cpp +++ b/dev/WindowsAppRuntime_BootstrapDLL/MddBootstrapTracelogging.cpp @@ -24,7 +24,7 @@ void MddBootstrap_StopActivity( initializationFrameworkPackageFullName, static_cast(activityContext.GetLastFailure().type), activityContext.GetLastFailure().file.c_str(), - activityContext.GetLastFailure().lineNumer, + activityContext.GetLastFailure().lineNumber, activityContext.GetLastFailure().message.c_str(), activityContext.GetLastFailure().module.c_str()); } @@ -35,7 +35,7 @@ void MddBootstrap_StopActivity( static_cast(initializationCount), static_cast(activityContext.GetLastFailure().type), activityContext.GetLastFailure().file.c_str(), - activityContext.GetLastFailure().lineNumer, + activityContext.GetLastFailure().lineNumber, activityContext.GetLastFailure().message.c_str(), activityContext.GetLastFailure().module.c_str()); } diff --git a/installer/dev/InstallActivityContext.cpp b/installer/dev/InstallActivityContext.cpp index 1e7b42de2..4fb9f897e 100644 --- a/installer/dev/InstallActivityContext.cpp +++ b/installer/dev/InstallActivityContext.cpp @@ -44,7 +44,7 @@ void WindowsAppRuntimeInstaller::InstallActivity::Context::SetLastFailure(const m_lastFailure.file.clear(); } - m_lastFailure.lineNumer = failure.uLineNumber; + m_lastFailure.lineNumber = failure.uLineNumber; if (failure.pszMessage) { @@ -131,6 +131,21 @@ BOOL WindowsAppRuntimeInstaller::InstallActivity::Context::LogInstallerFailureEv THROW_IF_WIN32_BOOL_FALSE(LogInstallerFailureEventWithResourceId(EVENTLOG_WARNING_TYPE, hresult, customProvisionMessage)); break; } + case InstallStage::StagePackage: + { + WCHAR customMessage[1024]{}; + auto deploymentActivityId{ winrt::to_hstring(*m_activity.Id()) }; + auto customMessageFormat{ L"Staging Package %s with DeploymentExtendedError: 0x%08X, DeploymentExtendedText:%s, DeploymentActivityId: %s" }; + FAIL_FAST_IF_FAILED(StringCchPrintfW(customMessage, + ARRAYSIZE(customMessage), + customMessageFormat, + m_currentResourceId, + m_deploymentErrorExtendedHresult, + m_deploymentErrorText, + m_deploymentErrorActivityId)); + THROW_IF_WIN32_BOOL_FALSE(LogInstallerFailureEventWithResourceId(EVENTLOG_ERROR_TYPE, hresult, customMessage)); + break; + } default: break; } diff --git a/installer/dev/InstallActivityContext.h b/installer/dev/InstallActivityContext.h index bdabc8a32..a0b5188b2 100644 --- a/installer/dev/InstallActivityContext.h +++ b/installer/dev/InstallActivityContext.h @@ -17,6 +17,7 @@ namespace WindowsAppRuntimeInstaller::InstallActivity RegisterPackage = 0x5, ProvisionPackage = 0x6, RestartPushNotificationsLRP = 0x7, + StagePackage = 0x8, }; struct WilFailure @@ -24,7 +25,7 @@ namespace WindowsAppRuntimeInstaller::InstallActivity wil::FailureType type; HRESULT hr; std::string file; - unsigned int lineNumer; + unsigned int lineNumber; std::wstring message; }; @@ -55,12 +56,11 @@ namespace WindowsAppRuntimeInstaller::InstallActivity return m_currentResourceId; } - const HRESULT& GetdeploymentErrorHresult() const + const HRESULT& GetDeploymentErrorHresult() const { return m_deploymentErrorHresult; } - const HRESULT& GetDeploymentErrorExtendedHResult() const { return m_deploymentErrorExtendedHresult; diff --git a/installer/dev/console.cpp b/installer/dev/console.cpp index d07f7e8c6..f91042766 100644 --- a/installer/dev/console.cpp +++ b/installer/dev/console.cpp @@ -38,7 +38,19 @@ void WindowsAppRuntimeInstaller::Console::DisplayInfo() void WindowsAppRuntimeInstaller::Console::DisplayError(const HRESULT hr) { - if (SUCCEEDED(hr)) + auto& installActivityContext{ WindowsAppRuntimeInstaller::InstallActivity::Context::Get() }; + + HRESULT hResult = hr; + + if (installActivityContext.GetDeploymentErrorHresult() && + installActivityContext.GetInstallStage() == InstallStage::StagePackage || + installActivityContext.GetInstallStage() == InstallStage::AddPackage || + installActivityContext.GetInstallStage() == InstallStage::RegisterPackage) + { + hResult = installActivityContext.GetDeploymentErrorHresult(); + } + + if (SUCCEEDED(hResult)) { std::wcout << std::endl; return; @@ -46,16 +58,16 @@ void WindowsAppRuntimeInstaller::Console::DisplayError(const HRESULT hr) wil::unique_hlocal_ptr message; if (FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, - nullptr, hr, 0, reinterpret_cast(&message), 0, nullptr) != 0) + nullptr, hResult, 0, reinterpret_cast(&message), 0, nullptr) != 0) { std::wcout << message.get(); } - auto& installActivityContext{ WindowsAppRuntimeInstaller::InstallActivity::Context::Get() }; // Don't log redundant Hr information if (installActivityContext.GetDeploymentErrorExtendedHResult() && - installActivityContext.GetDeploymentErrorExtendedHResult() != hr && - (installActivityContext.GetInstallStage() == InstallStage::AddPackage || + installActivityContext.GetDeploymentErrorExtendedHResult() != hResult && + (installActivityContext.GetInstallStage() == InstallStage::StagePackage || + installActivityContext.GetInstallStage() == InstallStage::AddPackage || installActivityContext.GetInstallStage() == InstallStage::RegisterPackage)) { std::wcout << "ExtendedError: 0x" << std::hex << installActivityContext.GetDeploymentErrorExtendedHResult() << " "; @@ -67,11 +79,11 @@ void WindowsAppRuntimeInstaller::Console::DisplayError(const HRESULT hr) } } - if (installActivityContext.GetDeploymentErrorText().empty() && - (installActivityContext.GetInstallStage() == InstallStage::AddPackage || + if (!installActivityContext.GetDeploymentErrorText().empty() && + (installActivityContext.GetInstallStage() == InstallStage::StagePackage || + installActivityContext.GetInstallStage() == InstallStage::AddPackage || installActivityContext.GetInstallStage() == InstallStage::RegisterPackage)) { std::wcout << "ErrorMessage: " << installActivityContext.GetDeploymentErrorText(); } } - diff --git a/installer/dev/install.cpp b/installer/dev/install.cpp index cf0d9fd73..c2c46a5a6 100644 --- a/installer/dev/install.cpp +++ b/installer/dev/install.cpp @@ -38,14 +38,17 @@ namespace WindowsAppRuntimeInstaller return S_OK; } - HRESULT AddPackage(const Uri& packageUri, const std::unique_ptr& packageProperties, bool forceDeployment) + HRESULT AddPackage( + WindowsAppRuntimeInstaller::InstallActivity::Context& installActivityContext, + const Uri& packageUri, + const std::unique_ptr& packageProperties, + bool forceDeployment) { - PackageManager packageManager; - const auto deploymentOptions{ forceDeployment ? - winrt::Windows::Management::Deployment::DeploymentOptions::ForceTargetApplicationShutdown : - winrt::Windows::Management::Deployment::DeploymentOptions::None }; + winrt::Windows::Management::Deployment::DeploymentOptions::ForceTargetApplicationShutdown : + winrt::Windows::Management::Deployment::DeploymentOptions::None }; + PackageManager packageManager; const auto deploymentOperation{ packageManager.AddPackageAsync(packageUri, nullptr, DeploymentOptions::None) }; deploymentOperation.get(); if (deploymentOperation.Status() != AsyncStatus::Completed) @@ -53,7 +56,7 @@ namespace WindowsAppRuntimeInstaller auto hrAddPackage{ static_cast(deploymentOperation.ErrorCode()) }; if (hrAddPackage == ERROR_PACKAGE_ALREADY_EXISTS) { - WindowsAppRuntimeInstaller::InstallActivity::Context::Get().SetInstallStage(InstallStage::RegisterPackage); + installActivityContext.SetInstallStage(InstallStage::RegisterPackage); // Package already exists (such as via provisioning), re-register it instead. RETURN_IF_FAILED(RegisterPackage(packageProperties->fullName.get())); @@ -62,14 +65,62 @@ namespace WindowsAppRuntimeInstaller else { const auto deploymentResult{ deploymentOperation.GetResults() }; - WindowsAppRuntimeInstaller::InstallActivity::Context::Get().SetDeploymentErrorInfo(hrAddPackage, deploymentResult.ExtendedErrorCode(), deploymentResult.ErrorText().c_str(), deploymentResult.ActivityId()); - + installActivityContext.SetDeploymentErrorInfo(hrAddPackage, deploymentResult.ExtendedErrorCode(), deploymentResult.ErrorText().c_str(), deploymentResult.ActivityId()); RETURN_HR(static_cast(deploymentResult.ExtendedErrorCode())); } } return S_OK; } + HRESULT StagePackage( + WindowsAppRuntimeInstaller::InstallActivity::Context& installActivityContext, + const Uri& packageUri) + { + const auto deploymentOptions{ winrt::Windows::Management::Deployment::DeploymentOptions::None }; + + PackageManager packageManager; + const auto deploymentOperation{ packageManager.StagePackageAsync(packageUri, nullptr, DeploymentOptions::None) }; + deploymentOperation.get(); + if (deploymentOperation.Status() != AsyncStatus::Completed) + { + auto hrStagePackage{ static_cast(deploymentOperation.ErrorCode()) }; + if (hrStagePackage == ERROR_PACKAGE_ALREADY_EXISTS) + { + // Package already exists, nothing more to do + return S_OK; + } + else + { + const auto deploymentResult{ deploymentOperation.GetResults() }; + installActivityContext.SetDeploymentErrorInfo(hrStagePackage, deploymentResult.ExtendedErrorCode(), deploymentResult.ErrorText().c_str(), deploymentResult.ActivityId()); + RETURN_HR(static_cast(deploymentResult.ExtendedErrorCode())); + } + } + return S_OK; + } + + HRESULT AddOrStagePackage( + WindowsAppRuntimeInstaller::InstallActivity::Context& installActivityContext, + const Uri& packageUri, + const std::unique_ptr& packageProperties, + bool forceDeployment) + { + // Windows doesn't support registering packages for LocalSystem + // If you're doing that you're really intending to provision the package for all users on the machine + // That means we need to Stage the package instead of Add it + if (Security::User::IsLocalSystem()) + { + installActivityContext.SetInstallStage(InstallStage::StagePackage); + RETURN_IF_FAILED(StagePackage(installActivityContext, packageUri)); + } + else + { + installActivityContext.SetInstallStage(InstallStage::AddPackage); + RETURN_IF_FAILED(AddPackage(installActivityContext, packageUri, packageProperties, forceDeployment)); + } + return S_OK; + } + HRESULT ProvisionPackage(const std::wstring& packageFamilyName) { PackageManager packageManager; @@ -253,11 +304,9 @@ namespace WindowsAppRuntimeInstaller THROW_IF_FAILED(outStream->Commit(STGC_OVERWRITE)); outStream.reset(); - installActivityContext.SetInstallStage(InstallStage::AddPackage); - - // Add the package + // Add-or-Stage the package Uri packageUri{ packageFilename }; - auto hrAddResult{ AddPackage(packageUri, packageProperties, forceDeployment) }; + auto hrAddResult{ AddOrStagePackage(installActivityContext, packageUri, packageProperties, forceDeployment) }; if (!quiet) { std::wcout << "Package deployment result : 0x" << std::hex << hrAddResult << " "; @@ -267,8 +316,7 @@ namespace WindowsAppRuntimeInstaller // Framework provisioning is not supported by the PackageManager ProvisionPackageForAllUsersAsync API. // Hence, skip attempting to provision framework package. - if (!packageProperties->isFramework && - Security::IntegrityLevel::IsElevated()) + if (!packageProperties->isFramework && Security::IntegrityLevel::IsElevated()) { installActivityContext.SetInstallStage(InstallStage::ProvisionPackage); diff --git a/installer/dev/main.cpp b/installer/dev/main.cpp index f28b2ecce..953218717 100644 --- a/installer/dev/main.cpp +++ b/installer/dev/main.cpp @@ -132,11 +132,11 @@ int wmain(int argc, wchar_t *argv[]) deployPackagesResult, static_cast(installActivityContext.GetLastFailure().type), installActivityContext.GetLastFailure().file.c_str(), - installActivityContext.GetLastFailure().lineNumer, + installActivityContext.GetLastFailure().lineNumber, installActivityContext.GetLastFailure().message.c_str(), static_cast(installActivityContext.GetInstallStage()), installActivityContext.GetCurrentResourceId().c_str(), - installActivityContext.GetdeploymentErrorHresult(), + installActivityContext.GetDeploymentErrorHresult(), installActivityContext.GetDeploymentErrorText().c_str(), installActivityContext.GetDeploymentErrorActivityId()); } diff --git a/installer/dev/pch.h b/installer/dev/pch.h index 4116c85eb..44615ed66 100644 --- a/installer/dev/pch.h +++ b/installer/dev/pch.h @@ -30,6 +30,7 @@ #include #include +#include #include #include "tracelogging.h" diff --git a/installer/dev/tracelogging.cpp b/installer/dev/tracelogging.cpp index b88a7816a..89f255457 100644 --- a/installer/dev/tracelogging.cpp +++ b/installer/dev/tracelogging.cpp @@ -77,7 +77,7 @@ void __stdcall wilResultLoggingCallback(const wil::FailureInfo& failure) noexcep failure.pszMessage, static_cast(installActivityContext.GetInstallStage()), installActivityContext.GetCurrentResourceId().c_str(), - installActivityContext.GetdeploymentErrorHresult(), + installActivityContext.GetDeploymentErrorHresult(), installActivityContext.GetDeploymentErrorText().c_str(), installActivityContext.GetDeploymentErrorActivityId()); diff --git a/test/Common/Common.vcxproj b/test/Common/Common.vcxproj index 3645b19a9..a0062156d 100644 --- a/test/Common/Common.vcxproj +++ b/test/Common/Common.vcxproj @@ -111,6 +111,7 @@ Create + diff --git a/test/Common/Common.vcxproj.filters b/test/Common/Common.vcxproj.filters index 165995cbb..de7921e7f 100644 --- a/test/Common/Common.vcxproj.filters +++ b/test/Common/Common.vcxproj.filters @@ -24,6 +24,9 @@ Source Files + + Source Files + diff --git a/test/Common/Test_Security_User.cpp b/test/Common/Test_Security_User.cpp new file mode 100644 index 000000000..8c3c8d7be --- /dev/null +++ b/test/Common/Test_Security_User.cpp @@ -0,0 +1,35 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +#include "pch.h" + +namespace Test::Common +{ + class SecurityUserTests_User + { + public: + BEGIN_TEST_CLASS(SecurityUserTests_User) + TEST_CLASS_PROPERTY(L"ThreadingModel", L"MTA") + TEST_CLASS_PROPERTY(L"RunAs", L"RestrictedUser") + END_TEST_CLASS() + + TEST_METHOD(IsLocalSystem) + { + VERIFY_IS_FALSE(::Security::User::IsLocalSystem()); + } + }; + + class SecurityUserTests_System + { + public: + BEGIN_TEST_CLASS(SecurityUserTests_System) + TEST_CLASS_PROPERTY(L"ThreadingModel", L"MTA") + TEST_CLASS_PROPERTY(L"RunAs", L"System") + END_TEST_CLASS() + + TEST_METHOD(IsLocalSystem) + { + VERIFY_IS_TRUE(::Security::User::IsLocalSystem()); + } + }; +} diff --git a/test/Common/pch.h b/test/Common/pch.h index 12ba0fb3a..58bcf86d6 100644 --- a/test/Common/pch.h +++ b/test/Common/pch.h @@ -35,6 +35,7 @@ #include #include +#include #include #include