need help fixing crashing hooks

[boredevil]

I seem to have a problem with my hooks. It´s all working fine on my computers, while it´s always crashing for a friend of mine.

I´m sorry to not provide the error message. I will edit this post, if he manages to send it to me.

the hook gets hit, my hook function gets executed, but it crashes when calling the backed up code. and i don´t understand why.
maybe i overlooked something obvious.

here is the log with the dumped hook:

01/20/12 17:56:04   DEBUG_1   Hook :  Creating and applying LoadLibraryA hook
01/20/12 17:56:04   DEBUG_4   applyDetour : Dumping SafeSpot before modification: 
0x775e4bc6  mov edi, edi
0x775e4bc8  push ebp
0x775e4bc9  mov ebp, esp
0x775e4bcb  cmp dword ptr [ebp+08h], 00000000h
0x775e4bcf  push ebx
0x775e4bd0  push esi
0x775e4bd1  push edi

01/20/12 17:56:04   DEBUG_4   applyDetour : Dumping backed up instructions  before modification: 
0x388d00  mov edi, edi
0x388d02  push ebp
0x388d03  mov ebp, esp
0x388d05  cmp dword ptr [ebp+08h], 00000000h

01/20/12 17:56:04   DEBUG_4   applyDetour : Dumping backed up instructions after fixing relative Instructions: 
0x388d00  mov edi, edi
0x388d02  push ebp
0x388d03  mov ebp, esp
0x388d05  cmp dword ptr [ebp+08h], 00000000h
0x388d09  jmp 775E4BCFh

01/20/12 17:56:04   DEBUG_4   applyDetour : Dumping Hooked Function 
0x775e4bc6  push 70F913DCh
0x775e4bcb  ret 
0x775e4bcc  nop 
0x775e4bcd  nop 
0x775e4bce  nop 
0x775e4bcf  push ebx
0x775e4bd0  push esi
0x775e4bd1  push edi

01/20/12 17:56:04   DEBUG_1   DllMain returning
01/20/12 17:56:04   DEBUG_1   myLoadLibrary :  calling original LoadLibraryA at 0x388d00 with kernel32.dll

typedef HMODULE (WINAPI* 	 tLoadLibrary)(const char * c);
tLoadLibrary 	g_oLoadLibrary = nullptr;

HMODULE WINAPI myLoadLibrary(const char * c)
{
	std::string s = c;
	HMODULE hMod = NULL;

	DBG << FKT << " calling original LoadLibraryA at " << (void*)g_oLoadLibrary << " with " << s << std::endl;

	hMod = g_oLoadLibrary(c); // <------- Crashing....

[Bananenbrot]

Did you check memory page protection flags? Is your generated code executable?

[boredevil]

i´m just allocating the momeory with new. i assumed it should be accessible afterwards without problems.
should i manually set the rights?

here is what i do:

/**
 *  Asm Stuff
 */
#pragma pack(1)
class Instruction
{
public:
	char 			opcode;
};
#pragma pack()

#pragma pack(1)
class AsmJmp : public Instruction
{
public:
	void Apply(unsigned long addr)
	{
		opcode = 0xE9;
		relativeAddr = addr;
	}
	unsigned long 	relativeAddr;
};
#pragma pack()


void* CPatchDetour::applyDetour(void *pTarget, void *pHook)
{
	size_t cpySize = 0;
	char* pSrc = reinterpret_cast<char*>(pTarget);

	pSrc = findSafeSpot(pSrc);   // working as intended
	m_pTarget = pSrc;

	cpySize = getNeededSize(pSrc); // working as intended
	m_Len = cpySize;

	LOG_(log_debug4) << FKT  << "Dumping SafeSpot before modification: " << std::endl
					 << toAsmString(pSrc,cpySize) 				  		<< std::endl;

	// allocate mem and backup original instructions
	m_pOriginal = std::tr1::shared_ptr<char>(new char[cpySize + size_jmp]);
	memcpy(m_pOriginal.get(), pSrc , cpySize);         // looking at log seems to write stuff as intended


	LOG_(log_debug4)  << FKT  << "Dumping backed up instructions before modification: " << std::endl
					  << toAsmString(m_pOriginal.get(),cpySize) 			<< std::endl;


	fixRelativeInstructions(m_pOriginal.get(), pSrc ,cpySize);     // working as intended

	// append jump back to Target Instruction
	reinterpret_cast<AsmJmp*>( cpySize + m_pOriginal.get() )
			->Apply( (DWORD)pSrc - (DWORD)(m_pOriginal.get() + size_jmp)  );   // looking at log seems to also write what it´s supposed to do


	LOG_(log_debug4)  << FKT  << "Dumping backed up instructions after fixing relative Instructions: " << std::endl
					  << toAsmString(m_pOriginal.get(),cpySize) 				   		   << std::endl;


	setupHook(pSrc, cpySize, (char*)pHook);  // also working


	LOG_(log_debug4)  << FKT  << "Dumping Hooked Function " << std::endl
					  << toAsmString(pSrc,cpySize)  		<< std::endl;


	return m_pOriginal.get();
}

void CPatchDetour::setupHook(char* pTarget, size_t size, char* pHook)
{
	// write our Hook code to target function
	VirtualProtectBlock tarProtect(pTarget, size, PAGE_EXECUTE_READWRITE );

	// write push addr ret
	reinterpret_cast<AsmPushRet*>(pTarget)
			->Apply((unsigned long)pHook);

	// nop the rest
	for( size_t i = size_push_ret; i < size ; i++)
	{	// nop the rest
		reinterpret_cast<char*>(pTarget)[i] = 0x90; //nop
	}
}

ugly, but doing the job, at least on my machine and i don´t want to rewrite till i know what´s wrong.

[boredevil]

you seem to be right. the region only got readwrite access


01/30/12 12:23:59 DEBUG_4 dumpHook : Protection backed up instr:
BaseAddress: 0x548000
AllocationBase: 0x540000
AllocationProtec: 4
RegionSize: 4096
State: 4096
Protect: 4
Type: 131072

this dump is from my computer. but i´m a little bit confused, why it´s crashing on his pc but not on my...
i´ll wait till he gets online, so he can try with adjusted access.

edit:
after reading up on msdn and turning on dep support in bios i was finally able to reproduce the error.

big thanks for pointing me in the right direction!

[Cypher]

Don't allocate memory for your hook and/or trampoline with 'new'. Use VirtualAlloc/VirtualAllocEx with the correct page flags (PAGE_READWRITE when you're setting up your hook, then PAGE_EXECUTE once the data is written and you're ready to detour your target function). Not only will that fix your DEP problem, but it will mean you're not opening up a security hole in your application (protection flags are set with a page-level granularity, so you may be modifying - and 'relaxing' from a security perspective - the flags of memory that you don't 'own'!).

(If you're going to be setting lots of hooks you're probably going to want to use the same page for multiple hooks, but if you're only setting one or two it's probably easier to just use a page per hook.)

EDIT:

To make it clear, you obviously don't want to 'solve' this problem by requiring DEP to be disable for the target process. Too many people do that, please don't join them or I'll have to hate you. VirtualProtect is your friend!

[boredevil]

thanks for the explanation.

turning off dep wasn´t an option after reading up on it. i wondered, why it´s not enabled by default in my bios.
i fixed it using VirtualProtect to set the correct protection.
what you say about VirtualAlloc seems pretty reasonable. going to read up on it now and rewrite stuff.

[Cypher]

thanks for the explanation.

turning off dep wasn´t an option after reading up on it. i wondered, why it´s not enabled by default in my bios.
i fixed it using VirtualProtect to set the correct protection.
what you say about VirtualAlloc seems pretty reasonable. going to read up on it now and rewrite stuff.

Yes, please use VirtualAlloc instead of 'new'. Making the heap executable is bad mojo.