From 333aef28440331d1ed325a3ed588ef2754821f8e Mon Sep 17 00:00:00 2001 From: Nir Bar Date: Tue, 2 Apr 2024 09:55:18 +0300 Subject: [PATCH] Fix reverting impersonation --- src/PanelSwCustomActions/ExecOnComponent.cpp | 54 ++++++++++++-------- src/PanelSwCustomActions/ExecOnComponent.h | 11 +++- src/TidyBuild.custom.props | 2 +- src/wixlib/PanelSwWixExtension.wxs | 20 ++++---- 4 files changed, 53 insertions(+), 34 deletions(-) diff --git a/src/PanelSwCustomActions/ExecOnComponent.cpp b/src/PanelSwCustomActions/ExecOnComponent.cpp index 67d4b73..d0e0a7f 100644 --- a/src/PanelSwCustomActions/ExecOnComponent.cpp +++ b/src/PanelSwCustomActions/ExecOnComponent.cpp @@ -617,13 +617,12 @@ HRESULT CExecOnComponent::ExecuteOne(const com::panelsw::ca::ExecOnDetails& deta LPCWSTR szDomain = nullptr; LPCWSTR szUser = nullptr; LPCWSTR szPassword = nullptr; - HANDLE hImpersonation = NULL; - bool bImpersonated = false; CWixString szLog; HANDLE hStdOut = NULL; HANDLE hProc = NULL; PMSIHANDLE hActionData; CWixString szEnvironmentMultiSz; + IMPERSONATION_CONTEXT ctxImpersonation; hr = szCommand.Copy((LPCWSTR)(LPVOID)details.command().plain().data()); ExitOnFailure(hr, "Failed to copy string"); @@ -649,9 +648,8 @@ HRESULT CExecOnComponent::ExecuteOne(const com::panelsw::ca::ExecOnDetails& deta } // We only impersonate for the duration of the process creation because I've encountered crashes when logging impersonated - hr = Impersonate(szDomain, szUser, szPassword, details.sessionid(), &hImpersonation, &szEnvironmentMultiSz); + hr = Impersonate(szDomain, szUser, szPassword, details.sessionid(), &szEnvironmentMultiSz, &ctxImpersonation); ExitOnFailure(hr, "Failed to impersonate"); - bImpersonated = (hr == S_OK); hr = SetEnvironment(&szEnvironmentMultiSz, details.environment()); ExitOnFailure(hr, "Failed setting environment"); @@ -665,14 +663,7 @@ HRESULT CExecOnComponent::ExecuteOne(const com::panelsw::ca::ExecOnDetails& deta ExitFunction(); } - if (bImpersonated) - { - bRes = RevertToSelf(); - ExitOnNullWithLastError(bRes, hr, "Failed reverting impersonation"); - bImpersonated = false; - - ReleaseHandle(hImpersonation); - } + Unimpersonate(&ctxImpersonation); if (SUCCEEDED(hr)) { @@ -728,11 +719,7 @@ HRESULT CExecOnComponent::ExecuteOne(const com::panelsw::ca::ExecOnDetails& deta ExitOnFailure(hr, "Failed to execute command '%ls'", szObfuscatedCommand); LExit: - if (bImpersonated) - { - ::RevertToSelf(); - } - ReleaseHandle(hImpersonation); + Unimpersonate(&ctxImpersonation); if (hStdOut && (hStdOut != INVALID_HANDLE_VALUE)) { ReleaseHandle(hStdOut); @@ -973,7 +960,7 @@ HRESULT CExecOnComponent::SetEnvironment(CWixString* pszEnvironmentMultiSz, cons { LPCWSTR szEqual = wcschr(szCurr, L'=');; DWORD dwLen1 = szEqual - szCurr; - if (!dwLen1) + if (!dwLen1 || !szEqual) { continue; } @@ -1096,6 +1083,7 @@ HRESULT CExecOnComponent::LaunchProcess(LPWSTR szCommand, LPCWSTR szWorkingDirec bRes = ::CreateProcessW(nullptr, szCommand, nullptr, nullptr, TRUE, ::GetPriorityClass(::GetCurrentProcess()) | CREATE_NO_WINDOW | CREATE_UNICODE_ENVIRONMENT, (LPVOID)rgszEnvironment, szWorkingDirectory, &si, &pi); ExitOnNullWithLastError(bRes, hr, "Failed to create process"); + WcaLog(LOGLEVEL::LOGMSG_VERBOSE, "Launched process '%ls'", szCommand); if (phStdOut) { @@ -1122,7 +1110,7 @@ HRESULT CExecOnComponent::LaunchProcess(LPWSTR szCommand, LPCWSTR szWorkingDirec return hr; } -HRESULT CExecOnComponent::Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR szPassword, DWORD dwSessionId, HANDLE* phUserToken, CWixString* pszEnvironmentMultiSz) +HRESULT CExecOnComponent::Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR szPassword, DWORD dwSessionId, CWixString* pszEnvironmentMultiSz, IMPERSONATION_CONTEXT *pctxImpersonate) { HRESULT hr = S_OK; BOOL bRes = TRUE; @@ -1135,13 +1123,14 @@ HRESULT CExecOnComponent::Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR CWixString szTempUserName; ZeroMemory(&profileInfo, sizeof(profileInfo)); + Unimpersonate(pctxImpersonate); // Impersonate current user? if (dwSessionId != WTS_CURRENT_SESSION) { DWORD dwNameSize = 0; - bRes = ::WTSQuerySessionInformationW(NULL, dwSessionId, WTS_INFO_CLASS::WTSUserName, &szSessionUserName, &dwNameSize); + bRes = ::WTSQuerySessionInformation(NULL, dwSessionId, WTS_INFO_CLASS::WTSUserName, &szSessionUserName, &dwNameSize); ExitOnNullWithLastError(bRes, hr, "Failed to get user name"); WcaLog(LOGLEVEL::LOGMSG_STANDARD, "Impersonating '%ls'", szSessionUserName); @@ -1180,7 +1169,7 @@ HRESULT CExecOnComponent::Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR if (!::LoadUserProfile(hUserToken, &profileInfo)) { - WcaLogError(HRESULT_FROM_WIN32(::GetLastError()), "Failed to load user profile"); + WcaLogError(HRESULT_FROM_WIN32(::GetLastError()), "Failed to load user profile, so running without it"); } } @@ -1200,6 +1189,12 @@ HRESULT CExecOnComponent::Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR ExitOnFailure(hr, "Failed to insert string to array"); } + pctxImpersonate->bImpersonated = bImpersonated; + pctxImpersonate->hUserToken = hUserToken; + pctxImpersonate->hProfile = profileInfo.hProfile; + hUserToken = NULL; + profileInfo.hProfile = NULL; + LExit: if (pEnvironment) { @@ -1221,6 +1216,21 @@ HRESULT CExecOnComponent::Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR ::NetApiBufferFree(pUserInfo4); pUserInfo4 = nullptr; } + return SUCCEEDED(hr) ? bImpersonated ? S_OK : S_FALSE : hr; +} - return SUCCEEDED(hr) ? bImpersonated ? S_FALSE : S_OK : hr; +void CExecOnComponent::Unimpersonate(IMPERSONATION_CONTEXT* pctxImpersonate) +{ + if (pctxImpersonate->hUserToken && pctxImpersonate->hProfile) + { + ::UnloadUserProfile(pctxImpersonate->hUserToken, pctxImpersonate->hProfile); + pctxImpersonate->hProfile = NULL; + } + if (pctxImpersonate->bImpersonated) + { + WcaLog(LOGLEVEL::LOGMSG_VERBOSE, "Reverting impersonation"); + ::RevertToSelf(); + pctxImpersonate->bImpersonated = FALSE; + } + ReleaseHandle(pctxImpersonate->hUserToken); } diff --git a/src/PanelSwCustomActions/ExecOnComponent.h b/src/PanelSwCustomActions/ExecOnComponent.h index 5053d7e..0bf4a7f 100644 --- a/src/PanelSwCustomActions/ExecOnComponent.h +++ b/src/PanelSwCustomActions/ExecOnComponent.h @@ -24,9 +24,18 @@ class CExecOnComponent : private: + struct IMPERSONATION_CONTEXT + { + HANDLE hUserToken = NULL; + BOOL bImpersonated = FALSE; + HANDLE hProfile = NULL; + }; + HRESULT ExecuteOne(const com::panelsw::ca::ExecOnDetails &details); - HRESULT Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR szPassword, DWORD dwSessionId, HANDLE* phUserToken, CWixString* pszEnvironmentMultiSz); + HRESULT Impersonate(LPCWSTR szDomain, LPCWSTR szUser, LPCWSTR szPassword, DWORD dwSessionId, CWixString* pszEnvironmentMultiSz, IMPERSONATION_CONTEXT* pctxImpersonation); + + void Unimpersonate(IMPERSONATION_CONTEXT* pctxImpersonation); HRESULT SetEnvironment(CWixString *pszEnvironmentMultiSz, const ::google::protobuf::Map &customEnv); diff --git a/src/TidyBuild.custom.props b/src/TidyBuild.custom.props index d1b59f4..07009e4 100644 --- a/src/TidyBuild.custom.props +++ b/src/TidyBuild.custom.props @@ -2,7 +2,7 @@ - 3.18.11 + 3.18.12 $(FullVersion).$(GITHUB_RUN_NUMBER) PanelSwWixExtension Panel::Software diff --git a/src/wixlib/PanelSwWixExtension.wxs b/src/wixlib/PanelSwWixExtension.wxs index 8d9935a..a3fa91b 100644 --- a/src/wixlib/PanelSwWixExtension.wxs +++ b/src/wixlib/PanelSwWixExtension.wxs @@ -311,19 +311,19 @@ - - - - + + + + - - + + - - + + - - + +