From 0410df93a5f29e4235b4b78b90a1384ffafd062b Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Fri, 22 Mar 2024 20:55:43 +0200 Subject: [PATCH] Don't follow junctions when recursing directories. When deleting directories recursively, an elevated custom action following junctions in a user-writable location could recurse into any directory, including some that you might not want to be deleted. Therefore, avoid recursing into directories that are actually junctions (aka "reparse points"). This applies to: - DTF's custom action runner. Protect elevated working folder from malicious data When running elevated, Burn uses the Windows Temp folder as its working folder to prevent normal processes from tampering with the files. Windows Temp does allow non-elevated processes to write to the folder but they cannot see the files there. Unfortunately, contrary to our belief, non-elevated processes can read the files in Windows Temp by watching for directory changes. This allows a malicious process to lie in wait, watching the Windows Temp folder until a Burn process is launched elevated, then attack the working folder. Mitigate that attack by protecting the working folder to only elevated users. Managed custom actions also fall back to using the Windows Temp folder in some cases and thus can be exposed in a similar fashion as an elevated Burn process. Remove that possibility. Work around lack of upper-bound limit on extension versions See issue 8033 for more details --- src/burn/engine/cache.cpp | 35 +++++++++++++++--- src/burn/engine/cache.h | 3 ++ src/burn/engine/core.cpp | 6 ++-- src/burn/engine/engine.cpp | 2 +- src/burn/engine/userexperience.cpp | 3 +- src/burn/engine/userexperience.h | 1 + src/dtf/SfxCA/SfxUtil.cpp | 36 +++++-------------- .../NetfxExtensionFixture.cs | 12 +++---- .../ExtensionFixture.cs | 2 +- 9 files changed, 57 insertions(+), 43 deletions(-) diff --git a/src/burn/engine/cache.cpp b/src/burn/engine/cache.cpp index 251cd24bc..119200ecc 100644 --- a/src/burn/engine/cache.cpp +++ b/src/burn/engine/cache.cpp @@ -107,6 +107,7 @@ static HRESULT SecurePath( __in LPCWSTR wzPath ); static HRESULT CopyEngineToWorkingFolder( + __in BOOL fElevated, __in BURN_CACHE* pCache, __in_z LPCWSTR wzSourcePath, __in_z LPCWSTR wzWorkingFolderName, @@ -342,6 +343,7 @@ extern "C" HRESULT CacheEnsureAcquisitionFolder( } extern "C" HRESULT CacheEnsureBaseWorkingFolder( + __in BOOL fElevated, __in BURN_CACHE* pCache, __deref_out_z_opt LPWSTR* psczBaseWorkingFolder ) @@ -350,15 +352,32 @@ extern "C" HRESULT CacheEnsureBaseWorkingFolder( HRESULT hr = S_OK; LPWSTR sczPotential = NULL; + PSECURITY_DESCRIPTOR psd = NULL; + LPSECURITY_ATTRIBUTES pWorkingFolderAcl = NULL; if (!pCache->fInitializedBaseWorkingFolder) { + // If elevated, allocate the pWorkingFolderAcl to protect the working folder to only SYSTEM and Admins. + if (fElevated) + { + LPCWSTR wzSddl = L"D:PAI(A;;FA;;;BA)(A;OICIIO;GA;;;BA)(A;;FA;;;SY)(A;OICIIO;GA;;;SY)"; + if (!::ConvertStringSecurityDescriptorToSecurityDescriptorW(wzSddl, SDDL_REVISION_1, &psd, NULL)) + { + ExitWithLastError(hr, "Failed to create the security descriptor for the working folder."); + } + + pWorkingFolderAcl = reinterpret_cast(MemAlloc(sizeof(SECURITY_ATTRIBUTES), TRUE)); + pWorkingFolderAcl->nLength = sizeof(SECURITY_ATTRIBUTES); + pWorkingFolderAcl->lpSecurityDescriptor = psd; + pWorkingFolderAcl->bInheritHandle = FALSE; + } + for (DWORD i = 0; i < pCache->cPotentialBaseWorkingFolders; ++i) { hr = PathConcatRelativeToFullyQualifiedBase(pCache->rgsczPotentialBaseWorkingFolders[i], pCache->wzGuid, &sczPotential); if (SUCCEEDED(hr)) { - hr = DirEnsureExists(sczPotential, NULL); + hr = DirEnsureExists(sczPotential, pWorkingFolderAcl); if (SUCCEEDED(hr)) { pCache->sczBaseWorkingFolder = sczPotential; @@ -385,6 +404,11 @@ extern "C" HRESULT CacheEnsureBaseWorkingFolder( } LExit: + ReleaseMem(pWorkingFolderAcl); + if (psd) + { + ::LocalFree(psd); + } ReleaseStr(sczPotential); return hr; @@ -900,6 +924,7 @@ extern "C" HRESULT CachePreparePackage( } extern "C" HRESULT CacheBundleToCleanRoom( + __in BOOL fElevated, __in BURN_CACHE* pCache, __in BURN_SECTION* pSection, __deref_out_z_opt LPWSTR* psczCleanRoomBundlePath @@ -914,7 +939,7 @@ extern "C" HRESULT CacheBundleToCleanRoom( wzExecutableName = PathFile(sczSourcePath); - hr = CopyEngineToWorkingFolder(pCache, sczSourcePath, BUNDLE_CLEAN_ROOM_WORKING_FOLDER_NAME, wzExecutableName, pSection, psczCleanRoomBundlePath); + hr = CopyEngineToWorkingFolder(fElevated, pCache, sczSourcePath, BUNDLE_CLEAN_ROOM_WORKING_FOLDER_NAME, wzExecutableName, pSection, psczCleanRoomBundlePath); ExitOnFailure(hr, "Failed to cache bundle to clean room."); LExit: @@ -924,6 +949,7 @@ extern "C" HRESULT CacheBundleToCleanRoom( } extern "C" HRESULT CacheBundleToWorkingDirectory( + __in BOOL fElevated, __in BURN_CACHE* pCache, __in_z LPCWSTR wzExecutableName, __in BURN_SECTION* pSection, @@ -948,7 +974,7 @@ extern "C" HRESULT CacheBundleToWorkingDirectory( } else // otherwise, carry on putting the bundle in the working folder. { - hr = CopyEngineToWorkingFolder(pCache, sczSourcePath, BUNDLE_WORKING_FOLDER_NAME, wzExecutableName, pSection, psczEngineWorkingPath); + hr = CopyEngineToWorkingFolder(fElevated, pCache, sczSourcePath, BUNDLE_WORKING_FOLDER_NAME, wzExecutableName, pSection, psczEngineWorkingPath); ExitOnFailure(hr, "Failed to copy engine to working folder."); } @@ -2099,6 +2125,7 @@ static HRESULT SecurePath( static HRESULT CopyEngineToWorkingFolder( + __in BOOL fElevated, __in BURN_CACHE* pCache, __in_z LPCWSTR wzSourcePath, __in_z LPCWSTR wzWorkingFolderName, @@ -2115,7 +2142,7 @@ static HRESULT CopyEngineToWorkingFolder( LPWSTR sczPayloadSourcePath = NULL; LPWSTR sczPayloadTargetPath = NULL; - hr = CacheEnsureBaseWorkingFolder(pCache, &sczWorkingFolder); + hr = CacheEnsureBaseWorkingFolder(fElevated, pCache, &sczWorkingFolder); ExitOnFailure(hr, "Failed to create working path to copy engine."); hr = PathConcatRelativeToFullyQualifiedBase(sczWorkingFolder, wzWorkingFolderName, &sczTargetDirectory); diff --git a/src/burn/engine/cache.h b/src/burn/engine/cache.h index cc28166e4..1ad5d96c4 100644 --- a/src/burn/engine/cache.h +++ b/src/burn/engine/cache.h @@ -97,6 +97,7 @@ HRESULT CacheEnsureAcquisitionFolder( __in BURN_CACHE* pCache ); HRESULT CacheEnsureBaseWorkingFolder( + __in BOOL fElevated, __in BURN_CACHE* pCache, __deref_out_z_opt LPWSTR* psczBaseWorkingFolder ); @@ -172,11 +173,13 @@ HRESULT CachePreparePackage( __in BURN_PACKAGE* pPackage ); HRESULT CacheBundleToCleanRoom( + __in BOOL fElevated, __in BURN_CACHE* pCache, __in BURN_SECTION* pSection, __deref_out_z_opt LPWSTR* psczCleanRoomBundlePath ); HRESULT CacheBundleToWorkingDirectory( + __in BOOL fElvated, __in BURN_CACHE* pCache, __in_z LPCWSTR wzExecutableName, __in BURN_SECTION* pSection, diff --git a/src/burn/engine/core.cpp b/src/burn/engine/core.cpp index 2c1e4e020..dfb5107f9 100644 --- a/src/burn/engine/core.cpp +++ b/src/burn/engine/core.cpp @@ -182,7 +182,7 @@ extern "C" HRESULT CoreInitialize( if (BURN_MODE_NORMAL == pEngineState->internalCommand.mode || BURN_MODE_EMBEDDED == pEngineState->internalCommand.mode) { // Extract all UX payloads to working folder. - hr = UserExperienceEnsureWorkingFolder(&pEngineState->cache, &pEngineState->userExperience.sczTempDirectory); + hr = UserExperienceEnsureWorkingFolder(pEngineState->internalCommand.fInitiallyElevated, &pEngineState->cache, &pEngineState->userExperience.sczTempDirectory); ExitOnFailure(hr, "Failed to get unique temporary folder for bootstrapper application."); hr = PayloadExtractUXContainer(&pEngineState->userExperience.payloads, &containerContext, pEngineState->userExperience.sczTempDirectory); @@ -605,7 +605,7 @@ extern "C" HRESULT CoreElevate( // If the elevated companion pipe isn't created yet, let's make that happen. if (!pEngineState->sczBundleEngineWorkingPath) { - hr = CacheBundleToWorkingDirectory(&pEngineState->cache, pEngineState->registration.sczExecutableName, &pEngineState->section, &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."); } @@ -714,7 +714,7 @@ extern "C" HRESULT CoreApply( // Ensure the engine is cached to the working path. if (!pEngineState->sczBundleEngineWorkingPath) { - hr = CacheBundleToWorkingDirectory(&pEngineState->cache, pEngineState->registration.sczExecutableName, &pEngineState->section, &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."); } diff --git a/src/burn/engine/engine.cpp b/src/burn/engine/engine.cpp index b093ec9be..79e6aab49 100644 --- a/src/burn/engine/engine.cpp +++ b/src/burn/engine/engine.cpp @@ -525,7 +525,7 @@ static HRESULT RunUntrusted( } else { - hr = CacheBundleToCleanRoom(&pEngineState->cache, &pEngineState->section, &sczCachedCleanRoomBundlePath); + hr = CacheBundleToCleanRoom(pEngineState->internalCommand.fInitiallyElevated, &pEngineState->cache, &pEngineState->section, &sczCachedCleanRoomBundlePath); ExitOnFailure(hr, "Failed to cache to clean room."); wzCleanRoomBundlePath = sczCachedCleanRoomBundlePath; diff --git a/src/burn/engine/userexperience.cpp b/src/burn/engine/userexperience.cpp index 9766e1c3e..aaaebec60 100644 --- a/src/burn/engine/userexperience.cpp +++ b/src/burn/engine/userexperience.cpp @@ -169,6 +169,7 @@ extern "C" HRESULT UserExperienceUnload( } extern "C" HRESULT UserExperienceEnsureWorkingFolder( + __in BOOL fElevated, __in BURN_CACHE* pCache, __deref_out_z LPWSTR* psczUserExperienceWorkingFolder ) @@ -176,7 +177,7 @@ extern "C" HRESULT UserExperienceEnsureWorkingFolder( HRESULT hr = S_OK; LPWSTR sczWorkingFolder = NULL; - hr = CacheEnsureBaseWorkingFolder(pCache, &sczWorkingFolder); + hr = CacheEnsureBaseWorkingFolder(fElevated, pCache, &sczWorkingFolder); ExitOnFailure(hr, "Failed to create working folder."); hr = StrAllocFormatted(psczUserExperienceWorkingFolder, L"%ls%ls\\", sczWorkingFolder, L".ba"); diff --git a/src/burn/engine/userexperience.h b/src/burn/engine/userexperience.h index 358ceb03e..689aa71ee 100644 --- a/src/burn/engine/userexperience.h +++ b/src/burn/engine/userexperience.h @@ -64,6 +64,7 @@ HRESULT UserExperienceUnload( __in BOOL fReload ); HRESULT UserExperienceEnsureWorkingFolder( + __in BOOL fElevated, __in BURN_CACHE* pCache, __deref_out_z LPWSTR* psczUserExperienceWorkingFolder ); diff --git a/src/dtf/SfxCA/SfxUtil.cpp b/src/dtf/SfxCA/SfxUtil.cpp index 1bf2c5b28..32dc6e04a 100644 --- a/src/dtf/SfxCA/SfxUtil.cpp +++ b/src/dtf/SfxCA/SfxUtil.cpp @@ -93,7 +93,9 @@ bool DeleteDirectory(const wchar_t* szDir) StringCchCopy(szPath + cchDir + 1, cchPathBuf - (cchDir + 1), fd.cFileName); if ((fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0) { - if (wcscmp(fd.cFileName, L".") != 0 && wcscmp(fd.cFileName, L"..") != 0) + if (wcscmp(fd.cFileName, L".") != 0 + && wcscmp(fd.cFileName, L"..") != 0 + && ((fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0)) { DeleteDirectory(szPath); } @@ -162,38 +164,18 @@ bool ExtractToTempDirectory(__in MSIHANDLE hSession, __in HMODULE hModule, StringCchCopy(szTempDir, cchTempDirBuf, szModule); StringCchCat(szTempDir, cchTempDirBuf, L"-"); + BOOL fCreatedDirectory = FALSE; DWORD cchTempDir = (DWORD) wcslen(szTempDir); - for (int i = 0; DirectoryExists(szTempDir); i++) + for (int i = 0; i < 10000 && !fCreatedDirectory; i++) { swprintf_s(szTempDir + cchTempDir, cchTempDirBuf - cchTempDir, L"%d", i); + fCreatedDirectory = ::CreateDirectory(szTempDir, NULL); } - if (!CreateDirectory(szTempDir, NULL)) + if (!fCreatedDirectory) { - cchCopied = GetTempPath(cchTempDirBuf, szTempDir); - if (cchCopied == 0 || cchCopied >= cchTempDirBuf) - { - Log(hSession, L"Failed to get temp directory. Error code %d", GetLastError()); - return false; - } - - wchar_t* szModuleName = wcsrchr(szModule, L'\\'); - if (szModuleName == NULL) szModuleName = szModule; - else szModuleName = szModuleName + 1; - StringCchCat(szTempDir, cchTempDirBuf, szModuleName); - StringCchCat(szTempDir, cchTempDirBuf, L"-"); - - cchTempDir = (DWORD) wcslen(szTempDir); - for (int i = 0; DirectoryExists(szTempDir); i++) - { - swprintf_s(szTempDir + cchTempDir, cchTempDirBuf - cchTempDir, L"%d", i); - } - - if (!CreateDirectory(szTempDir, NULL)) - { - Log(hSession, L"Failed to create temp directory. Error code %d", GetLastError()); - return false; - } + Log(hSession, L"Failed to create temp directory. Error code %d", ::GetLastError()); + return false; } Log(hSession, L"Extracting custom action to temporary directory: %s\\", szTempDir); diff --git a/src/ext/NetFx/test/WixToolsetTest.Netfx/NetfxExtensionFixture.cs b/src/ext/NetFx/test/WixToolsetTest.Netfx/NetfxExtensionFixture.cs index 4f0a114d2..7379655a8 100644 --- a/src/ext/NetFx/test/WixToolsetTest.Netfx/NetfxExtensionFixture.cs +++ b/src/ext/NetFx/test/WixToolsetTest.Netfx/NetfxExtensionFixture.cs @@ -24,7 +24,7 @@ public void CanBuildUsingLatestDotNetCorePackages() var extensionResult = WixRunner.Execute(new[] { "extension", "add", - "WixToolset.Bal.wixext" + "WixToolset.Bal.wixext/4.0.0" }); var compileResult = WixRunner.Execute(new[] @@ -33,7 +33,7 @@ public void CanBuildUsingLatestDotNetCorePackages() Path.Combine(bundleSourceFolder, "BundleLatest.wxs"), Path.Combine(bundleSourceFolder, "NetCore3.1.12_x86.wxs"), Path.Combine(bundleSourceFolder, "NetCore3.1.12_x64.wxs"), - "-ext", "WixToolset.Bal.wixext", + "-ext", "WixToolset.Bal.wixext/4.0.0", "-ext", TestData.Get(@"WixToolset.Netfx.wixext.dll"), "-intermediateFolder", intermediateFolder, "-o", bundleFile, @@ -57,7 +57,7 @@ public void CanBuildUsingLatestDotNetCorePackages_X64() var extensionResult = WixRunner.Execute(new[] { "extension", "add", - "WixToolset.Bal.wixext" + "WixToolset.Bal.wixext/4.0.0" }); var compileResult = WixRunner.Execute(new[] @@ -65,7 +65,7 @@ public void CanBuildUsingLatestDotNetCorePackages_X64() "build", Path.Combine(bundleSourceFolder, "BundleLatest_x64.wxs"), Path.Combine(bundleSourceFolder, "NetCore3.1.12_x64.wxs"), - "-ext", "WixToolset.Bal.wixext", + "-ext", "WixToolset.Bal.wixext/4.0.0", "-ext", TestData.Get(@"WixToolset.Netfx.wixext.dll"), "-intermediateFolder", intermediateFolder, "-o", bundleFile, @@ -89,14 +89,14 @@ public void CanBuildUsingNetFx481Packages() var extensionResult = WixRunner.Execute(new[] { "extension", "add", - "WixToolset.Bal.wixext" + "WixToolset.Bal.wixext/4.0.0" }); var compileResult = WixRunner.Execute(new[] { "build", Path.Combine(bundleSourceFolder, "BundleLatest.wxs"), - "-ext", "WixToolset.Bal.wixext", + "-ext", "WixToolset.Bal.wixext/4.0.0", "-ext", TestData.Get(@"WixToolset.Netfx.wixext.dll"), "-intermediateFolder", intermediateFolder, "-o", bundleFile, diff --git a/src/wix/test/WixToolsetTest.CoreIntegration/ExtensionFixture.cs b/src/wix/test/WixToolsetTest.CoreIntegration/ExtensionFixture.cs index 3bb3cac7f..e2165b6fb 100644 --- a/src/wix/test/WixToolsetTest.CoreIntegration/ExtensionFixture.cs +++ b/src/wix/test/WixToolsetTest.CoreIntegration/ExtensionFixture.cs @@ -231,7 +231,7 @@ public void CannotBuildWithMissingVersionedExtension() } } - [Fact(Skip="Pending WiX5 release")] + [Fact(Skip = "Blocked by issue #8033.")] public void CanManipulateExtensionCache() { var currentFolder = Environment.CurrentDirectory;