Uninjection thread safety

c++ / delphi package - dll injection and api hooking
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Uninjection thread safety

Post by EaSy »

Hi,
we have experienced an issue with uninjection. We use a little hack to force MCH to uninject apps as we please:

Code: Select all

bool UnInjectFrom(const wchar_t *LibraryFileName, const wchar_t *ProcessNameMask)
{
	//MCH uses UninjectLibraryW differently than we originally thought

	//first we dont want to inject running processes
	SetMadCHookOption(INJECT_INTO_RUNNING_PROCESSES, (LPCWSTR)0);

	//first we must create phony rule in MCH driver, 
	//so MCD does't fail during uninjection...
	InjectLibraryW(CINJ_DRIVER_NAME, LibraryFileName, ALL_SESSIONS,
		true, ProcessNameMask);

	SetMadCHookOption(INJECT_INTO_RUNNING_PROCESSES, (LPCWSTR)1);

	//now we can uninject library from all processes with same name mask
	return (
		UninjectLibraryW(CINJ_DRIVER_NAME, LibraryFileName, ALL_SESSIONS,
		true, ProcessNameMask) != FALSE
		);
}
Everything works fine until we call this multiple times per process. This is because MCH uninjection routime works with GetModuleHandle result and it could became corrupted if there are multiple uninjection threads running.

The crash usually looks like this:

Code: Select all


FAULTING_IP: 
ntdll!LdrpUpdateLoadCount2+4d
77099820 0fb74638        movzx   eax,word ptr [esi+38h]

EXCEPTION_RECORD:  ffffffff -- (.exr 0xffffffffffffffff)
ExceptionAddress: 77099820 (ntdll!LdrpUpdateLoadCount2+0x0000004d)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: 41005438
Attempt to read from address 41005438

DEFAULT_BUCKET_ID:  INVALID_POINTER_READ

PROCESS_NAME:  CSISYNCCLIENT.EXE

ERROR_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s.

EXCEPTION_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s.

EXCEPTION_PARAMETER1:  00000000

EXCEPTION_PARAMETER2:  41005438

READ_ADDRESS:  41005438 

FOLLOWUP_IP: 
ntdll!LdrpUpdateLoadCount2+4d
77099820 0fb74638        movzx   eax,word ptr [esi+38h]

MOD_LIST: <ANALYSIS/>

APPLICATION_VERIFIER_FLAGS:  0

FAULTING_THREAD:  000007e4

PRIMARY_PROBLEM_CLASS:  INVALID_POINTER_READ

BUGCHECK_STR:  APPLICATION_FAULT_INVALID_POINTER_READ

LAST_CONTROL_TRANSFER:  from 7709987a to 77099820

STACK_TEXT:  
09bef9dc 7709987a 00000000 00000002 6bd40000 ntdll!LdrpUpdateLoadCount2+0x4d
09bef9f8 770a3aea 00000000 00000002 7ea38353 ntdll!LdrpUpdateLoadCount2+0xff
09befa78 770a3c45 6bd40000 09befa9c 7ea38397 ntdll!LdrpUnloadDll+0x9c
09befabc 74d92d32 6bd40000 00000000 09befe88 ntdll!LdrUnloadDll+0x4a
09befacc 71af020b 6bd40000 00000000 00000000 KERNELBASE!FreeLibrary+0x15
WARNING: Frame IP not in any known module. Following frames may be wrong.
09befe88 76a7337a 71ae0000 09befed4 770992e2 0x71af020b
09befe94 770992e2 71ae0000 7ea387ff 00000000 kernel32!BaseThreadInitThunk+0xe
09befed4 770992b5 71af0000 71ae0000 00000000 ntdll!__RtlUserThreadStart+0x70
09befeec 00000000 71af0000 71ae0000 00000000 ntdll!_RtlUserThreadStart+0x1b
What we propose is to introduce some process-wide named MUTEX or another kind of thread safety lock, that will ensure that multiple running unhook routines will not corrupt or crash application because of thread concurency in GetModuleHandle and FreeLibrary.

What do you think?

PP
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

Hmmmm... Interesting hack... :wink:

Just wondering: Why would you perform uninjection from multiple threads at the same time? Sounds a bit weird to me.

Anyway, of course it's easy enough to add some protection for this. I'd suggest I'd simply synchronize UninjectLibrary calls by using a critical section. Should suffice, no?
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

It won't help in some cases when an app is waiting for some hooks to finish orig call like DeviceIoControl. InjectLibraryW will timeout but the remotethead will be still alive.
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

Are you using a reasonably recent madCodeHook version? In Hooking.cpp search for:

"only the first uninjection thread gets to actually perform unhooking"

Basically that code should make sure that if multiple uninject threads are running at the same time, only the first one will try to unhook the APIs. All further threads will simply exit, without trying to unhook and without trying to unload the dll.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

Ah,
you are right. There is some mechanism in place, but it is not enough. The threads can still call the FreeLibrary concurently.

ThreadA gets ModuleHandle, successfully opens FileMapping and calls pfnAutoUnhook. ThreadA calls FreeLibrary and it is done.
ThreadB calls ModuleHandle. Then it is suspended right after the succesful GetModuleHandle call. After the threadA finishes and threadB continues with execution. It is not able to open FileMapping, so it will never call pfnAutoUnhook, so it will never ExitThread. Later, it calls FreeLibrary on no more alive module handle. This causes read of nonexisting memory and we get the crash like it is in dump.

PP
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

Any progress?

PP
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

Later, it calls FreeLibrary on no more alive module handle. This causes read of nonexisting memory and we get the crash like it is in dump.
Wouldn't a VirtualQuery call to check if HMODULE is at least of MEM_IMAGE type make sense directly before the call to FreeLibrary, perhaps even more validation would be needed (and possibly synchronization)? I don't have MCH source so pardon me if I am unaware of how MCH deals with this situation. Not a perfect solution but safer?

--Iconic
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

To be honest, I'm a bit confused about the problem. There's a "loader lock" which AFAIK has the exact purpose of making module initialization and module related APIs thread safe. So why would calling FreeLibrary from multiple threads be a problem?

Of course I could call VirtualQuery() first, but if it's a multi-threading issue, then VirtualQuery() in thread A might still succeed and then FreeLibrary() crash in thread A, if thread B frees the DLL right between the calls of thread A to VirtualQuery() and FreeLibrary()
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

I think the main thing here is that GetModuleHandle can be called from multiple threads concurrently and the fact that time elapses and another thread can unhook and free the DLL, the module handle is no longer valid when its turn comes to FreeLibrary. Seems that a critical section may be needed in place before GetModuleHandle is called, assuming that the thread that handles unhooking and uninjection has completed its work then GetModuleHandle() would return NULL instead of allowing multiple threads to get the imagebase and sit on it for whatever amount of time.
in some cases when an app is waiting for some hooks to finish orig call like DeviceIoControl. InjectLibraryW will timeout but the remotethead will be still alive
I think that the above information can compound this issue. It's strange that UnInjectLibrary() would be called from multiple threads at the same time, though :o This seems more like a stress test than a real-world scenario

--Iconic
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

I guess my point is that most win32 APIs are robust against incorrect parameters. So why would FreeLibrary() crash if you call it with a module handle which is no longer valid? FreeLibrary() should simply return with an error value. At least that's what I would expect...
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

It really shouldn't "crash" per se. Actually, if you did a test such as h := LoadLibrary(DLL); FreeLibrary(h); Sleep(1000); FreeLibrary(h); GetLastError() should return 0x0000007E (specified module could not be found) since it's already unloaded. Application Verifier would treat this as a bug and complain however iirc

--Iconic
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

I did more research and madshi is right calling FreeLibrary doesn't crash app. But I found out that if you meddle with dll lock count (like setting it to "dll->LoadCount = 0xff;") it will cause a crash because
1] First thread starts unhooking thread and set it to 1
2] Second thread sets it to 0xFF again (The 0xFF is not static BTW, a 0xFFFF is!)
3] First thread starts to call FreeLibrary (Calls on hook DLL are fine, but it also updates reference counts of dependent DLLs!! Hook DLL had 0xFC reference count, but according to call stack it crashed on dependent dll.)
madshi
Site Admin
Posts: 10753
Joined: Sun Mar 21, 2004 5:25 pm

Re: Uninjection thread safety

Post by madshi »

Second thread setting it to 0xFF could only happen if you do injection and uninjection at the same time. Are you doing that?

I'm locking the loader lock myself when modifying the lock count. Shouldn't that protect from crashes?

I'm still not totally sure I understand why the crash occurs. Do you fully understand the reasons?
iconic
Site Admin
Posts: 1065
Joined: Wed Jun 08, 2005 5:08 am

Re: Uninjection thread safety

Post by iconic »

(UINT16)-1 / 0xFFFF represented a statically loaded module for Windows OS versions pre Windows 8. In Windows 8 and above the loader structures changed and the "real" load count field, used by the OS, is now (UINT32)-1 / 0xFFFFFFFF (see LDR_DDAG_NODE->LoadCount) to reflect a static module. That "old" load count value in the LDR_DATA_TABLE_ENTRY for a module is obsolete and not used anymore in Windows 8+ @ in response to
The 0xFF is not static BTW, a 0xFFFF is!
by EaSy

--Iconic
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: Uninjection thread safety

Post by EaSy »

Well,
we do have some kind of mechanism of choosing injected and uninjected apps dynamically. In some rare cases Windows could cause the injecting thread to be delayed until the uninjecting thread is started or the uninjecting thread is blocked by AutoUnhook until some hooks finish... especially during the mass hooking when the targeted app is suspended, or the machine is under the heavy load... like running multiple virtual machines? It is rare, but it do happens. I've seen at least three dumps from three different apps looking similar.

The problem is:

We have hooking DLL. It has some dependencies, for example named SUBDLL.
MCH loads DLL into the process by LoadLibrary. (DLL 1, SUBDLL 1)
MCH then changes DLL's load count (DLL 0xFF, SUBDLL 1)
Later.
Thread1 starts unloading and modifies refcount. (DLL 1, SUBDLL 1)
Thread1 is blocked for example in AutoUnhook on some DeviceIOControlHook, that takes a long time to finish.
Thread2 starts loading again and modifies refcount (DLL 0xFF, SUBDLL 1)
Thread1 continues, because DeviceIOControl finished.
Thread1 calls FreeLibrary. (DLL 0xFE, SUBDLL 0 - SUBDLL is unloaded)
Thread1 calls FreeLibrary in cycle (DLL 0xFC, SUBDLL -1? crash).
...

PP
Post Reply