Why my approach to create a codecave is not working ?

[Healian]

Well i am trying to create a codecave... this is my first time trying. i have injected the dll successfully in the target but my custom codecave is not getting called

#include <Windows.h>

__declspec(naked) void codecave()
{
	__asm
	{
		PUSHAD
		PUSHFD
	}

	MessageBox(NULL, "ROCK", "THE", MB_ICONINFORMATION);

	__asm
	{
		POPFD
		POPAD
		ret
	}
}

__declspec(naked) void *shortcut(const byte *dst)
{
	__asm{
		jmp dst
	}
}

void CreatRoute(byte *src, const byte *dst)
{
	DWORD oldProtection;
	VirtualProtect(src, sizeof(dst), PAGE_EXECUTE_READWRITE, &oldProtection);
	memcpy((byte*)shortcut(dst), src, sizeof(dst));
	VirtualProtect(src, sizeof(dst), oldProtection, &oldProtection);
}

BOOL WINAPI DllMain(HMODULE hModule, DWORD dwReason, LPVOID lpReserved)
{
	switch (dwReason)
	{
	case DLL_PROCESS_ATTACH:
		MessageBox(NULL, "DLL loaded!", "MyDLL", MB_ICONINFORMATION);
		CreatRoute((BYTE*)0x00E54D20, (BYTE*)codecave);
		break;
	case DLL_PROCESS_DETACH:
		MessageBox(NULL, "DLL unloaded!", "MyDLL", MB_ICONINFORMATION);
		break;
	default:
		break;
	}

	return TRUE;
}

[DarkstaR]

This line:

memcpy((byte*)shortcut(dst), src, sizeof(dst));

Is casting the RETURN value of shortcut(dst) to a byte and writing to that spot in memory. However, this write never happens, because shortcut() is jumping into dst directly. Except it's not actually even jumping into the code cave, because you're doing (BYTE*)codecave, which is passing a pointer to a byte at the start of codecave. The bytes being passed to shortcut(), then, are the first 4 bytes in the assembly code of the codecave function. It's jumping to the address it gets when it puts those bytes together, which is some garbage value.

Here's something that does what you want, strait out of the example code from my book:

template<typename T>
DWORD protectMemory(DWORD address, DWORD prot)
{
    DWORD oldProt;
    VirtualProtect((LPVOID)address, sizeof(T), prot, &oldProt);
    return oldProt;
}

template<typename T>
void writeMemory(DWORD address, T value)
{
    *((T*)address) = value;
}


DWORD hookWithJump(DWORD hookAt, DWORD newFunc, int size){
    if (size > 12) // shouldn't ever have to replace 12+ bytes
        return 0;
    DWORD newOffset = newFunc - hookAt - 5;


    auto oldProtection = protectMemory<DWORD[3]>(
        hookAt + 1,
        PAGE_EXECUTE_READWRITE);
    writeMemory<BYTE>(hookAt, 0xE9);
    writeMemory<DWORD>(hookAt + 1, newOffset);
    for (unsigned int i = 5; i < size; i++)
        writeMemory<BYTE>(hookAt + i, 0x90);
    protectMemory<DWORD[3]>(hookAt + 1, oldProtection);


    return hookAt + 5;
}

DWORD restoreJumpHook = 0;
void __declspec(naked) myTrampoline()
{
    __asm {
        PUSHFD
        PUSHAD
        CALL jumpHookCallback
        POPAD
        POPFD
        // assembly code you replaced should go here
        JMP [restoreJumpHook]
    }
}

void jumpHookCallback() { // do stuff }
restoreJumpHook = hookWithJump(0xDEADBEEF, &myTrampoline, 5); //5 is the number of bytes to replace. must be equal to size of ops you're replacing

[Healian]

This is amaaaaaaaazing piece of code, i really don't know how to thank you for your continuous support, i have studied your code, tried some things here and there and this is my code now. I commented out some questions that i hope you get the time to take a look at.

#include <iostream>
#include <string>
#include <Windows.h>

using namespace std;

void RealFunction()
{
	MessageBox(NULL, "RealFunction()", "Trace", MB_OK);
}


DWORD jmpBackAddr = 0x000000;
__declspec(naked) void CodeCave()
{
	__asm {
		pushad;
		pushfd;
	}

	MessageBox(NULL, "CodeCave()", "Trace", MB_OK);

	__asm{
		popfd;
		popad;
		jmp [jmpBackAddr];
	}
}

template<typename T>
DWORD protectMemory(DWORD address, DWORD prot)
{
	DWORD oldProt;
	VirtualProtect((LPVOID)address, sizeof(T), prot, &oldProt);
	return oldProt;
}

template<typename T>
void writeMemory(DWORD address, T value)
{
	*((T*)address) = value;
}


DWORD hookWithJump(DWORD hookAt, DWORD newFunc, int size){
	if (size > 12) // shouldn't ever have to replace 12+ bytes
		return 0;

	protectMemory<DWORD/*[3] >> Why ?*/>(hookAt, PAGE_EXECUTE_READWRITE); //If the current page is always 0x20 [PAGE_EXECUTE_READ] why we need to save the old value ?
	writeMemory<BYTE>(hookAt, 0xE9);
	writeMemory<DWORD>(hookAt + 1, (newFunc - hookAt - 5)); //As i understand we use -5 because the size of JMP instruction is 5, and now we are writing write after the JMP so we need to move past it, (like a little bit to the right :D) ?
	                                                        //Also what happens if the stack was going down instead of up or vice versa, would that newFunc - hookAt be the reverse in that situation ?
	//for (unsigned int i = 5; i < size; i++) //I think this code will never be excuted since we are passing 5, and `i` starts with 5. Wouldn't be more
	//	writeMemory<BYTE>(hookAt + i, 0x90); //If we somehow dynamically determined the target instruction's size, placed our jump then noped out the rest ?
	protectMemory<DWORD/*[3] >> Why ?*/>(hookAt + 1, 0x20);
	return hookAt + 5; //But Hook at is already overwritten right ? i think that will cause an infinite loop to the code cave ?, it gives me memory access violation btw but this is what i think will happen ?
}

void main()
{
	jmpBackAddr = hookWithJump((DWORD)RealFunction, (DWORD)CodeCave, 5);

	while (true)
	{
		cin.get();
		RealFunction();
	}
}

[DarkstaR]

1. We save the old value just in-case the page protection is different. It will generally be PAGE_EXECUTE_READ, but there's some niche cases where that might be changed for a reason. Also, it's good practice to use the pattern everywhere, as you might not always be working with executable memory, and it saves you from knowing "oh this type of memory only needs x protection".

2. My memory functions are template based, and use sizeof(T) to determine how many bytes to protect/read/write - this is because I generally create structures and classes that match the data being read or written. In this hook, though, the amount of data being replaced is variable depending on the OP-code length, so I just do DWORD[3], which evaluates to 12 bytes, as there's no reason you'll ever need to replace more than that.

3. The -5 is because the jump is relative to the NEXT instruction. Since the jump is 5 bytes, the offset is 5. So, yes, you're practically correct.

4. Not sure what your question about the stack "going down means"

5. In this case, the code will never be executed if the jump you're placing only replaces 5 bytes. Correct. But it can be up to 12. There's ways to dynamically determine instruction length, but I didn't want to teach you how to write a partial x86 disassembler

6. The value being returned is the address following the jump you placed. This is where you will jump to after your code cave to restore execution. If you're getting an access violation, it's because you're not executing the code that you replaced. (For example, if you replace CALL 0xDEADBEEF with your jump, then you need to do CALL 0xDEADBEEF right before jmp [jmpBackAddr])...



EDIT: Actually, on #6, you're right in that we've replaced the hook before storing the address. If the hook gets executed inbetween, you'll get an access violation. However, this shouldn't happen, as you should always sync with or freeze any threads that execute code you're hooking. Only near-call hooks and VF table hooks are atomic (as you only replace 4 bytes), so you should hook a function like PeekMessageA or Sleep and place your hooks from there. Else, you can freeze all threads while hooking.

[Healian]

1- So yeah you are passing an array of 3 DWORD(S) in order to simulate the value of 12 bytes as the DWORD on a 32-bit machine is equal to 4 bytes, Still I think you should have written 0xC instead right ? (Considering the same code running on a 64-bit machine)

2- Meh, i think i got confused about the -5 again... sorry (as i know is that jump address is relative to the current ESP (or the address of the jump instruction itself right ?))

3- This is what i mean by the stack growing up or down

direction of     |                                 |
  growth of      +---------------------------------+ 
   stack         | Parameters passed by fn1(caller)|
from higher addr.|                                 |
to lower addr.   | Direction of growth is opposite |
      |          |   to direction of stack growth  |
      |          +---------------------------------+ <-- SP on entry to fn2
      |          | Return address from fn2(callee) | 
      V          +---------------------------------+ 
                 | Callee saved registers being    | 
                 |   used in the callee function   | 
                 +---------------------------------+
                 | Local variables of fn2          |
                 |(Direction of growth of frame is |
                 | same as direction of growth of  |
                 |            stack)               |
                 +---------------------------------+ 
                 | Arguments to functions called   |
                 | by fn2                          |
                 +---------------------------------+ <- Current SP after stack 
                                                        frame is allocated

So depending on the figure above consider our stack is going down will `newFunc - hookAt` get us a negative value ? which may screw the formula of getting the target place up.

4- No, i am not seeking to learn this now , however i will look it up as i find it more neat to automatically determine the target instruction size/length.

I like your code style btw, it's neat and describes itself.

[DarkstaR]

1. Or I could have used BYTE[12]. but it's no so important because even DWORD[3] in x64 is large enough; it's just 24 instead of 12. Since memory protection persists for the whole page anyways, there's only a small chance that anything extra is protected (only if the extra 12 bytes spill over into a new page). Also, in the book, I'm only covering 32-bit processes, and most games will fit that description still. Once again, the reason I don't pass 0x0C is because it uses sizeof(). I could have done it differently, but that requires a different function and I had previously introduced readers to these functions already. Anyways, we can debate about this all day, bottom line is that it'll work either way and it's that way for a reason that reaches beyond this forum post.


2. Consider this:

0001: JMP somewhere
0006: RETN
0007: .....
0009: somewhere

The offset to "somewhere" needs to be relative to 0004, not 0001. So we do somewhere - jumpAt - 5 or somewhere - 0001 - 0005. If somewhere is 0009, the result is 0003. 0006 + 0003 = 0009.

3. newFunc - hookAt has nothing to do with the stack. The stack is used to store parameters and local variables, where these values are located in memory mapped directly from the PE header and aren't writable.

[Healian]

so assuming the following pseudo asm code:

0x000001: jmp 0x00000A
0x000002: push eax
0x000003: mov eax, 1
0x000004: sub eax, 1
0x00000A: pushfd

Why we don't just say jmp 0x00000A as i mentioned ? as i understand, when the processor come across an instruction it fetches it and the EIP is already pointing at the next instruction (although the ESP is virtually equals EIP- 1 STEP). So when we do calculate a relative jump we say Destination - (Current + sizeof(instruction)) the reason here for the + is because we have to calculate it relative to the JMP instruction address not the instruction that follows it right ?

2- But as far as i know the stack is involved in saving the function addresses as well as vtables and other data ?

3- The function in your code is returning currentAddress + 5 which you say is the address of the next instruction following the one we replaced with our codecave or trampoline jump... what bugs me here is that should't we add (5 + size of DWORD) because 5 is the jmp instruction size itself + the size of the address? or am i totally screwed here ?

4- Also in this code, why change protect at hookAt, and restore it at hookAt + 1 ?

protectMemory<DWORD[3]>(hookAt, PAGE_EXECUTE_READWRITE);
writeMemory<BYTE>(hookAt, 0xE9);
writeMemory<DWORD>(hookAt + 1, (newFunc - hookAt - 5));
protectMemory<DWORD[3]>(hookAt + 1, PAGE_EXECUTE_READ);

5- Your code works perfectly but i have an issue, when ever the jumpBack hit, i get memory access violation although it is an internal call to the current executable. What is the reason(s).

6- Allow me to add an edited version of your code as i love lambdas, also other members may benefit from it.

void RealFunction()
{
	MessageBox(NULL, "RealFunction()", "Trace", MB_OK);
}

void FakeFunction()
{
	MessageBox(NULL, "FakeFunction()", "Trace", MB_OK);
}


DWORD jmpBackAddr = 0;
__declspec(naked) void Trampoline()
{
	__asm {
		PUSHFD;
		PUSHAD;
		CALL FakeFunction;
		POPAD;
		POPFD;
		JMP [jmpBackAddr];
	}
}

template<typename T>
void writeMemory(DWORD address, T value)
{
	*((T*)address) = value;
}

template<typename T, typename F>
void editMemory(DWORD address, DWORD prot, F action)
{
	DWORD oldProt;
	VirtualProtect((LPVOID)address, sizeof(T), prot, &oldProt);
	action();
	VirtualProtect((LPVOID)address, sizeof(T), oldProt, NULL);
}

DWORD hookWithJump(DWORD hookAt, DWORD newFunc, int size)
{
	if (size > 12)
		throw new exception("size cannot exceed 12 bytes!");

	auto lambda = [target, trampoline, size]() {
		writeMemory<BYTE>(target, 0xE9);
		writeMemory<DWORD>(target + 1, (trampoline - target - 5));
		for (unsigned int i = 5; i < size; i++)
			writeMemory<BYTE>(target + i, 0x90);
	};

	editMemory<DWORD[3]>(hookAt, PAGE_EXECUTE_READWRITE, lambda);
	return hookAt + 5;
}

btw shouldn't the return hookAt + 5 be hookAt + size ? i mean to jump over all the nops we added

7- Also i don't think that your code consider's the replaces function's prologue and that's why i am getting the memory access violation because the function is being executed right after the trampoline without executing the prologue of it.

8- Sorry the post is so long but as i understand pretty much a lot of things to get me going testing on some game such as darkeden, i wonder how could i effectively do the testing and so because i find it very boring to build the dll, inject it, attach olly to debug then i find a bug and repeat all this steps... meh this is very time consuming and there must be an easier way. Also WHAT if i want to call a function directly from olly to test out (without having to inject the dll and all that staff)

[Darius]

First of all you need to understand the fact you can only safely replace 4 bytes (32bits) at a time in a 32bit process.
I won't go into too much detail but its about atomically changing operations without affecting other threads that may be executing that code.
For this reason most windows APIs are compiled with 5 NOPs previous to the function address followed by MOV EDI, EDI at the start of the function.
The MOV EDI, EDI is a 2 byte instruction and can only be replaced with another 2 byte instruction safely to make sure you don't corrupt the current execution of instructions.
(An interesting sidenote about Microsoft having MOV EDI, EDI instead of 0x90 (NOPs): MOV EDI, EDI is faster to execute. NOPs take a clockcycle to complete (Thus takes 2), while MOV EDI, EDI in practice only requires 1.5 clockcycles. You can read about it on MSDN.)

The way detour's works is to replace the MOV EDI, EDI with a relative jump back 7 bytes. This will take the execution path to the start of the NOPs.
So you first should replace the NOPs with a 32bit relative jump 0xE9 0xXX 0xXX 0xXX 0xXX and then replace the MOV EDI, EDI with 0xEB 0xF9 (JMP -7).
This will guarantee no screw ups.

Tibia's functions however, is not compiled with 5 NOPs previous to the function address, thus there is no MOV EDI, EDI to (0xE9) JMP back from and there is no 5 bytes to replace.
If you hook a specific function using above code, you'll overwrite the the first 5 bytes of the function which in turn makes it useless.
But if you're hooking a specific call, you're not doing it safe and you can't gurantee no screw ups. Instead you should not do a JMP (0xE9), you should rather do a (0xE8) CALL to make Tibia call your function, do your thing before you call the original address.
Thus no screw ups either since your essentially only replacing 4 bytes, the actual call address.

Codecaves is usually (in reverse engineering) a given set of 0x90s (the size of the OP codes you need to be written) inside the process / file, however, since you're injected and have your own function. You're essentially just hooking.

// dwAddress is the function call your hooking, dwFunction is your function.
// DWORD dwFunctionCallToHook = 0x00000000;
// HookCall(dwFunctionCallToHook, (DWORD)&MyFunction);
// 
// typedef void _TibiaFunction(void);
// _TibiaFunction *TibiaFunction;
// TibiaFunction = (_TibiaFunction*)dwFunctionCallToHook;

DWORD HookCall(DWORD dwAddress, DWORD dwFunction)
{
	DWORD dwOldProtect, dwOldCall, dwNewCall;
	BYTE callByte[5] = { 0xE8, 0x00, 0x00, 0x00, 0x00 };

	dwNewCall = dwFunction - dwAddress - 5;
	memcpy(&callByte[1], &dwNewCall, 4);

	VirtualProtectEx(GetCurrentProcess(), (LPVOID)(dwAddress), 5, PAGE_READWRITE, &dwOldProtect);
	memcpy(&dwOldCall, (LPVOID)(dwAddress + 1), 4);
	memcpy((LPVOID)(dwAddress), &callByte, 5);
	VirtualProtectEx(GetCurrentProcess(), (LPVOID)(dwAddress), 5, dwOldProtect, NULL);
	return dwOldCall;
}

// Your function which is CALLed at the hook CALL should then look like;
void MyFunction()
{
      // Do whatever.
      TibiaFunction(); // If the call has parameters, include those into 'MyFunction' and pass them along.
}