Some code correctness and hygiene issues

[JohnLaTwC]

Some code correctness issues in PPLDump

These are hygiene issues. Some of these are low priority and edge cases.

I initially spotted these in the port of the code here:
EspressoCake/PPLDump_BOF#1

and decided to file the bugs upstream here too.

Edge case leak if allocation fails

BOOL TokenCompareSids(PSID pSidA, PSID pSidB)
{
	BOOL bReturnValue = FALSE;
	LPWSTR pwszSidA = NULL;
	LPWSTR pwszSidB = NULL;

	if (ConvertSidToStringSid(pSidA, &pwszSidA) && ConvertSidToStringSid(pSidB, &pwszSidB))
	{
		bReturnValue = _wcsicmp(pwszSidA, pwszSidB) == 0;
		LocalFree(pwszSidA);
		LocalFree(pwszSidB);
	}
	else
! it's possible only one of the calls to ConvertSidToStringSid failed and this branch will leak the Sid for the success case
		PrintLastError(L"ConvertSidToStringSid");

	return bReturnValue;
}

See:

PPLdump/PPLdump/utils.cpp

Line 502 in fa48466

if (ConvertSidToStringSid(pSidA, &pwszSidA) && ConvertSidToStringSid(pSidB, &pwszSidB))

There is another case here:

   if (TokenGetSid(hTokenDup, &pSidTmp) && TokenGetUsername(hTokenDup, &pwszUsername))

PPLdump/PPLdump/exploit.cpp

Line 935 in fa48466

if (TokenGetSid(hTokenDup, &pSidTmp) && TokenGetUsername(hTokenDup, &pwszUsername))

Consider calling ADVAPI32!IsTokenRestricted instead of rolling your own function here:

BOOL TokenIsNotRestricted(HANDLE hToken, PBOOL pbIsNotRestricted) {

...

PPLdump/PPLdump/utils.cpp

Line 632 in fa48466

BOOL TokenIsNotRestricted(HANDLE hToken, PBOOL pbIsNotRestricted)

Fail to check if memory was successfully allocated for guid

Check for failed allocation from MiscGenerateGuidString

    MiscGenerateGuidString(&pwszGuid);

PPLdump/PPLdump/exploit.cpp

Line 303 in fa48466

MiscGenerateGuidString(&pwszGuid);

Leak of hCurrentToken token in DumpProcess()

	if (bCurrentUserIsSystem)
	{
		if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_DUPLICATE | TOKEN_ADJUST_PRIVILEGES, &hCurrentToken))
		{
			PrintLastError(L"OpenProcessToken");
			goto end;
		}
	}
	else
	{
		if (!OpenThreadToken(GetCurrentThread(), TOKEN_QUERY | TOKEN_DUPLICATE | TOKEN_ADJUST_PRIVILEGES, FALSE, &hCurrentToken))
		{
			PrintLastError(L"OpenThreadToken");
			goto end;
		}
	}

	PrintDebug(L"Enable privilege %ws\n", SE_ASSIGNPRIMARYTOKEN_NAME);

	if (!TokenCheckPrivilege(hCurrentToken, SE_ASSIGNPRIMARYTOKEN_NAME, TRUE))
		goto end;

	PrintDebug(L"Create a primary token\n");

	if (!DuplicateTokenEx(hCurrentToken, MAXIMUM_ALLOWED, NULL, SecurityAnonymous, TokenPrimary, &hNewProcessToken))
	{
		PrintLastError(L"DuplicateTokenEx");
		goto end;
	}
! No call to CloseHandle on hCurrentToken

PPLdump/PPLdump/exploit.cpp

Line 328 in fa48466

if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_DUPLICATE | TOKEN_ADJUST_PRIVILEGES, &hCurrentToken))

Handle of hTransaction leaked in WritePayloadDllTransacted

No call to CloseHandle for hTransaction

	status = NtCreateTransaction(&hTransaction, TRANSACTION_ALL_ACCESS, &oa, NULL, NULL, 0, 0, 0, NULL, NULL);

PPLdump/PPLdump/exploit.cpp

Line 798 in fa48466

BOOL WritePayloadDllTransacted(_Out_ PHANDLE pdhFile)

FindFileForTransaction leaks memory for pSidTarget

Need a call to LocalFree at function exit for pSidTarget

	PSID pSidTarget = NULL;

	ConvertStringSidToSid(L"S-1-5-18", &pSidTarget);

PPLdump/PPLdump/exploit.cpp

Line 754 in fa48466

ConvertStringSidToSid(L"S-1-5-18", &pSidTarget);

[itm4n]

Hi!

Did you run a source code review tool or something? :)

I am already aware of some of these issues as I intentionally took some shortcuts during the development.
However, I did forget to call CloseHandle on some of the handles.

So, thank you for your feedback. I'll see what I can do. 👍
All of these issues are rather easy to fix.

[JohnLaTwC]

Just code review sir