Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIXFEAT:4299 - Improve security for hidden burn variables #6

Merged
merged 6 commits into from May 22, 2014
Merged

WIXFEAT:4299 - Improve security for hidden burn variables #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8b367b
Add security to Hidden Burn Variables by calling SecureZeroMemory on …
rseanhall Mar 24, 2014
a7ea505
Make StringVariables use pointers so that the IBootstrapperEngineSecu…
rseanhall Mar 25, 2014
c35980f
Consolidate the StrUtil Secure methods so the code isn't duplicated.
rseanhall Mar 26, 2014
4e1c424
Formatting.
rseanhall Mar 26, 2014
8d860c4
Merge branch 'develop' of https://github.com/wixtoolset/wix3 into wix…
rseanhall Apr 10, 2014
f9d3f3f
Fix bugs.
rseanhall Apr 10, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/burn/engine/EngineForApplication.cpp
View file
Open in desktop
Expand Up @@ -87,6 +87,7 @@ class CEngineForApplication : public IBootstrapperEngine, public IMarshal
return hr;
}

// The contents of pllValue may be sensitive, if variable is hidden should keep value encrypted and SecureZeroMemory.
virtual STDMETHODIMP GetVariableNumeric(
__in_z LPCWSTR wzVariable,
__out LONGLONG* pllValue
Expand All @@ -106,6 +107,7 @@ class CEngineForApplication : public IBootstrapperEngine, public IMarshal
return hr;
}

// The contents of wzValue may be sensitive, if variable is hidden should keep value encrypted and SecureZeroFree.
virtual STDMETHODIMP GetVariableString(
__in_z LPCWSTR wzVariable,
__out_ecount_opt(*pcchValue) LPWSTR wzValue,
Expand Down Expand Up @@ -146,10 +148,11 @@ class CEngineForApplication : public IBootstrapperEngine, public IMarshal
hr = E_INVALIDARG;
}

ReleaseStr(sczValue);
StrSecureZeroFreeString(sczValue);
return hr;
}

// The contents of wzValue may be sensitive, if variable is hidden should keep value encrypted and SecureZeroMemory.
virtual STDMETHODIMP GetVariableVersion(
__in_z LPCWSTR wzVariable,
__out DWORD64* pqwValue
Expand All @@ -169,6 +172,7 @@ class CEngineForApplication : public IBootstrapperEngine, public IMarshal
return hr;
}

// The contents of wzOut may be sensitive, should keep encrypted and SecureZeroFree.
virtual STDMETHODIMP FormatString(
__in_z LPCWSTR wzIn,
__out_ecount_opt(*pcchOut) LPWSTR wzOut,
Expand Down Expand Up @@ -208,7 +212,7 @@ class CEngineForApplication : public IBootstrapperEngine, public IMarshal
hr = E_INVALIDARG;
}

ReleaseStr(sczValue);
StrSecureZeroFreeString(sczValue);
return hr;
}

Expand Down Expand Up @@ -249,7 +253,7 @@ class CEngineForApplication : public IBootstrapperEngine, public IMarshal
hr = E_INVALIDARG;
}

ReleaseStr(sczValue);
StrSecureZeroFreeString(sczValue);
return hr;
}

Expand Down
100 changes: 84 additions & 16 deletions src/burn/engine/condition.cpp
View file
Open in desktop
Expand Up @@ -388,19 +388,37 @@ static HRESULT ParseTerm(
}
else
{
LONGLONG llValue = 0;
LPWSTR sczValue = NULL;
DWORD64 qwValue = 0;
switch (firstValue.Type)
{
case BURN_VARIANT_TYPE_NONE:
*pf = FALSE;
break;
case BURN_VARIANT_TYPE_STRING:
*pf = firstValue.sczValue && *firstValue.sczValue;
hr = BVariantGetString(&firstValue, &sczValue);
if (SUCCEEDED(hr))
{
*pf = sczValue && *sczValue;
}
StrSecureZeroFreeString(sczValue);
break;
case BURN_VARIANT_TYPE_NUMERIC:
*pf = 0 != firstValue.llValue;
hr = BVariantGetNumeric(&firstValue, &llValue);
if (SUCCEEDED(hr))
{
*pf = 0 != llValue;
}
SecureZeroMemory(&llValue, sizeof(llValue));
break;
case BURN_VARIANT_TYPE_VERSION:
*pf = 0 != firstValue.qwValue;
hr = BVariantGetVersion(&firstValue, &qwValue);
if (SUCCEEDED(hr))
{
*pf = 0 != qwValue;
}
SecureZeroMemory(&llValue, sizeof(qwValue));
break;
default:
ExitFunction1(hr = E_UNEXPECTED);
Expand All @@ -420,6 +438,7 @@ static HRESULT ParseValue(
{
HRESULT hr = S_OK;

// Symbols don't encrypt their value, so can access the value directly.
switch (pContext->NextSymbol.Type)
{
case BURN_SYMBOL_TYPE_IDENTIFIER:
Expand Down Expand Up @@ -699,6 +718,7 @@ static HRESULT NextSymbol(
}
}

// Symbols don't encrypt their value, so can access the value directly.
hr = FileVersionFromStringEx(&pContext->wzRead[1], n - 1, &pContext->NextSymbol.Value.qwValue);
if (FAILED(hr))
{
Expand Down Expand Up @@ -768,72 +788,112 @@ static HRESULT CompareValues(
)
{
HRESULT hr = S_OK;
LONGLONG ll = 0;
DWORD64 qw = 0;
LONGLONG llLeft = 0;
DWORD64 qwLeft = 0;
LPWSTR sczLeft = NULL;
LONGLONG llRight = 0;
DWORD64 qwRight = 0;
LPWSTR sczRight = NULL;

// get values to compare based on type
if (BURN_VARIANT_TYPE_STRING == leftOperand.Type && BURN_VARIANT_TYPE_STRING == rightOperand.Type)
{
hr = CompareStringValues(comparison, leftOperand.sczValue, rightOperand.sczValue, pfResult);
hr = BVariantGetString(&leftOperand, &sczLeft);
ExitOnFailure(hr, "Failed to get the left string");
hr = BVariantGetString(&rightOperand, &sczRight);
ExitOnFailure(hr, "Failed to get the right string");
hr = CompareStringValues(comparison, sczLeft, sczRight, pfResult);
}
else if (BURN_VARIANT_TYPE_NUMERIC == leftOperand.Type && BURN_VARIANT_TYPE_NUMERIC == rightOperand.Type)
{
hr = CompareIntegerValues(comparison, leftOperand.llValue, rightOperand.llValue, pfResult);
hr = BVariantGetNumeric(&leftOperand, &llLeft);
ExitOnFailure(hr, "Failed to get the left numeric");
hr = BVariantGetNumeric(&rightOperand, &llRight);
ExitOnFailure(hr, "Failed to get the right numeric");
hr = CompareIntegerValues(comparison, llLeft, llRight, pfResult);
}
else if (BURN_VARIANT_TYPE_VERSION == leftOperand.Type && BURN_VARIANT_TYPE_VERSION == rightOperand.Type)
{
hr = CompareVersionValues(comparison, leftOperand.qwValue, rightOperand.qwValue, pfResult);
hr = BVariantGetVersion(&leftOperand, &qwLeft);
ExitOnFailure(hr, "Failed to get the left version");
hr = BVariantGetVersion(&rightOperand, &qwRight);
ExitOnFailure(hr, "Failed to get the right version");
hr = CompareVersionValues(comparison, qwLeft, qwRight, pfResult);
}
else if (BURN_VARIANT_TYPE_VERSION == leftOperand.Type && BURN_VARIANT_TYPE_STRING == rightOperand.Type)
{
hr = BVariantGetVersion(&rightOperand, &qw);
hr = BVariantGetVersion(&leftOperand, &qwLeft);
ExitOnFailure(hr, "Failed to get the left version");
hr = BVariantGetVersion(&rightOperand, &qwRight);
if (FAILED(hr))
{
if (DISP_E_TYPEMISMATCH != hr)
{
ExitOnFailure(hr, "Failed to get the right version");
}
*pfResult = (BURN_SYMBOL_TYPE_NE == comparison);
hr = S_OK;
}
else
{
hr = CompareVersionValues(comparison, leftOperand.qwValue, qw, pfResult);
hr = CompareVersionValues(comparison, qwLeft, qwRight, pfResult);
}
}
else if (BURN_VARIANT_TYPE_STRING == leftOperand.Type && BURN_VARIANT_TYPE_VERSION == rightOperand.Type)
{
hr = BVariantGetVersion(&leftOperand, &qw);
hr = BVariantGetVersion(&rightOperand, &qwRight);
ExitOnFailure(hr, "Failed to get the right version");
hr = BVariantGetVersion(&leftOperand, &qwLeft);
if (FAILED(hr))
{
if (DISP_E_TYPEMISMATCH != hr)
{
ExitOnFailure(hr, "Failed to get the left version");
}
*pfResult = (BURN_SYMBOL_TYPE_NE == comparison);
hr = S_OK;
}
else
{
hr = CompareVersionValues(comparison, qw, rightOperand.qwValue, pfResult);
hr = CompareVersionValues(comparison, qwLeft, qwRight, pfResult);
}
}
else if (BURN_VARIANT_TYPE_NUMERIC == leftOperand.Type && BURN_VARIANT_TYPE_STRING == rightOperand.Type)
{
hr = StrStringToInt64(rightOperand.sczValue, 0, &ll);
hr = BVariantGetNumeric(&leftOperand, &llLeft);
ExitOnFailure(hr, "Failed to get the left numeric");
hr = BVariantGetNumeric(&rightOperand, &llRight);
if (FAILED(hr))
{
if (DISP_E_TYPEMISMATCH != hr)
{
ExitOnFailure(hr, "Failed to get the right numeric");
}
*pfResult = (BURN_SYMBOL_TYPE_NE == comparison);
hr = S_OK;
}
else
{
hr = CompareIntegerValues(comparison, leftOperand.llValue, ll, pfResult);
hr = CompareIntegerValues(comparison, llLeft, llRight, pfResult);
}
}
else if (BURN_VARIANT_TYPE_STRING == leftOperand.Type && BURN_VARIANT_TYPE_NUMERIC == rightOperand.Type)
{
hr = StrStringToInt64(leftOperand.sczValue, 0, &ll);
hr = BVariantGetNumeric(&rightOperand, &llRight);
ExitOnFailure(hr, "Failed to get the right numeric");
hr = BVariantGetNumeric(&leftOperand, &llLeft);
if (FAILED(hr))
{
if (DISP_E_TYPEMISMATCH != hr)
{
ExitOnFailure(hr, "Failed to get the left numeric");
}
*pfResult = (BURN_SYMBOL_TYPE_NE == comparison);
hr = S_OK;
}
else
{
hr = CompareIntegerValues(comparison, ll, rightOperand.llValue, pfResult);
hr = CompareIntegerValues(comparison, llLeft, llRight, pfResult);
}
}
else
Expand All @@ -842,6 +902,14 @@ static HRESULT CompareValues(
*pfResult = (BURN_SYMBOL_TYPE_NE == comparison);
}

LExit:
SecureZeroMemory(&qwLeft, sizeof(DWORD64));
SecureZeroMemory(&llLeft, sizeof(LONGLONG));
StrSecureZeroFreeString(sczLeft);
SecureZeroMemory(&qwRight, sizeof(DWORD64));
SecureZeroMemory(&llRight, sizeof(LONGLONG));
StrSecureZeroFreeString(sczRight);

return hr;
}

Expand Down
4 changes: 2 additions & 2 deletions src/burn/engine/embedded.cpp
View file
Open in desktop
Expand Up @@ -81,7 +81,7 @@ extern "C" HRESULT EmbeddedRunBundle(
hr = PipeCreatePipes(&connection, FALSE, &hCreatedPipesEvent);
ExitOnFailure(hr, "Failed to create embedded pipe.");

hr = StrAllocFormatted(&sczCommand, L"%ls -%ls %ls %ls %u", wzArguments, BURN_COMMANDLINE_SWITCH_EMBEDDED, connection.sczName, connection.sczSecret, dwCurrentProcessId);
hr = StrAllocateFormatted(&sczCommand, TRUE, L"%ls -%ls %ls %ls %u", wzArguments, BURN_COMMANDLINE_SWITCH_EMBEDDED, connection.sczName, connection.sczSecret, dwCurrentProcessId);
ExitOnFailure(hr, "Failed to allocate embedded command.");

if (!::CreateProcessW(wzExecutablePath, sczCommand, NULL, NULL, FALSE, CREATE_NO_WINDOW, NULL, NULL, &si, &pi))
Expand All @@ -107,7 +107,7 @@ extern "C" HRESULT EmbeddedRunBundle(
ReleaseHandle(pi.hThread);
ReleaseHandle(pi.hProcess);

ReleaseStr(sczCommand);
StrSecureZeroFreeString(sczCommand);
ReleaseHandle(hCreatedPipesEvent);
PipeConnectionUninitialize(&connection);

Expand Down
10 changes: 10 additions & 0 deletions src/burn/engine/engine.cpp
View file
Open in desktop
Expand Up @@ -70,6 +70,7 @@ extern "C" HRESULT EngineRun(
HRESULT hr = S_OK;
BOOL fComInitialized = FALSE;
BOOL fLogInitialized = FALSE;
BOOL fCrypInitialized = FALSE;
BOOL fRegInitialized = FALSE;
BOOL fWiuInitialized = FALSE;
BOOL fXmlInitialized = FALSE;
Expand Down Expand Up @@ -104,6 +105,10 @@ extern "C" HRESULT EngineRun(
LogInitialize(::GetModuleHandleW(NULL));
fLogInitialized = TRUE;

hr = CrypInitialize();
ExitOnFailure(hr, "Failed to initialize Cryputil.");
fCrypInitialized = TRUE;

hr = RegInitialize();
ExitOnFailure(hr, "Failed to initialize Regutil.");
fRegInitialized = TRUE;
Expand Down Expand Up @@ -206,6 +211,11 @@ extern "C" HRESULT EngineRun(
RegUninitialize();
}

if (fCrypInitialized)
{
CrypUninitialize();
}

if (fComInitialized)
{
::CoUninitialize();
Expand Down
8 changes: 4 additions & 4 deletions src/burn/engine/exeengine.cpp
View file
Open in desktop
Expand Up @@ -457,7 +457,7 @@ extern "C" HRESULT ExeEngineExecutePackage(
hr = VariableFormatString(pVariables, wzArguments, &sczArgumentsFormatted, NULL);
ExitOnFailure(hr, "Failed to format argument string.");

hr = StrAllocFormatted(&sczCommand, L"\"%ls\" %s", sczExecutablePath, sczArgumentsFormatted);
hr = StrAllocateFormatted(&sczCommand, TRUE, L"\"%ls\" %s", sczExecutablePath, sczArgumentsFormatted);
ExitOnFailure(hr, "Failed to create executable command.");

hr = VariableFormatStringObfuscated(pVariables, wzArguments, &sczArgumentsObfuscated, NULL);
Expand All @@ -477,7 +477,7 @@ extern "C" HRESULT ExeEngineExecutePackage(
// Add the list of dependencies to ignore, if any, to the burn command line.
if (pExecuteAction->exePackage.sczIgnoreDependencies && BURN_EXE_PROTOCOL_TYPE_BURN == pExecuteAction->exePackage.pPackage->Exe.protocol)
{
hr = StrAllocFormatted(&sczCommand, L"%ls -%ls=%ls", sczCommand, BURN_COMMANDLINE_SWITCH_IGNOREDEPENDENCIES, pExecuteAction->exePackage.sczIgnoreDependencies);
hr = StrAllocateFormatted(&sczCommand, TRUE, L"%ls -%ls=%ls", sczCommand, BURN_COMMANDLINE_SWITCH_IGNOREDEPENDENCIES, pExecuteAction->exePackage.sczIgnoreDependencies);
ExitOnFailure(hr, "Failed to append the list of dependencies to ignore to the command line.");

hr = StrAllocFormatted(&sczCommandObfuscated, L"%ls -%ls=%ls", sczCommandObfuscated, BURN_COMMANDLINE_SWITCH_IGNOREDEPENDENCIES, pExecuteAction->exePackage.sczIgnoreDependencies);
Expand Down Expand Up @@ -544,11 +544,11 @@ extern "C" HRESULT ExeEngineExecutePackage(
::SetCurrentDirectoryW(wzCurrentDirectory);
}

ReleaseStr(sczArgumentsFormatted);
StrSecureZeroFreeString(sczArgumentsFormatted);
ReleaseStr(sczArgumentsObfuscated);
ReleaseStr(sczCachedDirectory);
ReleaseStr(sczExecutablePath);
ReleaseStr(sczCommand);
StrSecureZeroFreeString(sczCommand);
ReleaseStr(sczCommandObfuscated);

ReleaseHandle(pi.hThread);
Expand Down