Skip to content

Commit

Permalink
Do not follow junctions when recursively deleting directories
Browse files Browse the repository at this point in the history 
This will prevent elevated processes from accidentally following a junction
from a user-writable directory to a per-machine directory and erroneously
deleting the per-machine contents.

Revert the mergemod.cub to pre-ARM64 builds to fix MSM validation

When introducing ARM64 support into the Windows Installer, Microsoft broke
the ICE CUBe files in various ways. To minimize the impact of the breakage
move the mergemod.cub file back to pre-ARM64 support. Validating ARM64 Merge
Modules is not likely to work but that option is better than the regression.

Fixes wixtoolset/issues#8065

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:

- The RemoveFoldersEx custom action (which doesn't actually do deletions
but would instruct elevated MSI to delete on your behalf).
- 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.
  • Loading branch information
@robmen @nirbar
robmen authored and nirbar committed Mar 25, 2024
1 parent 8b72b37 commit 99e754b
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 37 deletions.
36 changes: 9 additions & 27 deletions src/DTF/Tools/SfxCA/SfxUtil.cpp
View file
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
53 changes: 43 additions & 10 deletions src/burn/engine/cache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,16 @@ static BOOL vfInitializedCache = FALSE;
static BOOL vfRunningFromCache = FALSE;
static LPWSTR vsczSourceProcessPath = NULL;
static LPWSTR vsczWorkingFolder = NULL;
static BOOL vfWorkingFolderElevated = FALSE;
static LPWSTR vsczDefaultUserPackageCache = NULL;
static LPWSTR vsczDefaultMachinePackageCache = NULL;
static LPWSTR vsczCurrentMachinePackageCache = NULL;
static LPWSTR vsczRegistrationId = NULL;

static HRESULT CalculateWorkingFolder(
__in_z LPCWSTR wzBundleId,
__deref_out_z LPWSTR* psczWorkingFolder
__deref_out_z LPWSTR* psczWorkingFolder,
__out_opt BOOL* pfWorkingFolderElevated
);
static HRESULT GetLastUsedSourceFolder(
__in BURN_VARIABLES* pVariables,
Expand Down Expand Up @@ -217,11 +219,29 @@ extern "C" HRESULT CacheEnsureWorkingFolder(
{
HRESULT hr = S_OK;
LPWSTR sczWorkingFolder = NULL;
BOOL fElevatedWorkingFolder = FALSE;
PSECURITY_DESCRIPTOR psd = NULL;
LPSECURITY_ATTRIBUTES pWorkingFolderAcl = NULL;

hr = CalculateWorkingFolder(wzBundleId, &sczWorkingFolder);
hr = CalculateWorkingFolder(wzBundleId, &sczWorkingFolder, &fElevatedWorkingFolder);
ExitOnFailure(hr, "Failed to calculate working folder to ensure it exists.");

hr = DirEnsureExists(sczWorkingFolder, NULL);
// If elevated, allocate the pWorkingFolderAcl to protect the working folder to only Admins and SYSTEM.
if (fElevatedWorkingFolder)
{
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;
}

hr = DirEnsureExists(sczWorkingFolder, pWorkingFolderAcl);
ExitOnFailure(hr, "Failed create working folder.");

// Best effort to ensure our working folder is not encrypted.
Expand All @@ -234,6 +254,11 @@ extern "C" HRESULT CacheEnsureWorkingFolder(
}

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

return hr;
Expand All @@ -259,7 +284,7 @@ extern "C" HRESULT CacheCalculateBundleWorkingPath(
}
else // Otherwise, use the real working folder.
{
hr = CalculateWorkingFolder(wzBundleId, &sczWorkingFolder);
hr = CalculateWorkingFolder(wzBundleId, &sczWorkingFolder, NULL);
ExitOnFailure(hr, "Failed to get working folder for bundle.");

hr = StrAllocFormatted(psczWorkingPath, L"%ls%ls\\%ls", sczWorkingFolder, BUNDLE_WORKING_FOLDER_NAME, wzExecutableName);
Expand All @@ -280,7 +305,7 @@ extern "C" HRESULT CacheCalculateBundleLayoutWorkingPath(
HRESULT hr = S_OK;
LPWSTR sczWorkingFolder = NULL;

hr = CalculateWorkingFolder(wzBundleId, psczWorkingPath);
hr = CalculateWorkingFolder(wzBundleId, psczWorkingPath, NULL);
ExitOnFailure(hr, "Failed to get working folder for bundle layout.");

hr = StrAllocConcat(psczWorkingPath, wzBundleId, 0);
Expand All @@ -300,7 +325,7 @@ extern "C" HRESULT CacheCalculatePayloadWorkingPath(
{
HRESULT hr = S_OK;

hr = CalculateWorkingFolder(wzBundleId, psczWorkingPath);
hr = CalculateWorkingFolder(wzBundleId, psczWorkingPath, NULL);
ExitOnFailure(hr, "Failed to get working folder for payload.");

hr = StrAllocConcat(psczWorkingPath, pPayload->sczKey, 0);
Expand All @@ -318,7 +343,7 @@ extern "C" HRESULT CacheCalculateContainerWorkingPath(
{
HRESULT hr = S_OK;

hr = CalculateWorkingFolder(wzBundleId, psczWorkingPath);
hr = CalculateWorkingFolder(wzBundleId, psczWorkingPath, NULL);
ExitOnFailure(hr, "Failed to get working folder for container.");

hr = StrAllocConcat(psczWorkingPath, pContainer->sczHash, 0);
Expand Down Expand Up @@ -943,7 +968,7 @@ extern "C" HRESULT CacheRemoveWorkingFolder(

if (vfInitializedCache)
{
hr = CalculateWorkingFolder(wzBundleId, &sczWorkingFolder);
hr = CalculateWorkingFolder(wzBundleId, &sczWorkingFolder, NULL);
ExitOnFailure(hr, "Failed to calculate the working folder to remove it.");

// Try to clean out everything in the working folder.
Expand Down Expand Up @@ -1057,7 +1082,7 @@ extern "C" void CacheCleanup(

if (!fPerMachine)
{
hr = CalculateWorkingFolder(wzBundleId, &sczFolder);
hr = CalculateWorkingFolder(wzBundleId, &sczFolder, NULL);
if (SUCCEEDED(hr))
{
hr = PathConcat(sczFolder, L"*.*", &sczFiles);
Expand Down Expand Up @@ -1122,7 +1147,8 @@ extern "C" void CacheUninitialize()

static HRESULT CalculateWorkingFolder(
__in_z LPCWSTR /*wzBundleId*/,
__deref_out_z LPWSTR* psczWorkingFolder
__deref_out_z LPWSTR* psczWorkingFolder,
__out_opt BOOL* pfWorkingFolderElevated
)
{
HRESULT hr = S_OK;
Expand Down Expand Up @@ -1166,11 +1192,18 @@ static HRESULT CalculateWorkingFolder(

hr = StrAllocFormatted(&vsczWorkingFolder, L"%ls%ls\\", wzTempPath, wzGuid);
ExitOnFailure(hr, "Failed to append bundle id on to temp path for working folder.");

vfWorkingFolderElevated = fElevated;
}

hr = StrAllocString(psczWorkingFolder, vsczWorkingFolder, 0);
ExitOnFailure(hr, "Failed to copy working folder path.");

if (pfWorkingFolderElevated)
{
*pfWorkingFolderElevated = vfWorkingFolderElevated;
}

LExit:
return hr;
}
Expand Down
Binary file modified src/tools/light/mergemod.cub
View file
Binary file not shown.

0 comments on commit 99e754b

Please sign in to comment.