From f2e9af96142439ebfdbc1e884335bb8874f8a427 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Tue, 11 Feb 2025 05:21:34 -0800 Subject: Harden Burn's BootstrapperApplication and elevated engine extraction Fixes 8914 --- src/burn/engine/bootstrapperapplication.cpp | 8 +++ src/burn/engine/cache.cpp | 86 ++++++++++++++++++++-------- src/burn/engine/cache.h | 7 ++- src/burn/engine/container.cpp | 18 ++++++ src/burn/engine/container.h | 5 ++ src/burn/engine/core.cpp | 14 ++--- src/burn/engine/core.h | 1 - src/burn/engine/elevation.cpp | 4 +- src/burn/engine/engine.cpp | 3 +- src/burn/engine/payload.cpp | 14 ++++- src/burn/engine/payload.h | 1 + src/burn/engine/plan.cpp | 1 + src/burn/test/BurnUnitTest/ElevationTest.cpp | 2 +- src/burn/test/BurnUnitTest/ExitCodeTest.cpp | 2 +- 14 files changed, 122 insertions(+), 44 deletions(-) (limited to 'src') diff --git a/src/burn/engine/bootstrapperapplication.cpp b/src/burn/engine/bootstrapperapplication.cpp index 947b3720..dc3bd5da 100644 --- a/src/burn/engine/bootstrapperapplication.cpp +++ b/src/burn/engine/bootstrapperapplication.cpp @@ -306,6 +306,14 @@ EXTERN_C HRESULT BootstrapperApplicationRemove( { HRESULT hr = S_OK; + // Release any open file handles so we can try to recursively delete the temp folder. + for (DWORD i = 0; i < pUserExperience->payloads.cPayloads; ++i) + { + BURN_PAYLOAD* pPayload = pUserExperience->payloads.rgPayloads + i; + + ReleaseFileHandle(pPayload->hLocalFile); + } + // Remove temporary UX directory if (pUserExperience->sczTempDirectory) { diff --git a/src/burn/engine/cache.cpp b/src/burn/engine/cache.cpp index 5a8388c4..16db4a81 100644 --- a/src/burn/engine/cache.cpp +++ b/src/burn/engine/cache.cpp @@ -112,13 +112,15 @@ static HRESULT CopyEngineToWorkingFolder( __in_z LPCWSTR wzWorkingFolderName, __in_z LPCWSTR wzExecutableName, __in BURN_SECTION* pSection, - __deref_out_z_opt LPWSTR* psczEngineWorkingPath + __out_z LPWSTR* psczEngineWorkingPath, + __out HANDLE* phEngineWorkingFile ); static HRESULT CopyEngineWithSignatureFixup( __in HANDLE hEngineFile, __in_z LPCWSTR wzEnginePath, __in_z LPCWSTR wzTargetPath, - __in BURN_SECTION* pSection + __in BURN_SECTION* pSection, + __out HANDLE* phEngineFile ); static HRESULT RemoveBundleOrPackage( __in BURN_CACHE* pCache, @@ -915,33 +917,59 @@ extern "C" HRESULT CacheBundleToWorkingDirectory( __in BOOL fElevated, __in BURN_CACHE* pCache, __in_z LPCWSTR wzExecutableName, - __in BURN_SECTION* pSection, - __deref_out_z_opt LPWSTR* psczEngineWorkingPath + __in BURN_SECTION* pSection ) { Assert(pCache->fInitializedCache); HRESULT hr = S_OK; LPWSTR sczSourcePath = NULL; + LPWSTR sczEngineWorkingPath = NULL; + HANDLE hEngineWorkingFile = INVALID_HANDLE_VALUE; + + // If we already cached the engine, bail. + if (pCache->sczBundleEngineWorkingPath && INVALID_HANDLE_VALUE != pCache->hBundleEngineWorkingFile) + { + ExitFunction(); + } // Initialize the source. hr = PathForCurrentProcess(&sczSourcePath, NULL); ExitOnFailure(hr, "Failed to get current process path."); // If the bundle is running out of the package cache then we don't need to copy it to - // the working folder since we feel safe in the package cache and will run from there. + // the working folder (and we don't need to lock the file either) since we feel safe + // in the package cache and will run from there. if (CacheBundleRunningFromCache(pCache)) { - hr = StrAllocString(psczEngineWorkingPath, sczSourcePath, 0); - ExitOnFailure(hr, "Failed to use current process path as target path."); + hr = StrAllocString(&sczEngineWorkingPath, sczSourcePath, 0); + ExitOnFailure(hr, "Failed to copy current process path as bundle engine working path."); } - else // otherwise, carry on putting the bundle in the working folder. + else // otherwise, carry on putting the bundle in the working folder and lock it. { - hr = CopyEngineToWorkingFolder(fElevated, pCache, sczSourcePath, BUNDLE_WORKING_FOLDER_NAME, wzExecutableName, pSection, psczEngineWorkingPath); + hr = CopyEngineToWorkingFolder(fElevated, pCache, sczSourcePath, BUNDLE_WORKING_FOLDER_NAME, wzExecutableName, pSection, &sczEngineWorkingPath, &hEngineWorkingFile); ExitOnFailure(hr, "Failed to copy engine to working folder."); - } + + // Close the engine file handle (if we opened it) then reopen it read-only as quickly as possible to lock it. + ReleaseFileHandle(hEngineWorkingFile); + + hr = FileCreateWithRetry(sczEngineWorkingPath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 30, 100, &hEngineWorkingFile); + ExitOnFailure(hr, "Failed to lock bundle engine file: %ls", sczEngineWorkingPath); + } + + // Clean out any previous values (there shouldn't be any). + ReleaseNullStr(pCache->sczBundleEngineWorkingPath); + ReleaseFileHandle(pCache->hBundleEngineWorkingFile); + + pCache->sczBundleEngineWorkingPath = sczEngineWorkingPath; + sczEngineWorkingPath = NULL; + + pCache->hBundleEngineWorkingFile = hEngineWorkingFile; + hEngineWorkingFile = INVALID_HANDLE_VALUE; LExit: + ReleaseFileHandle(hEngineWorkingFile); + ReleaseStr(sczEngineWorkingPath); ReleaseStr(sczSourcePath); return hr; @@ -1206,9 +1234,14 @@ extern "C" HRESULT CacheRemoveBaseWorkingFolder( if (pCache->fInitializedBaseWorkingFolder) { + // Release the engine file handle if it is open to ensure the working folder can be deleted. + ReleaseFileHandle(pCache->hBundleEngineWorkingFile); + // Try to clean out everything in the working folder. hr = DirEnsureDeleteEx(pCache->sczBaseWorkingFolder, DIR_DELETE_FILES | DIR_DELETE_RECURSE | DIR_DELETE_SCHEDULE); TraceError(hr, "Could not delete bundle engine working folder."); + + pCache->fInitializedBaseWorkingFolder = FALSE; } return hr; @@ -1396,6 +1429,8 @@ extern "C" void CacheUninitialize( ReleaseStr(pCache->sczBaseWorkingFolder); ReleaseStr(pCache->sczAcquisitionFolder); ReleaseStr(pCache->sczSourceProcessFolder); + ReleaseStr(pCache->sczBundleEngineWorkingPath) + ReleaseFileHandle(pCache->hBundleEngineWorkingFile) memset(pCache, 0, sizeof(BURN_CACHE)); } @@ -2094,16 +2129,15 @@ static HRESULT CopyEngineToWorkingFolder( __in_z LPCWSTR wzWorkingFolderName, __in_z LPCWSTR wzExecutableName, __in BURN_SECTION* pSection, - __deref_out_z_opt LPWSTR* psczEngineWorkingPath + __out_z LPWSTR* psczEngineWorkingPath, + __out HANDLE* phEngineWorkingFile ) { HRESULT hr = S_OK; LPWSTR sczWorkingFolder = NULL; LPWSTR sczTargetDirectory = NULL; LPWSTR sczTargetPath = NULL; - LPWSTR sczSourceDirectory = NULL; - LPWSTR sczPayloadSourcePath = NULL; - LPWSTR sczPayloadTargetPath = NULL; + HANDLE hTargetFile = INVALID_HANDLE_VALUE; hr = CacheEnsureBaseWorkingFolder(fElevated, pCache, &sczWorkingFolder); ExitOnFailure(hr, "Failed to create working path to copy engine."); @@ -2118,19 +2152,17 @@ static HRESULT CopyEngineToWorkingFolder( ExitOnFailure(hr, "Failed to combine working path with engine file name."); // Copy the engine without any attached containers to the working path. - hr = CopyEngineWithSignatureFixup(pSection->hEngineFile, wzSourcePath, sczTargetPath, pSection); + hr = CopyEngineWithSignatureFixup(pSection->hEngineFile, wzSourcePath, sczTargetPath, pSection, &hTargetFile); ExitOnFailure(hr, "Failed to copy engine: '%ls' to working path: %ls", wzSourcePath, sczTargetPath); - if (psczEngineWorkingPath) - { - hr = StrAllocString(psczEngineWorkingPath, sczTargetPath, 0); - ExitOnFailure(hr, "Failed to copy target path for engine working path."); - } + *psczEngineWorkingPath = sczTargetPath; + sczTargetPath = NULL; + + *phEngineWorkingFile = hTargetFile; + hTargetFile = INVALID_HANDLE_VALUE; LExit: - ReleaseStr(sczPayloadTargetPath); - ReleaseStr(sczPayloadSourcePath); - ReleaseStr(sczSourceDirectory); + ReleaseFileHandle(hTargetFile); ReleaseStr(sczTargetPath); ReleaseStr(sczTargetDirectory); ReleaseStr(sczWorkingFolder); @@ -2143,7 +2175,8 @@ static HRESULT CopyEngineWithSignatureFixup( __in HANDLE hEngineFile, __in_z LPCWSTR wzEnginePath, __in_z LPCWSTR wzTargetPath, - __in BURN_SECTION* pSection + __in BURN_SECTION* pSection, + __out HANDLE* phEngineFile ) { HRESULT hr = S_OK; @@ -2151,7 +2184,7 @@ static HRESULT CopyEngineWithSignatureFixup( LARGE_INTEGER li = { }; DWORD dwZeroOriginals[3] = { }; - hTarget = ::CreateFileW(wzTargetPath, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_DELETE, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL | FILE_FLAG_SEQUENTIAL_SCAN, NULL); + hTarget = ::CreateFileW(wzTargetPath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); if (INVALID_HANDLE_VALUE == hTarget) { ExitWithLastError(hr, "Failed to create engine file at path: %ls", wzTargetPath); @@ -2200,6 +2233,9 @@ static HRESULT CopyEngineWithSignatureFixup( ExitOnFailure(hr, "Failed to zero out original data offset."); } + *phEngineFile = hTarget; + hTarget = INVALID_HANDLE_VALUE; + LExit: ReleaseFileHandle(hTarget); diff --git a/src/burn/engine/cache.h b/src/burn/engine/cache.h index cce35df0..c2e6e157 100644 --- a/src/burn/engine/cache.h +++ b/src/burn/engine/cache.h @@ -49,6 +49,10 @@ typedef struct _BURN_CACHE // Only valid after CacheEnsureBaseWorkingFolder BOOL fInitializedBaseWorkingFolder; LPWSTR sczBaseWorkingFolder; + + // Only valid after CacheBundleToWorkingDirectory + LPWSTR sczBundleEngineWorkingPath; + HANDLE hBundleEngineWorkingFile; } BURN_CACHE; typedef struct _BURN_CACHE_MESSAGE @@ -175,8 +179,7 @@ HRESULT CacheBundleToWorkingDirectory( __in BOOL fElvated, __in BURN_CACHE* pCache, __in_z LPCWSTR wzExecutableName, - __in BURN_SECTION* pSection, - __deref_out_z_opt LPWSTR* psczEngineWorkingPath + __in BURN_SECTION* pSection ); HRESULT CacheLayoutBundle( __in_z LPCWSTR wzExecutableName, diff --git a/src/burn/engine/container.cpp b/src/burn/engine/container.cpp index e6b91532..fad010cf 100644 --- a/src/burn/engine/container.cpp +++ b/src/burn/engine/container.cpp @@ -326,6 +326,24 @@ extern "C" HRESULT ContainerStreamToBuffer( return hr; } +extern "C" HRESULT ContainerStreamToHandle( + __in BURN_CONTAINER_CONTEXT* pContext, + __in HANDLE hFile + ) +{ + HRESULT hr = S_OK; + + switch (pContext->type) + { + case BURN_CONTAINER_TYPE_CABINET: + hr = CabExtractStreamToHandle(pContext, hFile); + break; + } + +//LExit: + return hr; +} + extern "C" HRESULT ContainerSkipStream( __in BURN_CONTAINER_CONTEXT* pContext ) diff --git a/src/burn/engine/container.h b/src/burn/engine/container.h index a38afa90..5306e7db 100644 --- a/src/burn/engine/container.h +++ b/src/burn/engine/container.h @@ -48,6 +48,7 @@ enum BURN_CAB_OPERATION BURN_CAB_OPERATION_NEXT_STREAM, BURN_CAB_OPERATION_STREAM_TO_FILE, BURN_CAB_OPERATION_STREAM_TO_BUFFER, + BURN_CAB_OPERATION_STREAM_TO_HANDLE, BURN_CAB_OPERATION_SKIP_STREAM, BURN_CAB_OPERATION_CLOSE, }; @@ -184,6 +185,10 @@ HRESULT ContainerStreamToBuffer( __out BYTE** ppbBuffer, __out SIZE_T* pcbBuffer ); +HRESULT ContainerStreamToHandle( + __in BURN_CONTAINER_CONTEXT* pContext, + __in HANDLE hFile + ); HRESULT ContainerSkipStream( __in BURN_CONTAINER_CONTEXT* pContext ); diff --git a/src/burn/engine/core.cpp b/src/burn/engine/core.cpp index 3fe9b586..b20f7f0c 100644 --- a/src/burn/engine/core.cpp +++ b/src/burn/engine/core.cpp @@ -586,11 +586,8 @@ extern "C" HRESULT CoreElevate( while (INVALID_HANDLE_VALUE == pEngineState->companionConnection.hPipe) { // If the elevated companion pipe isn't created yet, let's make that happen. - if (!pEngineState->sczBundleEngineWorkingPath) - { - hr = CacheBundleToWorkingDirectory(pEngineState->internalCommand.fInitiallyElevated, &pEngineState->cache, pEngineState->registration.sczExecutableName, &pEngineState->section, &pEngineState->sczBundleEngineWorkingPath); - ExitOnFailure(hr, "Failed to cache engine to working directory."); - } + hr = CacheBundleToWorkingDirectory(pEngineState->internalCommand.fInitiallyElevated, &pEngineState->cache, pEngineState->registration.sczExecutableName, &pEngineState->section); + ExitOnFailure(hr, "Failed to cache engine to working directory."); hr = ElevationElevate(pEngineState, reason, hwndParent); if (E_SUSPECTED_AV_INTERFERENCE == hr && 1 > cAVRetryAttempts) @@ -695,11 +692,8 @@ extern "C" HRESULT CoreApply( ::InitializeCriticalSection(&applyContext.csApply); // Ensure the engine is cached to the working path. - if (!pEngineState->sczBundleEngineWorkingPath) - { - hr = CacheBundleToWorkingDirectory(pEngineState->internalCommand.fInitiallyElevated, &pEngineState->cache, pEngineState->registration.sczExecutableName, &pEngineState->section, &pEngineState->sczBundleEngineWorkingPath); - ExitOnFailure(hr, "Failed to cache engine to working directory."); - } + hr = CacheBundleToWorkingDirectory(pEngineState->internalCommand.fInitiallyElevated, &pEngineState->cache, pEngineState->registration.sczExecutableName, &pEngineState->section); + ExitOnFailure(hr, "Failed to cache engine to working directory."); // Elevate. if (pEngineState->plan.fPerMachine) diff --git a/src/burn/engine/core.h b/src/burn/engine/core.h index 787100b7..c5d0a370 100644 --- a/src/burn/engine/core.h +++ b/src/burn/engine/core.h @@ -172,7 +172,6 @@ typedef struct _BURN_ENGINE_STATE BURN_REDIRECTED_LOGGING_CONTEXT elevatedLoggingContext; HANDLE hUnelevatedLoggingThread; - LPWSTR sczBundleEngineWorkingPath; BURN_PIPE_CONNECTION companionConnection; BURN_PIPE_CONNECTION embeddedConnection; diff --git a/src/burn/engine/elevation.cpp b/src/burn/engine/elevation.cpp index 85d5a543..90e9db01 100644 --- a/src/burn/engine/elevation.cpp +++ b/src/burn/engine/elevation.cpp @@ -1677,8 +1677,8 @@ static HRESULT LaunchElevatedProcess( // Since ShellExecuteEx doesn't support passing inherited handles, don't bother with CoreAppendFileHandleSelfToCommandLine. // We could fallback to using ::DuplicateHandle to inject the file handle later if necessary. - hr = ShelExec(pEngineState->sczBundleEngineWorkingPath, sczParameters, L"runas", NULL, SW_SHOWNA, hwndParent, &hProcess); - ExitOnFailure(hr, "Failed to launch elevated child process: %ls", pEngineState->sczBundleEngineWorkingPath); + hr = ShelExec(pEngineState->cache.sczBundleEngineWorkingPath, sczParameters, L"runas", NULL, SW_SHOWNA, hwndParent, &hProcess); + ExitOnFailure(hr, "Failed to launch elevated child process: %ls", pEngineState->cache.sczBundleEngineWorkingPath); pConnection->dwProcessId = ::GetProcessId(hProcess); pConnection->hProcess = hProcess; diff --git a/src/burn/engine/engine.cpp b/src/burn/engine/engine.cpp index 5fec289d..2232358d 100644 --- a/src/burn/engine/engine.cpp +++ b/src/burn/engine/engine.cpp @@ -364,6 +364,8 @@ static HRESULT InitializeEngineState( HANDLE hSectionFile = hEngineFile; HANDLE hSourceEngineFile = INVALID_HANDLE_VALUE; + pEngineState->cache.hBundleEngineWorkingFile = INVALID_HANDLE_VALUE; + pEngineState->hUnelevatedLoggingThread = INVALID_HANDLE_VALUE; pEngineState->elevatedLoggingContext.hPipe = INVALID_HANDLE_VALUE; pEngineState->elevatedLoggingContext.hThread = INVALID_HANDLE_VALUE; @@ -413,7 +415,6 @@ static void UninitializeEngineState( BurnPipeConnectionUninitialize(&pEngineState->embeddedConnection); BurnPipeConnectionUninitialize(&pEngineState->companionConnection); - ReleaseStr(pEngineState->sczBundleEngineWorkingPath) ReleaseHandle(pEngineState->hMessageWindowThread); diff --git a/src/burn/engine/payload.cpp b/src/burn/engine/payload.cpp index 1d8328e3..270da6aa 100644 --- a/src/burn/engine/payload.cpp +++ b/src/burn/engine/payload.cpp @@ -239,6 +239,7 @@ extern "C" void PayloadUninitialize( ReleaseMem(pPayload->pbCertificateRootThumbprint); ReleaseMem(pPayload->pbCertificateRootPublicKeyIdentifier); ReleaseStr(pPayload->sczSourcePath); + ReleaseFileHandle(pPayload->hLocalFile); ReleaseStr(pPayload->sczLocalFilePath); ReleaseStr(pPayload->sczFailedLocalAcquisitionPath); ReleaseStr(pPayload->downloadSource.sczUrl); @@ -278,6 +279,7 @@ extern "C" HRESULT PayloadExtractUXContainer( LPWSTR sczStreamName = NULL; LPWSTR sczDirectory = NULL; BURN_PAYLOAD* pPayload = NULL; + HANDLE hTargetFile = INVALID_HANDLE_VALUE; // extract all payloads for (;;) @@ -306,9 +308,18 @@ extern "C" HRESULT PayloadExtractUXContainer( hr = DirEnsureExists(sczDirectory, NULL); ExitOnFailure(hr, "Failed to ensure directory exists"); - hr = ContainerStreamToFile(pContainerContext, pPayload->sczLocalFilePath); + hTargetFile = ::CreateFileW(pPayload->sczLocalFilePath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); + ExitOnInvalidHandleWithLastError(hTargetFile, hr, "Failed to create file: %ls", pPayload->sczLocalFilePath); + + hr = ContainerStreamToHandle(pContainerContext, hTargetFile); ExitOnFailure(hr, "Failed to extract file."); + // Reopen the payload for read-only access to prevent the file from being removed or tampered with while the BA is running. + ReleaseFileHandle(hTargetFile); + + hr = FileCreateWithRetry(pPayload->sczLocalFilePath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 30, 100, &pPayload->hLocalFile); + ExitOnFailure(hr, "Failed to open file: %ls", pPayload->sczLocalFilePath); + // flag that the payload has been acquired pPayload->state = BURN_PAYLOAD_STATE_ACQUIRED; } @@ -326,6 +337,7 @@ extern "C" HRESULT PayloadExtractUXContainer( } LExit: + ReleaseFileHandle(hTargetFile); ReleaseStr(sczStreamName); ReleaseStr(sczDirectory); diff --git a/src/burn/engine/payload.h b/src/burn/engine/payload.h index 351fd058..4c21ae54 100644 --- a/src/burn/engine/payload.h +++ b/src/burn/engine/payload.h @@ -57,6 +57,7 @@ typedef struct _BURN_PAYLOAD // mutable members BURN_PAYLOAD_STATE state; LPWSTR sczLocalFilePath; // location of extracted or downloaded copy + HANDLE hLocalFile; LPWSTR sczUnverifiedPath; DWORD cRemainingInstances; diff --git a/src/burn/engine/plan.cpp b/src/burn/engine/plan.cpp index 47b1c621..7994dd32 100644 --- a/src/burn/engine/plan.cpp +++ b/src/burn/engine/plan.cpp @@ -2117,6 +2117,7 @@ static void ResetPlannedPayloadsState( pPayload->cRemainingInstances = 0; pPayload->state = BURN_PAYLOAD_STATE_NONE; pPayload->fFailedVerificationFromAcquisition = FALSE; + ReleaseFileHandle(pPayload->hLocalFile); ReleaseNullStr(pPayload->sczLocalFilePath); ReleaseNullStr(pPayload->sczFailedLocalAcquisitionPath); } diff --git a/src/burn/test/BurnUnitTest/ElevationTest.cpp b/src/burn/test/BurnUnitTest/ElevationTest.cpp index f9ae2579..81e9f93e 100644 --- a/src/burn/test/BurnUnitTest/ElevationTest.cpp +++ b/src/burn/test/BurnUnitTest/ElevationTest.cpp @@ -55,7 +55,7 @@ namespace Bootstrapper BURN_PIPE_CONNECTION* pConnection = &engineState.companionConnection; DWORD dwResult = S_OK; - engineState.sczBundleEngineWorkingPath = L"tests\\ignore\\this\\path\\to\\burn.exe"; + engineState.cache.sczBundleEngineWorkingPath = L"tests\\ignore\\this\\path\\to\\burn.exe"; try { diff --git a/src/burn/test/BurnUnitTest/ExitCodeTest.cpp b/src/burn/test/BurnUnitTest/ExitCodeTest.cpp index c742543b..9b66f4c0 100644 --- a/src/burn/test/BurnUnitTest/ExitCodeTest.cpp +++ b/src/burn/test/BurnUnitTest/ExitCodeTest.cpp @@ -105,7 +105,7 @@ static void LoadEngineState( { (DWORD)HRESULT_FROM_WIN32(ERROR_FAIL_REBOOT_INITIATED), HRESULT_FROM_WIN32(ERROR_FAIL_REBOOT_INITIATED), BOOTSTRAPPER_APPLY_RESTART_NONE, L"Custom" }, }; - engineState.sczBundleEngineWorkingPath = L"tests\\ignore\\this\\path\\to\\burn.exe"; + engineState.cache.sczBundleEngineWorkingPath = L"tests\\ignore\\this\\path\\to\\burn.exe"; try { -- cgit v1.2.3-55-g6feb