Modifying an out parameter of a hooked function

See more:

C

hooking

I want to hook EnumProcesses() and, as a first exercise, remove some process IDs from the array it populates.

This is an out parameter so I had to let the function to execute, and then to do my dirty thing.

Basic idea, using a DLL that can be injected into a process:
- hook EnumProcesses() (did this)
- copy the address of the first (_Out_ DWORD *pProcessIds) and 3rd (_Out_ DWORD *pBytesReturned) parameters (did this, but I store them in a global variable)
- save current return address (at [ebp + 4]) (did this, but also in a global variable)
- change the return address so the function will return in one of my functions
- give control back to EnumProcesses()
Now instead of going back to where it was called, it goes to one of my friends, which will:
- save the return value from eax into a local variable (just to be safe)
- iterate through the array of process IDs, removing what I want to remove
- put back the return value in eax
- push the return value on the stack, return

This works as intended with one little drawback which I don't like: when my original program (the one who called EnumProcesses) exists I get: "Run-Time Check Failure #2 - Stack around the variable 'allProcesses' was corrupted", allProcesses is the variable used as the first parameter for my EnumProcesses call.
The resulting array is the expected one, but I don't like having my stack corrupted. And I also want to know where I messed up,

Here I save the out parameters addresses and I change the return address:

__declspec(naked)
void
__stdcall
InterceptEnumProc()
{
    DWORD_PTR retAdr;

    FARPROC change;

    HMODULE hModule;
    FARPROC oldFuncAdr;

    LPBYTE pJmpAdr;

    __asm
    {
        // redo the bites we over written with our hook
        sub edi, edi
        push ebp
        mov ebp, esp

        sub esp, __LOCAL_SIZE

        mov eax, [ebp + 4] // return address
        mov g_returnAdrEnumProc, eax

        mov eax, [ebp + 8] // *pProcessIds
        mov g_pProcIds, eax
        mov eax, [ebp + 16] // *pBytesReturned
        mov g_pBytedReturned, eax

        // save registers
        push esi
        push ebx
    };

    change = (FARPROC)ChangeEnumProc; // change the output
    retAdr = (DWORD_PTR)change;

    hModule = NULL;
    oldFuncAdr = NULL;
    hModule = LoadLibrary(TEXT("Psapi.dll"));
    // error checks for possible LoadLibrary failure omitted here
    oldFuncAdr = GetProcAddress(hModule,
        "EnumProcesses");
    // error checks for possible GetProcAddress omitted here

    pJmpAdr = (LPBYTE)(DWORD_PTR)oldFuncAdr + 5;

    __asm
    {
        pop ebx
        pop esi

        mov eax, retAdr // new return address
        mov [ebp + 4], eax
        mov esp, ebp
        jmp pJmpAdr
    };
}

And here I change the result:

__declspec(naked)
void
__stdcall
ChangeEnumProc()
{
    DWORD rValue;
    DWORD bytesReturned;
    DWORD procsReturned;
    DWORD i;
    DWORD currentProcId;
    DWORD poz;

    /* this was the problem:
      __asm
      {
          mov rValue, eax // save return value
      };
    */

    __asm
    {
        // create a stacj frame
        push ebp
        mov ebp, esp

        sub esp, __LOCAL_SIZE
        mov rValue, eax

        push esi
        push ebx
    };

    bytesReturned = *(DWORD* )g_pBytedReturned;
    procsReturned = bytesReturned/sizeof(DWORD);

    currentProcId = GetCurrentProcessId(); // I just hide the current process from the results

    for(i = 0; i < procsReturned; i++)
    {
        DWORD procId;

        procId = *g_pProcIds;

        if(procId == currentProcId)
        {
            poz = i;
            break;
        }
        g_pProcIds++;
    }

    g_pProcIds++;

    for(i = poz + 1; i < procsReturned; i++)
    {
        *(g_pProcIds - 1) = *g_pProcIds;
        g_pProcIds++;
    }

    // also, change the number of process IDs the caller should look for
    bytesReturned = bytesReturned - sizeof(DWORD);
    *(DWORD* )g_pBytedReturned = bytesReturned;

    __asm
    {
        pop ebx
        pop esi

        mov esp, ebp
        pop ebp

        mov eax, rValue
        jmp g_returnAdrEnumProc // go back to the caller
    };

}

The global variables:

PDWORD g_pProcIds;
PDWORD g_pBytedReturned;
DWORD_PTR g_returnAdrEnumProc;

EnumProcesses call:

DWORD allProcesses[1024];
DWORD bytesReturned = 0;

if(!EnumProcesses(allProcesses,
    sizeof(allProcesses),
    &bytesReturned))
{
    // etc, etc
}

The hooking is basic inline hooking, over writing the first 5 bytes of the API.
I think I do some wrong things when I manipulate those pointers. I took advantage of the fact that EnumProcesses needs pointers for the first and 3rd argument, but I'm not sure how correct is the whole approach.

As a side-note: I was thinking to loose the global variables, and let some space at the beginning of ChangeEnumProc (fill it wit NOPs) and then over write that space with the data I need. I will also like some feedback on this idea too (it seems like over kill, but I never liked global variables).

EDIT: found the problem with a bit of step by step running. I was assigning a value to a local variable before I reserved space on the stack for my local variables. Edited the code above to illustrate it. I'm still open to feedback and advises.