Code Reviews

[JohnLaTwC]

Issue 1

wmi_query always returns S_OK even on failure cases.

I am not sure this leads to any bugs, but it may not be the developer's intent.

HRESULT wmi_query(
    LPWSTR pwszServer,
    LPWSTR pwszNameSpace,   
    LPWSTR pwszQuery
)
{
    HRESULT hr = S_OK;

...

    hr = S_OK;

    // Initialize COM
    hr = Wmi_Initialize(&m_WMI);
    if (FAILED(hr))
    {
        BeaconPrintf(CALLBACK_ERROR, "Wmi_Initialize failed: 0x%08lx", hr);
        goto fail;
    }

    // Connect to WMI on host
    hr = Wmi_Connect(&m_WMI, pwszServer, pwszNameSpace);
    if (FAILED(hr))
    {
        BeaconPrintf(CALLBACK_ERROR, "Wmi_Connect failed: 0x%08lx", hr);
        goto fail;
    }

...

fail:

    hr = Wmi_Finalize(&m_WMI);
! this overrides any previous value of hr from a `goto fail` and will result in it being set to S_OK
    if (FAILED(hr))
! this code path will never be called
    {
-       BeaconPrintf(CALLBACK_ERROR, "Wmi_Destroy failed: 0x%08lx", hr);
+       BeaconPrintf(CALLBACK_ERROR, "Wmi_Finalize failed: 0x%08lx", hr);
    }

    return hr;
}

See the implementation of Wmi_Finalize always returns S_OK:

HRESULT Wmi_Finalize(
    WMI* pWmi
)
{
    HRESULT hr = S_OK;

    SAFE_RELEASE(pWmi->pWbemServices);
    SAFE_RELEASE(pWmi->pWbemLocator);

    SAFE_FREE(pWmi->bstrLanguage);
    SAFE_FREE(pWmi->bstrServer);
    SAFE_FREE(pWmi->bstrNameSpace);
    SAFE_FREE(pWmi->bstrNetworkResource);
    SAFE_FREE(pWmi->bstrQuery);

    // un-initialize the COM library
    OLE32$CoUninitialize();

! always returns S_OK
    return hr;

}

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/9dfb0524cbebe15bd6805b50c8e8b77075df0229/src/SA/wmi_query/entry.c#L92

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/9dfb0524cbebe15bd6805b50c8e8b77075df0229/src/common/wmi.c#L584

Issue 2

OpenSCManagerA and enumerate_services return win32 errors, not HRESULT so error code should be checked against ERROR_SUCCESS

I am not sure this leads to any bugs, but it may not be the developer's intent.

VOID go( 
    IN PCHAR Buffer, 
    IN ULONG Length 
) 
{
    const char * hostname = NULL;
    const char * servicename = NULL;
-   DWORD result = 0;
+   DWORD result = ERROR_SUCCESS;
... 
if ((gscManager = ADVAPI32$OpenSCManagerA(hostname, SERVICES_ACTIVE_DATABASEA, SC_MANAGER_CONNECT | GENERIC_READ)) == NULL)
    {
        result = KERNEL32$GetLastError();
    }
-   if(result == S_OK)
+   if(result == ERROR_SUCCESS)
    {
        result = enumerate_services();
-       if(result != S_OK)
+       if(result != ERROR_SUCCESS)
        {
            BeaconPrintf(CALLBACK_ERROR, "Failed to query service: %u", result);
        }

DWORD enumerate_services()
{
    DWORD dwResult = ERROR_SUCCESS;
...
    return dwResult;
! returns ERROR_SUCCESS or GetLastError, not S_OK
}

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/0fb0bb9cec045909e992d49ed407e8f895cecfd9/src/SA/sc_enum/entry.c#L424

Issue 3

enumerate_loaded_drivers() leaks memory in early return

int enumerate_loaded_drivers()
{
    // Create a handle to the service manager for calls to EnumServicesStatusExW.
    SC_HANDLE scm_handle = ADVAPI32$OpenSCManagerA(NULL, NULL, SC_MANAGER_ENUMERATE_SERVICE);
    if (!scm_handle)
    {
        internal_printf("[ERROR] Cannot open handle to service manager.\n");
        return 1;
    }

    // Determine the bytes needed for allocation.
    unsigned long bytes_needed = 0;
    unsigned long services_returned = 0;
    ADVAPI32$EnumServicesStatusExW(scm_handle, SC_ENUM_PROCESS_INFO, SERVICE_DRIVER, SERVICE_ACTIVE, NULL, 0, &bytes_needed, &services_returned, NULL, NULL);

    // Retrieve a buffer of active driver services.
    PBYTE services = (PBYTE)intAlloc(bytes_needed);
    if (!ADVAPI32$EnumServicesStatusExW(scm_handle, SC_ENUM_PROCESS_INFO, SERVICE_DRIVER , SERVICE_ACTIVE, services, bytes_needed, &bytes_needed, &services_returned, NULL, NULL))
    {
        internal_printf("[ERROR] Cannot enumerate services -> %ld.\n", KERNEL32$GetLastError());
! leaks memory allocated for `services`
        return 1;
    }
    LPENUM_SERVICE_STATUS_PROCESSW service = (LPENUM_SERVICE_STATUS_PROCESSW)services;
    // Get the ImagePath for each service from registry, and pass that to the validate_driver() function.
    for (unsigned long i = 0; i < services_returned; i++)
    {
        PWCHAR registry_path = (PWCHAR)intAlloc(MAX_PATH * 2);
        if (registry_path)
        {
            MSVCRT$wcsncat(registry_path,  L"SYSTEM\\CurrentControlSet\\Services\\", MAX_PATH);
            MSVCRT$wcsncat(registry_path, service->lpServiceName, MAX_PATH);
        }
        {
            BeaconPrintf(CALLBACK_ERROR, "Out of memory");
! leaks memory allocated for `services`
            return 1;
        }

        HKEY key_handle;
        ADVAPI32$RegOpenKeyExW(HKEY_LOCAL_MACHINE, registry_path, 0, KEY_QUERY_VALUE, &key_handle);

        unsigned long length = 0;
        ADVAPI32$RegQueryValueExW(key_handle, L"ImagePath", NULL, NULL, NULL, &length);
        intFree(registry_path);
        if (length)
        {
            PWCHAR driver_path = (PWCHAR)intAlloc(length);
! should check for failed allocation for `driver_path`
            ADVAPI32$RegQueryValueExW(key_handle, L"ImagePath", NULL, NULL, (LPBYTE)driver_path, &length);
            validate_driver(driver_path);
            intFree(driver_path);
        }

        KERNEL32$CloseHandle(key_handle);
        service++;
    }

    intFree(services);
    KERNEL32$CloseHandle(scm_handle);
    return 0;
}

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/0fb0bb9cec045909e992d49ed407e8f895cecfd9/src/SA/driversigs/entry.c#L187 https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/0fb0bb9cec045909e992d49ed407e8f895cecfd9/src/SA/driversigs/entry.c#L202

Issue 4

SCM handle must be closed with CloseServiceHandle not CloseHandle

int enumerate_loaded_drivers()
{
    // Create a handle to the service manager for calls to EnumServicesStatusExW.
    SC_HANDLE scm_handle = ADVAPI32$OpenSCManagerA(NULL, NULL, SC_MANAGER_ENUMERATE_SERVICE);
@@ scm_handle is not a kernel handle @@ 

...
-   KERNEL32$CloseHandle(scm_handle);
+   ADVAPI32$CloseServiceHandle(scm_handle );

    return 0;

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/0fb0bb9cec045909e992d49ed407e8f895cecfd9/src/SA/driversigs/entry.c#L224

Issue 5

Registry handle needs to be closed with RegCloseKey not CloseHandle

int enumerate_loaded_drivers()
{

...
        HKEY key_handle;
        ADVAPI32$RegOpenKeyExW(HKEY_LOCAL_MACHINE, registry_path, 0, KEY_QUERY_VALUE, &key_handle);

        unsigned long length = 0;
        ADVAPI32$RegQueryValueExW(key_handle, L"ImagePath", NULL, NULL, NULL, &length);
        intFree(registry_path);
        if (length)
        {
            PWCHAR driver_path = (PWCHAR)intAlloc(length);
            ADVAPI32$RegQueryValueExW(key_handle, L"ImagePath", NULL, NULL, (LPBYTE)driver_path, &length);
            validate_driver(driver_path);
            intFree(driver_path);
        }

-       KERNEL32$CloseHandle(key_handle);
+       ADVAPI32$RegCloseKey(key_handle);
        service++;
    }

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/0fb0bb9cec045909e992d49ed407e8f895cecfd9/src/SA/driversigs/entry.c#L219

Issue 6

I am not sure the effect here but I wanted to flag it. There are mismatches between the count of elements and the number of elements in calls to antiStringResolve

    EServiceStatus = antiStringResolve(8, "SPACER", "STOPPED", "START_PENDING", "STOP_PENDING", "RUNNING", "CONTINUE_PENDING", "PAUSE_PENDING");
! 8 items declared but only 7 in va_args
...
    EServiceStartup = antiStringResolve(6, "BOOT_DRIVER", "SYSTEM_START_DRIVER", "AUTO_START", "DEMAND_START", "DISABLED");
! 6 items declared but only 5 in va_args

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/9dfb0524cbebe15bd6805b50c8e8b77075df0229/src/SA/sc_qc/entry.c#L17

    EServiceStatus = antiStringResolve(8, "SPACER", "STOPPED", "START_PENDING", "STOP_PENDING", "RUNNING", "CONTINUE_PENDING", "PAUSE_PENDING");
! 8 items declared but only 7 in va_args
    EServiceStartup = antiStringResolve(6, "BOOT_DRIVER", "SYSTEM_START_DRIVER", "AUTO_START", "DEMAND_START", "DISABLED");
! 6 items declared but only 5 in va_args

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/9dfb0524cbebe15bd6805b50c8e8b77075df0229/src/SA/sc_enum/entry.c#L46

    EServiceStatus = antiStringResolve(8, "SPACER", "STOPPED", "START_PENDING", "STOP_PENDING", "RUNNING", "CONTINUE_PENDING", "PAUSE_PENDING");
! 8 items declared but only 7 in va_args

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/9dfb0524cbebe15bd6805b50c8e8b77075df0229/src/SA/sc_query/entry.c#L14

antiStringResolve will call va_arg on the extra argument and I am not sure the result.

char ** antiStringResolve(unsigned int count, ...)
{
    va_list strings;
    va_start(strings, count);
    char ** result = intAlloc(sizeof(char *) * count);
    for(int i = 0; i < count; i++)
    {
        result[i] = (char *)va_arg(strings, char *);
    }
    va_end(strings);
    return result;
}

Issue 7

Reg_EnumKey loop missing a call to intFree on item. It frees the memory of the item contents (val->keypath, val->hreg), but not the item itself.

pregkeyval init_regkey(const char * curpath, DWORD dwcurpathsz, const char * childkey, DWORD dwchildkeysz, HKEY hreg)
{
    pregkeyval item = (pregkeyval)intAlloc(sizeof(regkeyval));
@@ allocates memory for item @@
    item->dwkeypathsz = dwcurpathsz + ((dwchildkeysz) ? dwchildkeysz + 1 : 0); //str\str does not include null or just str, if we don't have a child key
    item->keypath = intAlloc(item->dwkeypathsz + 1);
    memcpy(item->keypath, curpath, dwcurpathsz);
    if(dwchildkeysz > 0)
    {
        item->keypath[dwcurpathsz] = '\\';
        memcpy(item->keypath + dwcurpathsz + 1, childkey, dwchildkeysz);
    }
    item->hreg = hreg;
    //item->keypath[item->dwkeypathsz] = 0;
    return item;
}

void free_regkey(pregkeyval val)
{
    if(val->keypath)
    {
        intFree(val->keypath);
    }
    if(val->hreg)
    {
        ADVAPI32$RegCloseKey(val->hreg);
    }
}

DWORD Reg_EnumKey(const char * hostname, HKEY hivekey, DWORD Arch, const char* keystring, BOOL recursive){
...
    keyStack->push(keyStack, init_regkey(keystring, strlen(keystring), NULL, 0, rootkey));
    while((curitem = keyStack->pop(keyStack)) != NULL)
...
        nextloop:
        if(currentkeyname)
        {intFree(currentkeyname); currentkeyname = NULL;}
        if(currentvaluename)
        {intFree(currentvaluename); currentvaluename = NULL;}
        if(currentdata)
        {intFree(currentdata); currentdata = NULL;}
        retCode = ERROR_SUCCESS;
        i = 0;
        cSubKeys = 0;
        cbMaxSubKey = 0;
        cValues = 0;
        cchMaxValue = 0;
        cchMaxData = 0;
        free_regkey(curitem);
! curitem itself not freed?
        curitem = NULL;
    } // end While

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/0fb0bb9cec045909e992d49ed407e8f895cecfd9/src/SA/reg_query/entry.c#L334

Issue 8

Copy Paste error leads to incorrect initialization. It doesn't cause a bug because the buffer size matches.

    char szGroupName[255] = {0};
    char szDomainName[255] = {0};

    DWORD cchGroupName  = _countof(szGroupName);
-    DWORD cchDomainName = _countof(szGroupName);
+    DWORD cchDomainName = _countof(szDomainName);

https://github.com/trustedsec/CS-Situational-Awareness-BOF/blob/0fb0bb9cec045909e992d49ed407e8f895cecfd9/src/SA/whoami/entry.c#L132

[freefirex]

I'm curious, especially from your earlier tweet, are you using any specific toolchain in your checks, or manual verification?

Just let me know when your done finding new things and I'll get them fixed :)

(ps: I'm off work for the next 2.5 weeks starting tomorrow so depending on when you say your done may be a slower turn around then last time)

[JohnLaTwC]

Code review only and findstr when I find a pattern. Obviously it's not perfect as some of these issues escaped me the first time around.

[freefirex]

Alright cool. I think no process is perfect, as I threw scan-build, cppcheck and manual runs with dr.memory at all of these now and obviously some things still get missed 😂

Thanks again though, the help is appreciated.

[JohnLaTwC]

I think this concludes my review. Thanks for the efforts looking into the issues I've flagged so far!