Skip to content

Commit

Permalink
Don't follow junctions when recursing directories.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
robmen authored and nirbar committed Mar 25, 2024
1 parent b6dea14 commit 0410df9
Show file tree
Hide file tree
Showing 9 changed files with 57 additions and 43 deletions.
35 changes: 31 additions & 4 deletions src/burn/engine/cache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
)
Expand All @@ -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<LPSECURITY_ATTRIBUTES>(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;
Expand All @@ -385,6 +404,11 @@ extern "C" HRESULT CacheEnsureBaseWorkingFolder(
}

LExit:
ReleaseMem(pWorkingFolderAcl);
if (psd)
{
::LocalFree(psd);
}
ReleaseStr(sczPotential);

return hr;
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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,
Expand All @@ -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.");
}

Expand Down Expand Up @@ -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,
Expand All @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions src/burn/engine/cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions src/burn/engine/core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.");
}

Expand Down Expand Up @@ -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.");
}

Expand Down
2 changes: 1 addition & 1 deletion src/burn/engine/engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/burn/engine/userexperience.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -169,14 +169,15 @@ extern "C" HRESULT UserExperienceUnload(
}

extern "C" HRESULT UserExperienceEnsureWorkingFolder(
__in BOOL fElevated,
__in BURN_CACHE* pCache,
__deref_out_z LPWSTR* psczUserExperienceWorkingFolder
)
{
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");
Expand Down
1 change: 1 addition & 0 deletions src/burn/engine/userexperience.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ HRESULT UserExperienceUnload(
__in BOOL fReload
);
HRESULT UserExperienceEnsureWorkingFolder(
__in BOOL fElevated,
__in BURN_CACHE* pCache,
__deref_out_z LPWSTR* psczUserExperienceWorkingFolder
);
Expand Down
36 changes: 9 additions & 27 deletions src/dtf/SfxCA/SfxUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions src/ext/NetFx/test/WixToolsetTest.Netfx/NetfxExtensionFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
Expand All @@ -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,
Expand All @@ -57,15 +57,15 @@ 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[]
{
"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,
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 0410df9

Please sign in to comment.