From b5553689ed1b1bd32f854654f56935c039a9b13b Mon Sep 17 00:00:00 2001 From: Sean Hall Date: Sun, 1 Nov 2020 14:16:32 -0600 Subject: WIXFEAT:6258 - Format variables when evaluating condition. --- src/engine/condition.cpp | 11 ++- src/engine/variable.cpp | 114 ++++++++++++++++++++---------- src/engine/variable.h | 3 +- src/test/BurnUnitTest/VariableHelpers.cpp | 4 +- src/test/BurnUnitTest/VariableHelpers.h | 2 +- src/test/BurnUnitTest/VariableTest.cpp | 33 ++++++--- 6 files changed, 112 insertions(+), 55 deletions(-) (limited to 'src') diff --git a/src/engine/condition.cpp b/src/engine/condition.cpp index 224eb0da..d6038b34 100644 --- a/src/engine/condition.cpp +++ b/src/engine/condition.cpp @@ -432,6 +432,7 @@ static HRESULT ParseOperand( ) { HRESULT hr = S_OK; + LPWSTR sczFormatted = NULL; // Symbols don't encrypt their value, so can access the value directly. switch (pContext->NextSymbol.Type) @@ -451,9 +452,11 @@ static HRESULT ParseOperand( if (BURN_VARIANT_TYPE_FORMATTED == pOperand->Value.Type) { - // TODO: actually format the value? - hr = BVariantChangeType(&pOperand->Value, BURN_VARIANT_TYPE_STRING); - ExitOnRootFailure(hr, "Failed to change variable '%ls' type for condition '%ls'", pContext->NextSymbol.Value.sczValue, pContext->wzCondition); + hr = VariableGetFormatted(pContext->pVariables, pContext->NextSymbol.Value.sczValue, &sczFormatted, &pOperand->fHidden); + ExitOnRootFailure(hr, "Failed to format variable '%ls' for condition '%ls'", pContext->NextSymbol.Value.sczValue, pContext->wzCondition); + + hr = BVariantSetString(&pOperand->Value, sczFormatted, 0, FALSE); + ExitOnRootFailure(hr, "Failed to store formatted value for variable '%ls' for condition '%ls'", pContext->NextSymbol.Value.sczValue, pContext->wzCondition); } break; @@ -477,6 +480,8 @@ static HRESULT ParseOperand( ExitOnFailure(hr, "Failed to read next symbol."); LExit: + StrSecureZeroFreeString(sczFormatted); + return hr; } diff --git a/src/engine/variable.cpp b/src/engine/variable.cpp index 3425b92c..8b6b318b 100644 --- a/src/engine/variable.cpp +++ b/src/engine/variable.cpp @@ -54,7 +54,14 @@ static HRESULT FormatString( __in_z LPCWSTR wzIn, __out_z_opt LPWSTR* psczOut, __out_opt DWORD* pcchOut, - __in BOOL fObfuscateHiddenVariables + __in BOOL fObfuscateHiddenVariables, + __out BOOL* pfContainsHiddenVariable + ); +static HRESULT GetFormatted( + __in BURN_VARIABLES* pVariables, + __in_z LPCWSTR wzVariable, + __out_z LPWSTR* psczValue, + __out BOOL* pfContainsHiddenVariable ); static HRESULT AddBuiltInVariable( __in BURN_VARIABLES* pVariables, @@ -629,43 +636,18 @@ LExit: extern "C" HRESULT VariableGetFormatted( __in BURN_VARIABLES* pVariables, __in_z LPCWSTR wzVariable, - __out_z LPWSTR* psczValue + __out_z LPWSTR* psczValue, + __out BOOL* pfContainsHiddenVariable ) { HRESULT hr = S_OK; - BURN_VARIABLE* pVariable = NULL; - LPWSTR scz = NULL; - - ::EnterCriticalSection(&pVariables->csAccess); - hr = GetVariable(pVariables, wzVariable, &pVariable); - if (SUCCEEDED(hr) && BURN_VARIANT_TYPE_NONE == pVariable->Value.Type) + if (pfContainsHiddenVariable) { - ExitFunction1(hr = E_NOTFOUND); - } - else if (E_NOTFOUND == hr) - { - ExitFunction(); + *pfContainsHiddenVariable = FALSE; } - ExitOnFailure(hr, "Failed to get variable: %ls", wzVariable); - if (BURN_VARIANT_TYPE_FORMATTED == pVariable->Value.Type) - { - hr = BVariantGetString(&pVariable->Value, &scz); - ExitOnFailure(hr, "Failed to get unformatted string."); - - hr = VariableFormatString(pVariables, scz, psczValue, NULL); - ExitOnFailure(hr, "Failed to format value '%ls' of variable: %ls", pVariable->fHidden ? L"*****" : pVariable->Value.sczValue, wzVariable); - } - else - { - hr = BVariantGetString(&pVariable->Value, psczValue); - ExitOnFailure(hr, "Failed to get value as string for variable: %ls", wzVariable); - } - -LExit: - ::LeaveCriticalSection(&pVariables->csAccess); - StrSecureZeroFreeString(scz); + hr = GetFormatted(pVariables, wzVariable, psczValue, pfContainsHiddenVariable); return hr; } @@ -736,7 +718,7 @@ extern "C" HRESULT VariableFormatString( __out_opt DWORD* pcchOut ) { - return FormatString(pVariables, wzIn, psczOut, pcchOut, FALSE); + return FormatString(pVariables, wzIn, psczOut, pcchOut, FALSE, NULL); } extern "C" HRESULT VariableFormatStringObfuscated( @@ -746,7 +728,7 @@ extern "C" HRESULT VariableFormatStringObfuscated( __out_opt DWORD* pcchOut ) { - return FormatString(pVariables, wzIn, psczOut, pcchOut, TRUE); + return FormatString(pVariables, wzIn, psczOut, pcchOut, TRUE, NULL); } extern "C" HRESULT VariableEscapeString( @@ -1116,7 +1098,8 @@ static HRESULT FormatString( __in_z LPCWSTR wzIn, __out_z_opt LPWSTR* psczOut, __out_opt DWORD* pcchOut, - __in BOOL fObfuscateHiddenVariables + __in BOOL fObfuscateHiddenVariables, + __out BOOL* pfContainsHiddenVariable ) { HRESULT hr = S_OK; @@ -1204,20 +1187,22 @@ static HRESULT FormatString( } else { - if (fObfuscateHiddenVariables) + hr = VariableIsHidden(pVariables, scz, &fHidden); + ExitOnFailure(hr, "Failed to determine variable visibility: '%ls'.", scz); + + if (pfContainsHiddenVariable) { - hr = VariableIsHidden(pVariables, scz, &fHidden); - ExitOnFailure(hr, "Failed to determine variable visibility: '%ls'.", scz); + *pfContainsHiddenVariable |= fHidden; } - if (fHidden) + if (fObfuscateHiddenVariables && fHidden) { hr = StrAllocString(&rgVariables[cVariables], L"*****", 0); } else { // get formatted variable value - hr = VariableGetFormatted(pVariables, scz, &rgVariables[cVariables]); + hr = GetFormatted(pVariables, scz, &rgVariables[cVariables], pfContainsHiddenVariable); if (E_NOTFOUND == hr) // variable not found { hr = StrAllocStringSecure(&rgVariables[cVariables], L"", 0); @@ -1327,6 +1312,57 @@ LExit: return hr; } +// The contents of psczOut may be sensitive, should keep encrypted and SecureZeroFree. +static HRESULT GetFormatted( + __in BURN_VARIABLES* pVariables, + __in_z LPCWSTR wzVariable, + __out_z LPWSTR* psczValue, + __out BOOL* pfContainsHiddenVariable + ) +{ + HRESULT hr = S_OK; + BURN_VARIABLE* pVariable = NULL; + LPWSTR scz = NULL; + + ::EnterCriticalSection(&pVariables->csAccess); + + hr = GetVariable(pVariables, wzVariable, &pVariable); + if (SUCCEEDED(hr) && BURN_VARIANT_TYPE_NONE == pVariable->Value.Type) + { + ExitFunction1(hr = E_NOTFOUND); + } + else if (E_NOTFOUND == hr) + { + ExitFunction(); + } + ExitOnFailure(hr, "Failed to get variable: %ls", wzVariable); + + if (pfContainsHiddenVariable) + { + *pfContainsHiddenVariable |= pVariable->fHidden; + } + + if (BURN_VARIANT_TYPE_FORMATTED == pVariable->Value.Type) + { + hr = BVariantGetString(&pVariable->Value, &scz); + ExitOnFailure(hr, "Failed to get unformatted string."); + + hr = FormatString(pVariables, scz, psczValue, NULL, FALSE, pfContainsHiddenVariable); + ExitOnFailure(hr, "Failed to format value '%ls' of variable: %ls", pVariable->fHidden ? L"*****" : pVariable->Value.sczValue, wzVariable); + } + else + { + hr = BVariantGetString(&pVariable->Value, psczValue); + ExitOnFailure(hr, "Failed to get value as string for variable: %ls", wzVariable); + } + +LExit: + ::LeaveCriticalSection(&pVariables->csAccess); + StrSecureZeroFreeString(scz); + + return hr; +} + static HRESULT AddBuiltInVariable( __in BURN_VARIABLES* pVariables, __in LPCWSTR wzVariable, diff --git a/src/engine/variable.h b/src/engine/variable.h index 6437c32f..713fe6e3 100644 --- a/src/engine/variable.h +++ b/src/engine/variable.h @@ -95,7 +95,8 @@ HRESULT VariableGetVariant( HRESULT VariableGetFormatted( __in BURN_VARIABLES* pVariables, __in_z LPCWSTR wzVariable, - __out_z LPWSTR* psczValue + __out_z LPWSTR* psczValue, + __out BOOL* pfContainsHiddenVariable ); HRESULT VariableSetNumeric( __in BURN_VARIABLES* pVariables, diff --git a/src/test/BurnUnitTest/VariableHelpers.cpp b/src/test/BurnUnitTest/VariableHelpers.cpp index 99ba492a..40f958f8 100644 --- a/src/test/BurnUnitTest/VariableHelpers.cpp +++ b/src/test/BurnUnitTest/VariableHelpers.cpp @@ -98,13 +98,13 @@ namespace Bootstrapper } } - String^ VariableGetFormattedHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable) + String^ VariableGetFormattedHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable, BOOL* pfContainsHiddenVariable) { HRESULT hr = S_OK; LPWSTR scz = NULL; try { - hr = VariableGetFormatted(pVariables, wzVariable, &scz); + hr = VariableGetFormatted(pVariables, wzVariable, &scz, pfContainsHiddenVariable); TestThrowOnFailure1(hr, L"Failed to get formatted: %s", wzVariable); return gcnew String(scz); diff --git a/src/test/BurnUnitTest/VariableHelpers.h b/src/test/BurnUnitTest/VariableHelpers.h index 96122219..d460c60f 100644 --- a/src/test/BurnUnitTest/VariableHelpers.h +++ b/src/test/BurnUnitTest/VariableHelpers.h @@ -20,7 +20,7 @@ void VariableSetVersionHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable, LP System::String^ VariableGetStringHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable); __int64 VariableGetNumericHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable); System::String^ VariableGetVersionHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable); -System::String^ VariableGetFormattedHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable); +System::String^ VariableGetFormattedHelper(BURN_VARIABLES* pVariables, LPCWSTR wzVariable, BOOL* pfContainsHiddenVariable); System::String^ VariableFormatStringHelper(BURN_VARIABLES* pVariables, LPCWSTR wzIn); System::String^ VariableEscapeStringHelper(LPCWSTR wzIn); bool EvaluateConditionHelper(BURN_VARIABLES* pVariables, LPCWSTR wzCondition); diff --git a/src/test/BurnUnitTest/VariableTest.cpp b/src/test/BurnUnitTest/VariableTest.cpp index f5511199..676c134e 100644 --- a/src/test/BurnUnitTest/VariableTest.cpp +++ b/src/test/BurnUnitTest/VariableTest.cpp @@ -80,6 +80,7 @@ namespace Bootstrapper HRESULT hr = S_OK; IXMLDOMElement* pixeBundle = NULL; BURN_VARIABLES variables = { }; + BOOL fContainsHiddenData = FALSE; try { LPCWSTR wzDocument = @@ -90,6 +91,7 @@ namespace Bootstrapper L"