1

I am tring to create a Remote thread that will load a DLL I wrote, and run a function from it. The DLL is working fine (Checked) but from some reason, the Remote thread fails and the proccess in which it was created stop responding.

I used ollyDebug to try and see what is going wrong and I noticed two things...

  1. My strings (dll name and function name) are passed to the remote thread correctly
  2. The thread fails on LoadLibrary with lasterror code 87 "ERROR_INVALID_PARAMETER"

My best guess is that somehow, The remote thread can't find LoadLibrary (Is this because the linker is done with repspect to my proccess???, Just a guess...)

What am I doing wrong?

This is the code to the remote function:

static DWORD WINAPI SetRemoteHook (DATA *data)
{
  HINSTANCE dll;
  HHOOK WINAPI hook;
  HOOK_PROC hookAdress;

  dll = LoadLibrary(data->dll);

  hookAdress = (HOOK_PROC) GetProcAddress(dll,data->func);
  if (hookAdress != NULL)
  {
    (hookAdress)(); 
  }
  return 1;
}

Edit:

This is the part in which I allocate the memory to the remote proccess:

typedef struct
{
    char* dll;
    char* func;
} DATA;

char* dllName = "C:\\Windows\\System32\\cptnhook.dll";  
char* funcName = "SetHook";
char* targetPrgm = "mspaint.exe";
Data lData;
lData.dll = (char*) VirtualAllocEx( explorer, 0, sizeof(char)*strlen(dllName), MEM_COMMIT, PAGE_READWRITE );
lData.func = (char*) VirtualAllocEx( explorer, 0, sizeof(char)*strlen(funcName), MEM_COMMIT, PAGE_READWRITE );
WriteProcessMemory( explorer, lData.func, funcName, sizeof(char)*strlen(funcName), &v );
WriteProcessMemory( explorer, lData.dll, dllName, sizeof(char)*strlen(dllName), &v );
rDataP = (DATA*) VirtualAllocEx( explorer, 0, sizeof(DATA), MEM_COMMIT, PAGE_READWRITE );
WriteProcessMemory( explorer, rDataP, &lData, sizeof(DATA), NULL );

Edit: It looks like the problem is that the remote thread is calling a "garbage" address instead of LoadLibrary base address. Is there a possibily Visual studio linked the remote proccess LoadLibrary address wrong?

Edit: when I try to run the same exact code as a local thread (I use a handle to the current procces in CreateRemoteThread) the entire thing works just fine. What can cause this?

Should I add the calling function code? It seems to be doing its job as the code is being executed in the remote thread with the correct parameters...

The code is compiled under VS2010.

data is a simple struct with char* 's to the names. (As explicetly writing the strings in code would lead to pointers to my original proccess).

What am I doing wrong?

Roi Ronn
  • 55
  • 1
  • 8
  • have you tried to use LoadLibrary with explicit dll path? Example: LoadLibrary("C:\\project\\my_app.dll"); just to make sure parameters are passed OK – marcinj Dec 31 '11 at 13:11
  • Yes. I am using the full path. (Altough it should have worked with the dll name alone, as the exe and the dll resides in the same directory). I should also mention that the fact that its the LoadLibrary function that is a failing is a guess as well. (I can see that it is a function that got only one paramter and that it is the correct path, looks like LoadLibrary...) – Roi Ronn Dec 31 '11 at 13:31
  • How do you communicate "data" to the remote thread? How do you allocate and fill the memory pointed by "data"? – Giuseppe Guerrini Dec 31 '11 at 15:46
  • The "data" was communicate using VirtualAllocEx and WriteProcessMemory. It is a pointer to a struct that hold pointer to string with my dll name and the function name in the dll - both allocated in the remote thread proccess memory. I edited the post to include the allocation – Roi Ronn Dec 31 '11 at 15:56
  • Classic off-by-one bug, you're forgetting to also copy the string terminator. – Hans Passant Dec 31 '11 at 16:00
  • Should have seen the 1 off bug... I changed it, but it doesn't solve the problem. The wierd thing is, that even with that bug Olly shows that when the (LoadLibrary?) function fails, the currect name is loaded into the ecx registery... Also, If I try to run the exact same code only in a local thread - it works fine... – Roi Ronn Dec 31 '11 at 16:18
  • Does the remote process have `PROCESS_CREATE_THREAD` access rights? – DRH Dec 31 '11 at 16:31
  • It should. The remote proccess is mspaint.exe or explorer.exe (It should be explorer, I just test on paint as it is easier to recover when it crushes as it does...) – Roi Ronn Dec 31 '11 at 16:37
  • Use a shell extension instead of injecting into Explorer. Shell extensions are supported. Injection not so much. You are also much more likely to be flagged as malware. – Raymond Chen Dec 31 '11 at 17:14
  • I would have, but this is actually an ex, in which explorer dll injection is a goal. On that note - the ex was writetn for windows XP, and I am working on a Vista machin, is there a chance I'm "dying" over a malware protection system that is not present in XP? – Roi Ronn Dec 31 '11 at 17:20

2 Answers2

1

Failing with ERROR_INVALID_PARAMETER indicates that there is a problem with the parameters passed.

So one should look at data->dll which represents the only parameter in question.

It is initialised here:

lData.dll = VirtualAllocEx(explorer, 0, sizeof(char) * (strlen(dllName) + 1), MEM_COMMIT, PAGE_READWRITE);

So let's add a check whether the allocation of the memory which's reference should be store into lData.dll really succeded.

if (!lData.dll) {
  // do some error logging/handling/whatsoever
}

Having done so, you might have detected that the call as implemented failed because (verbatim from MSDN for VirtualAllocEx()):

The function fails if you attempt to commit a page that has not been reserved. The resulting error code is ERROR_INVALID_ADDRESS.

So you might like to modifiy the fourth parameter of the call in question as recommended (again verbatim from MSDN):

To reserve and commit pages in one step, call VirtualAllocEx with MEM_COMMIT | MEM_RESERVE.

PS: Repeat this exercise for the call to allocate lData.func. ;-)

alk
  • 69,737
  • 10
  • 105
  • 255
0

It's possible that LoadLibrary is actually aliasing LoadLibraryW (depending on project settings), which is the Unicode version. Whenever you use the Windows API with "char" strings instead of "TCHAR", you should explicitly use ANSI version names. This will prevent debugging hassles when the code is written, and also in the future for you or somebody else in case the project ever flips to Unicode.

So, in addition to fixing that horrible unterminated string problem, make sure to use:

LoadLibraryA(data->dll);