The function DetourCreateRemoteNative32 is thread safety? about scyllahide HOT 9 CLOSED

windbg output information:

0:000> kvn
 # ChildEBP RetAddr  Args to Child              
WARNING: Frame IP not in any known module. Following frames may be wrong.
00 00aff6d0 774307ec 772bdc3b 00000000 00aff718 0x0
01 00aff6d4 772bdc3b 00000000 00aff718 9e4c10c3 ntdll!NtDelayExecution+0xc (FPO: [2,0,0])
02 00aff73c 772bdbdf 00000064 00000000 00aff758 KERNELBASE!SleepEx+0x4b
03 00aff74c 003010a7 00000064 00aff7a0 00301800 KERNELBASE!Sleep+0xf
04 00aff758 00301800 00000001 00e63ca8 00e63ab0 ConsoleApplication1+0x10a7
05 00aff7a0 75070419 0093b000 75070400 00aff80c ConsoleApplication1+0x1800
06 00aff7b0 7742662d 0093b000 5226ccd2 00000000 KERNEL32!BaseThreadInitThunk+0x19 (FPO: [Non-Fpo])
07 00aff80c 774265fd ffffffff 774451e8 00000000 ntdll!__RtlUserThreadStart+0x2f (FPO: [SEH])
08 00aff81c 00000000 00301888 0093b000 00000000 ntdll!_RtlUserThreadStart+0x1b (FPO: [Non-Fpo])

from scyllahide.

First of all, no, DetourCreateRemoteNative32 is not thread safe. You can't safely call it from multiple threads because it does not use locks. I doubt this is really what you meant though :)

There are two problems here:

1. Hooks may be applied at the exact moment a thread in the debuggee is executing an instruction that is then (partially) overwritten by the hook. (Only true if attaching to a running process.)
2. If hooks are applied a second time (plugin options changed, or the CLI injector runs a second time), ScyllaHide reinjects the hook library DLL, and because a hook is already in place, it writes a jmp instruction pointing to garbage memory instead of the original WOW64 syscall transition.

(1) cannot truly be fixed as far as I can see. The problem can be mitigated somewhat by suspending the process before applying the hooks and then resuming it (this reduces the number of AV-generating states the application can be in while SH is changing memory properties), but if a thread happens to get suspended in the middle of executing a syscall stub, it will still crash the process when it is resumed. I guess technically it would be possible to detect such threads by checking their EIP/RIP with NtGetContextThread, but even if detected you still can't fix this. However, the chances of this race condition actually happening in practice are very small. I will still make a commit to suspend the process during injection because it reduces the chances of this happening and it is more correct anyway, but note that this will not fix the issue. The only way to truly fix this is to start an application debugged instead of attaching. This rules out using the CLI injector.

(2) can be fixed by doing proper unhooking of hooks already in place before reinjecting. This is also something that can only be done by a plugin, because the CLI injector doesn't know about any past injections by plugins or previous versions of itself. How much work this would be to fix, I'm not sure. I think this doesn't affect x64, only WOW64 and maybe native x86, since they both use a shared syscall gateway that may or may not be restored correctly. I will add some primitive hook detection to at least prevent a crash, but this also isn't a true fix and more of a bandaid. Note that situation (1) can occur when restoring hooks just like it can occur when applying them.

TLDR: debugger plugins are preferable over the CLI injector, starting in a debugger is preferable to attaching, and avoid changing options/reapplying hooks at runtime if you can. The last one should still be OK on x64 processes though.

from scyllahide.

i already compiled the latest commit, and it still crashes ,

the details about the bug reproduce video(pwd: 123):

http://s000.tinyupload.com/index.php?file_id=09317845334746442175

i use InjectorCLI.exe to anti-anti debug because i wanna use windbg to debug target process

from scyllahide.

I know the commit didn't fix the issue, otherwise I would have closed it. Read (2) again.

from scyllahide.

... i notice this😂, i just use injectorclI.exe inject program once , the x64dbg no scyllaHide plugin(it's clean version) and i'm sure no any hooked in ConsoleApplication1.exe

from scyllahide.

Wow, I'm impressed you managed that. I did reproduce this eventually without having to reinject (after trying for a long time...), but it is definitely hard to do. The reason I couldn't reproduce this was because I wasn't debugging ScyllaHide.

1. The debuggee must be running at the time of injection (you get a free breakpoint on attach, this is when you should inject ideally)
2. DetourCreateRemoteNative32 must have been called at least once by the injector so that the syscall gateway is hooked
3. Some thread in the debuggee must be executing a syscall stub (which is now rerouted to HookedNativeCallInternal)
4. The injector may not have written the hook DLL data to the process yet.

If above conditions are met, HookDllData.NativeCallContinue will be NULL, and when the debuggee executes jmp HookDllData.NativeCallContinue an AV will obviously follow.

So this is a race condition that is very hard to pull off... unless you are stepping through ScyllaHide in Visual Studio like in your video, while the process is running and executing a Sleep(1); loop ;)

I moved the suspend/resume region so that both the hooking and the writing of hook DLL data happen while the process is suspended. It now Works For Me (TM). Let me know if this fixes it for you - I'm guessing this is a reduced test case of a real app. Leaving this open for now because issue (2) from my first post is not fixed.

from scyllahide.

this fixes is great for me 👍

from scyllahide.

because issue (2) is not fixed i haven't close issue immediately

from scyllahide.

It turns out the combination of the first and second commits (detect already installed hooks, and suspend the process for the duration of the entire injection procedure) fixed issue (2) as well and there is no more crash on reinjection. The CLI still can't inject a second time since it never unhooks, but this is by design. Closing this.

from scyllahide.