search

outflanknl / Ps-Tools

Ps-Tools, an advanced process monitoring toolkit for offensive operations
329 stars 83 forks source link

Multiple memory leaks and handle leaks in PsX/PsC/PsX/etc #1

Closed JohnLaTwC closed 3 years ago

JohnLaTwC commented 3 years ago

After a quick look. I think these issues affect all the Ps* tools because they have copies of the same code.

Issue 1

Leak when exiting function early from error path in IntegrityLevel()

DWORD IntegrityLevel(HANDLE hProcess) {
...

        PTOKEN_MANDATORY_LABEL pTIL = (PTOKEN_MANDATORY_LABEL)GlobalAlloc(GPTR, ReturnLength);
! No check for failed allocation for pTIL

        status = NtQueryInformationToken(hToken, TokenIntegrityLevel, pTIL, ReturnLength, &ReturnLength);
        if (status != STATUS_SUCCESS) {
+           GlobalFree(pTIL);
            CloseHandle(hToken);
            return 0;
        }

        dwIntegrityLevel = *GetSidSubAuthority(pTIL->Label.Sid, (DWORD)(UCHAR)(*GetSidSubAuthorityCount(pTIL->Label.Sid) - 1));

        GlobalFree(pTIL);
        CloseHandle(hToken);

        return dwIntegrityLevel;
    }

    return 0;
}

This bug also exists in PsC: https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsC-rDLL/PsC/ReflectiveDll.c#L101

Issue 2

Leak when exiting function early from error path in GetTokenUser()

LPWSTR GetTokenUser(HANDLE hProcess) {
...

    if (status == STATUS_SUCCESS) {
        status = NtQueryInformationToken(hToken, TokenUser, NULL, 0, &ReturnLength);
        if (status != STATUS_BUFFER_TOO_SMALL) {
            CloseHandle(hToken);
            return NULL;
        }

        PTOKEN_USER Ptoken_User = (PTOKEN_USER)GlobalAlloc(GPTR, ReturnLength);
! No check for failed allocation  
        status = NtQueryInformationToken(hToken, TokenUser, Ptoken_User, ReturnLength, &ReturnLength);
        if (status != STATUS_SUCCESS) {
+           GlobalFree(Ptoken_User);
            CloseHandle(hToken);
            return NULL;
        }

        if (!LookupAccountSid(NULL, Ptoken_User->User.Sid, lpName, &dwSize, lpDomain, &dwSize, &SidType))
        {
            GlobalFree(Ptoken_User);
            CloseHandle(hToken);
            return NULL;
        }

        LPWSTR lpUser = (LPWSTR)calloc(MAX_NAME, 1);
        wcscat_s(lpUser, MAX_NAME, lpDomain);
        wcscat_s(lpUser, MAX_NAME, L"\\");
        wcscat_s(lpUser, MAX_NAME, lpName);

        GlobalFree(Ptoken_User);
        CloseHandle(hToken);

        return lpUser;
    }

    return NULL;
}

Issue 3

free() must be called on chUserName to avoid leaking memory

                    LPWSTR chUserName = GetTokenUser(hProcess);
                    if (chUserName != NULL) {
                        wprintf(L"    UserName:\t %s\n", chUserName);
+                       free(chUserName);
                        chUserName = NULL;
                    }

https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L785

Issue 4

Leak when exiting function early from error path in EnumFileProperties(). Many of these code paths fail to close the handle for hFile as well.

    status = NtQuerySystemInformation(SystemProcessIdInformation, &pInfo, sizeof(pInfo), NULL);
    if (status != STATUS_SUCCESS) {
        return FALSE;
! This early exit leaks the memory from `pInfo.ImageName.Buffer`
...
    NTSTATUS Status = NtCreateFile(&hFile, (GENERIC_READ | SYNCHRONIZE), &FileObjectAttributes, &IoStatusBlock, 0,
        0, FILE_SHARE_READ, FILE_OPEN, FILE_SYNCHRONOUS_IO_NONALERT | FILE_NON_DIRECTORY_FILE, NULL, 0);

! This hFile handle is leaked in the early function exits below

    }

https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L317 https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L330 https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L359 https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L336 https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L339 https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L364

Issue 5

Memory used to get version info is never freed

    PBYTE lpVerInfo = (PBYTE)calloc(dwLen, sizeof(BYTE));
! This memory is never freed
    if (!GetFileVersionInfo(pwszPath, dwHandle, dwLen, lpVerInfo)) {
        return FALSE;
    }
...
    LPWSTR lpCompany = (LPWSTR)calloc(MAX_PATH, sizeof(WCHAR));
    LPWSTR lpDescription = (LPWSTR)calloc(MAX_PATH, sizeof(WCHAR));
    LPWSTR lpProductVersion = (LPWSTR)calloc(MAX_PATH, sizeof(WCHAR));
! This memory is never freed

https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L362

Issue 6

EnumSecurityProc leaks memory every time it is called. It should check to see if the memory is already allocated.

void EnumSecurityProc(LPWSTR lpCompany, LPWSTR lpDescription, DWORD dwPID) {
    pSecProducts[dwSecProcCount] = (PSECPROD)calloc(1, sizeof(SECPROD));
! Never freed

https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.c#L218

Issue 7

GetTcpSessions and GetTcp6Sessions leak memory for pTcpTable on early function exit

                _RtlIpv4AddressToStringW RtlIpv4AddressToStringW = (_RtlIpv4AddressToStringW)
                    GetProcAddress(GetModuleHandle(L"ntdll.dll"), "RtlIpv4AddressToStringW");
                if (RtlIpv4AddressToStringW == NULL) {
                    return FALSE;
! Fails to call HeapFree on pTcpTable before return and leaks it
                }

https://github.com/outflanknl/Ps-Tools/blob/81c83ebcd366eae20bc9008d735f7b5dd0117ff0/Src/Outflank-PsH-rDLL/PsH/ReflectiveDll.c#L776

GetTcp6Sessions has a similar issue here:

https://github.com/outflanknl/Ps-Tools/blob/81c83ebcd366eae20bc9008d735f7b5dd0117ff0/Src/Outflank-PsH-rDLL/PsH/ReflectiveDll.c#L875

These functions also have different return values and it isn't clear why: GetTcp6Sessions returns TRUE whereas GetTcpSessions returns `FALSE

https://github.com/outflanknl/Ps-Tools/blob/81c83ebcd366eae20bc9008d735f7b5dd0117ff0/Src/Outflank-PsH-rDLL/PsH/ReflectiveDll.c#L799

https://github.com/outflanknl/Ps-Tools/blob/81c83ebcd366eae20bc9008d735f7b5dd0117ff0/Src/Outflank-PsH-rDLL/PsH/ReflectiveDll.c#L906

Issue 8

PsW EnumWindowsProc leaks memory on error exit

        LPVOID pBuffer = NULL;
        SIZE_T uSize = uReturnLength;
        status = NtAllocateVirtualMemory(NtCurrentProcess(), &pBuffer, 0, &uSize, MEM_COMMIT, PAGE_READWRITE);
        if (status != STATUS_SUCCESS) {
            return TRUE;
        }

        status = NtQuerySystemInformation(SystemProcessInformation, pBuffer, uReturnLength, &uReturnLength);
        if (status != STATUS_SUCCESS) {
            return TRUE;
! Leaks pBuffer
        }

https://github.com/outflanknl/Ps-Tools/blob/106a7bd75a0316da12b6b3be55398c36272c0bac/Src/Outflank-PsW-rDLL/PsW/ReflectiveDll.c#L71

Cn33liz commented 3 years ago

Hi John, thanks for pointing this out. I appreciate you taking the time for this! I apologise for it being it a bit of double work as I already did a cleanup of the psx code a few months ago...but simply forgot to commit.

Let me know if you still find any issues in the code. Btw the other tools (psc e.g.) wil be updated asap.

MarcOverIP commented 3 years ago

Its also a bit of my fault: as we are in heavy development of version 2 of RedELK (one of our other projects), we wanted to align the release of new versions of both tools. @Cn33liz had a new version of psx ready for some time.

But thanks again for taking the effort and keeping us sharp.

image

JohnLaTwC commented 3 years ago

I took a quick look at the new version (thanks for pointing that out).

Can you look over these issues and if they seem valid, I'll keep looking.

Issue 1

LPWSTR GetProcessUser(IN HANDLE hProcess, BOOL bCloseHandle, BOOL bReturnDomainname, BOOL bReturnUsername) {
    HANDLE hToken = NULL;
    ULONG ReturnLength;
    PTOKEN_USER Ptoken_User = NULL;
    WCHAR lpName[MAX_NAME];
    WCHAR lpDomain[MAX_NAME];
    DWORD dwSize = MAX_NAME;
    LPWSTR lpwUser = NULL;
    SID_NAME_USE SidType;

...
        lpwUser = (LPWSTR)calloc(MAX_NAME, sizeof(WCHAR));
! No check for failed allocation before use
        if (bReturnDomainname) {
            wcscat_s(lpwUser, MAX_NAME, lpDomain);
            if (bReturnUsername) {
                wcscat_s(lpwUser, MAX_NAME, L"\\");
            }
        }
        if (bReturnUsername) {
            wcscat_s(lpwUser, MAX_NAME, lpName);
        }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L156

Issue 2

RtlCopyMemory requires a byte count for Length, but EnumSecurityProc passes in MAX_PATH which is a character count because the fields are WCHAR

BOOL EnumSecurityProc(IN LPWSTR lpCompany, IN LPWSTR lpDescription, IN DWORD dwPID) {
    LPCWSTR pwszCompany[26];
...
    const DWORD dwSize = _countof(pwszCompany);
    for (DWORD i = 0; i < dwSize && dwSecProcCount < MAX_SEC_PRD; i++) {
        if (StrStrIW(lpCompany, pwszCompany[i])) {
            pSecProducts[dwSecProcCount]->dwPID = dwPID;
            RtlCopyMemory(pSecProducts[dwSecProcCount]->wcCompany, lpCompany, MAX_PATH);
! MAX_PATH is character count, not byte count
            RtlCopyMemory(pSecProducts[dwSecProcCount]->wcDescription, lpDescription, MAX_PATH);
            dwSecProcCount++;
        }
    }

    if (dwSecProcCount < MAX_SEC_PRD) {
        //Windows Defender (ATP)
        if (StrStrIW(lpDescription, L"Antimalware Service Executable") || StrStrIW(lpDescription, L"Windows Defender")) {
            pSecProducts[dwSecProcCount]->dwPID = dwPID;
            RtlCopyMemory(pSecProducts[dwSecProcCount]->wcCompany, lpCompany, MAX_PATH);
            RtlCopyMemory(pSecProducts[dwSecProcCount]->wcDescription, lpDescription, MAX_PATH);
            dwSecProcCount++;
        }
    }

    if (dwSecProcCount < MAX_SEC_PRD) {
        //Carbon Black
        if (StrStrIW(lpDescription, L"Carbon Black")) {
            pSecProducts[dwSecProcCount]->dwPID = dwPID;
            RtlCopyMemory(pSecProducts[dwSecProcCount]->wcCompany, lpCompany, MAX_PATH);
            RtlCopyMemory(pSecProducts[dwSecProcCount]->wcDescription, lpDescription, MAX_PATH);
            dwSecProcCount++;
        }
    }

In psx.h these fields are declared as wide chars:

typedef struct _SECPROD {
    DWORD dwPID;
    WCHAR wcCompany[MAX_PATH];
    WCHAR wcDescription[MAX_PATH];
} SECPROD, *PSECPROD;

Issue 3

Leak of lpwProcUser in DllMain in error branch

                    status = NtOpenProcess(&hProcess, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, &ObjectAttributes, &uPid);
                    if (hProcess != NULL) {
                        LPWSTR lpwProcUser = GetProcessUser(hProcess, FALSE, TRUE, TRUE);
                        if (lpwProcUser != NULL) {
                            swprintf_s(chBuffer, _countof(chBuffer), L"    UserName:    %s\n", lpwProcUser);
                            if (FAILED(hr = lpStream->Write(chBuffer, (ULONG)wcslen(chBuffer) * sizeof(WCHAR), &dwWritten))) {
+                               free(lpwProcUser);
                                goto CleanUp;
                            }

                            RtlZeroMemory(chBuffer, _countof(chBuffer));
                            free(lpwProcUser);
                        }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L1188

Issue 4

RtlZeroMemory takes a byte count but EnumPeb passes in a character count resulting in a partial clear of the memory

BOOL EnumPeb(HANDLE hProcess, IN LPSTREAM lpStream) {
    WCHAR chReadBuf[MAX_BUF] = { 0 };
    WCHAR chWriteBuf[MAX_BUF] = { 0 };
    DWORD dwWritten = 0;
...
    RtlZeroMemory(chReadBuf, _countof(chReadBuf));  <<<<< _countof is character count not byte count
    RtlZeroMemory(chWriteBuf, _countof(chWriteBuf));

...
    RtlZeroMemory(chReadBuf, _countof(chReadBuf));
    RtlZeroMemory(chWriteBuf, _countof(chWriteBuf));

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L524

This also occurs here: https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L616 https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L625 https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L633 https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L683 https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L675 https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L691

Issue 5

chReadBuf is vulnerable to stack buffer overrun because of missing length check prior to copy of ImagePathName and CommandLine

ImagePathName is defined as a UNICODE_STRING which defines upp.CommandLine.MaximumLength as a USHORT. However the target buffer chReadBuf is MAX_BUF (8192) * 2 = 16,384 bytes long, which is less than the max value of an unsigned short (65,535).

#define MAX_BUF 8192

...
BOOL EnumPeb(HANDLE hProcess, IN LPSTREAM lpStream) {
...
    WCHAR chReadBuf[MAX_BUF] = { 0 };
...
    status = NtReadVirtualMemory(hProcess, upp.ImagePathName.Buffer, &chReadBuf, upp.ImagePathName.MaximumLength, NULL);
! No check that upp.ImagePathName.MaximumLength exceeds size of chReadBuf
    if (status != STATUS_SUCCESS) {
        return FALSE;
    }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L514

typedef struct _RTL_USER_PROCESS_PARAMETERS {
  BYTE           Reserved1[16];
  PVOID          Reserved2[10];
  UNICODE_STRING ImagePathName;
  UNICODE_STRING CommandLine;
}

typedef struct _UNICODE_STRING {
  USHORT Length;
  USHORT MaximumLength;
  PWSTR  Buffer;
} UNICODE_STRING, *PUNICODE_STRING;

There is a similar issue when reading the command line:

    status = NtReadVirtualMemory(hProcess, upp.CommandLine.Buffer, &chReadBuf, upp.CommandLine.MaximumLength, NULL);
! No check that upp.CommandLine.MaximumLength exceeds size of chReadBuf
    if (status != STATUS_SUCCESS) {
        return FALSE;
    }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L527

Issue 6

Mismatch on declaration/allocation of pInfo.ImageName in EnumFileProperties Its max length set to MAX_PATH but ImageName.MaximumLength is a byte count not a character count. The allocated buffer is MAX_PATH * 2.


BOOL EnumFileProperties(IN HANDLE ProcessId, IN LPSTREAM lpStream) {
    SYSTEM_PROCESS_ID_INFORMATION pInfo;
...
    pInfo.ProcessId = ProcessId;
    pInfo.ImageName.Length = 0;
-   pInfo.ImageName.MaximumLength = MAX_PATH;
+   pInfo.ImageName.MaximumLength = MAX_PATH * sizeof (WCHAR);
    pInfo.ImageName.Buffer = NULL;

    pInfo.ImageName.Buffer = (PWSTR)calloc(pInfo.ImageName.MaximumLength, sizeof(WCHAR));

    status = NtQuerySystemInformation(SystemProcessIdInformation, &pInfo, sizeof(pInfo), NULL);
    if (status != STATUS_SUCCESS) {
        goto CleanUp;
    }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L578
Cn33liz commented 3 years ago

Hi John, above issues should be fixed. Could you run this check again?

JohnLaTwC commented 3 years ago

Hi, here are a few more. Can you take a look?

Issue 1

lpwKernelPath is never freed leading to memory leak

    lpwKernelPath = Utf8ToUtf16((LPSTR)pModuleInfo->Module[0].FullPathName);
    if (lpwKernelPath != NULL) {
        UNICODE_STRING uKernel;
        RtlInitUnicodeString(&uKernel, lpwKernelPath);

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L797

Issue 2

Incorrect calls to RtlZeroMemory specify a character count instead of a byte count leading to partial zeroing of memory.

There are numerous calls that pass a character count to RtlZeroMemory using _countof. RtlZeroMemory takes a byte count.

        RtlZeroMemory(chBuffer, _countof(chBuffer));

There are many lines where this occurs. Here are a couple:

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L356 https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L407

Issue 3

In EnumFileProperties ImageName in SYSTEM_PROCESS_ID_INFORMATION is typed as a UNICODE_STRING where MaximumLength is a byte count. This code initializes it using a character count.

    SYSTEM_PROCESS_ID_INFORMATION pInfo;

    pInfo.ProcessId = ProcessId;
    pInfo.ImageName.Length = 0;
-   pInfo.ImageName.MaximumLength = MAX_PATH;
+   pInfo.ImageName.MaximumLength = MAX_PATH * sizeof(WCHAR);
    pInfo.ImageName.Buffer = NULL;

    pInfo.ImageName.Buffer = (PWSTR)calloc(pInfo.ImageName.MaximumLength, sizeof(WCHAR));

    status = NtQuerySystemInformation(SystemProcessIdInformation, &pInfo, sizeof(pInfo), NULL);
    if (status != STATUS_SUCCESS) {
        goto CleanUp;
    }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L581

See (https://docs.microsoft.com/en-us/windows/win32/api/subauth/ns-subauth-unicode_string) 

MaximumLength
Specifies the total size, in bytes, of memory allocated for Buffer. Up to MaximumLength bytes may be written into the buffer without trampling memory.

Issue 4

In DllMain pwszParams is allocated but never freed.

BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD dwReason, LPVOID lpReserved)
{
    BOOL bReturnValue = TRUE;
    LPWSTR pwszParams = (LPWSTR)calloc(strlen((LPSTR)lpReserved) + 1, sizeof(WCHAR));

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L949

Issue 5

I had mentioned this one before (it was Issue 5 in an earlier comment)

In EnumPeb chReadBuf is vulnerable to stack buffer overrun because of missing length check prior to copy of ImagePathName and CommandLine

ImagePathName is defined as a UNICODE_STRING which defines upp.CommandLine.MaximumLength as a USHORT. However the target buffer chReadBuf is MAX_BUF (8192) * 2 = 16,384 bytes long, which is less than the max value of an unsigned short (65,535).

#define MAX_BUF 8192

...
BOOL EnumPeb(HANDLE hProcess, IN LPSTREAM lpStream) {
...
    WCHAR chReadBuf[MAX_BUF] = { 0 };
...
    status = NtReadVirtualMemory(hProcess, upp.ImagePathName.Buffer, &chReadBuf, upp.ImagePathName.MaximumLength, NULL);
! No check that upp.ImagePathName.MaximumLength exceeds size of chReadBuf
    if (status != STATUS_SUCCESS) {
        return FALSE;
    }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L514

typedef struct _RTL_USER_PROCESS_PARAMETERS {
  BYTE           Reserved1[16];
  PVOID          Reserved2[10];
  UNICODE_STRING ImagePathName;
  UNICODE_STRING CommandLine;
}

typedef struct _UNICODE_STRING {
  USHORT Length;
  USHORT MaximumLength;
  PWSTR  Buffer;
} UNICODE_STRING, *PUNICODE_STRING;

There is a similar issue when reading the command line:


    status = NtReadVirtualMemory(hProcess, upp.CommandLine.Buffer, &chReadBuf, upp.CommandLine.MaximumLength, NULL);
! No check that upp.CommandLine.MaximumLength exceeds size of chReadBuf
    if (status != STATUS_SUCCESS) {
        return FALSE;
    }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L527

Issue 6

There are several error paths in DllMain where the handle hProcess is not properly closed. The code soon calls ExitProcess where this handle will be automatically closed, but from a correctness point of view, it should be closed.

NOTE: You cannot close it in the CleanUp label because it is declared in a block that goes out of scope (if (bExtended) block) by the time the goto would be called and you would be operating on undefined memory. You should declare the handle higher in the function. https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L1174

                //Psx (Extended Process Info)
                if (bExtended) {
                    HANDLE hProcess = NULL;
                    OBJECT_ATTRIBUTES ObjectAttributes;
                    InitializeObjectAttributes(&ObjectAttributes, NULL, 0, NULL, NULL);
                    CLIENT_ID uPid = { 0 };

                    uPid.UniqueProcess = pProcInfo->ProcessId;
                    uPid.UniqueThread = (HANDLE)0;

                    status = NtOpenProcess(&hProcess, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, &ObjectAttributes, &uPid);
                    if (hProcess != NULL) {
                        LPWSTR lpwProcUser = GetProcessUser(hProcess, FALSE, TRUE, TRUE);
                        if (lpwProcUser != NULL) {
                            swprintf_s(chBuffer, _countof(chBuffer), L"    UserName:    %s\n", lpwProcUser);
                            if (FAILED(hr = lpStream->Write(chBuffer, (ULONG)wcslen(chBuffer) * sizeof(WCHAR), &dwWritten))) {
                                goto CleanUp;
                            }

                            RtlZeroMemory(chBuffer, _countof(chBuffer));
                            free(lpwProcUser);
                        }

                        DWORD dwIntegrityLevel = IntegrityLevel(hProcess);
                        if (dwIntegrityLevel == LowIntegrity) {
                            swprintf_s(chBuffer, _countof(chBuffer), L"    Integrity:   Low\n");
                            if (FAILED(hr = lpStream->Write(chBuffer, (ULONG)wcslen(chBuffer) * sizeof(WCHAR), &dwWritten))) {
                                goto CleanUp;
! Leaks hProcess handle
                            }

                            RtlZeroMemory(chBuffer, _countof(chBuffer));
                            dwLowProc++;
                        }
                        else if (dwIntegrityLevel == MediumIntegrity) {
                            swprintf_s(chBuffer, _countof(chBuffer), L"    Integrity:   Medium\n");
                            if (FAILED(hr = lpStream->Write(chBuffer, (ULONG)wcslen(chBuffer) * sizeof(WCHAR), &dwWritten))) {
                                goto CleanUp;
! Leaks hProcess handle
                            }

                            RtlZeroMemory(chBuffer, _countof(chBuffer));
                            dwMediumProc++;
                        }
                        else if (dwIntegrityLevel == HighIntegrity) {
                            swprintf_s(chBuffer, _countof(chBuffer), L"    Integrity:   High\n");
                            if (FAILED(hr = lpStream->Write(chBuffer, (ULONG)wcslen(chBuffer) * sizeof(WCHAR), &dwWritten))) {
                                goto CleanUp;
! Leaks hProcess handle
                            }

                            RtlZeroMemory(chBuffer, _countof(chBuffer));
                            dwHighProc++;
                        }
                        else if (dwIntegrityLevel == SystemIntegrity) {
                            swprintf_s(chBuffer, _countof(chBuffer), L"    Integrity:   System\n");
                            if (FAILED(hr = lpStream->Write(chBuffer, (ULONG)wcslen(chBuffer) * sizeof(WCHAR), &dwWritten))) {
                                goto CleanUp;
! Leaks hProcess handle
                            }

                            RtlZeroMemory(chBuffer, _countof(chBuffer));
                            dwSystemProc++;
                        }

https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L1182

Several gotos like this one: https://github.com/outflanknl/Ps-Tools/blob/1ac7299fb2b83bc3154a6ba785741a3f6903f1f7/Src/Outflank-PsX-rDLL/PsX/ReflectiveDll.cpp#L1188

Cn33liz commented 3 years ago

Are you sure you checked the latest commit? In commit a41c3b9 most of these issues have already been fixed.

JohnLaTwC commented 3 years ago

Yes, I must have reviewed an earlier on. I think these still remain from my previous comment: Issue 1 Issue 4 Issue 6

Cn33liz commented 3 years ago

These are now fixed in commit: bd2920a