Bug in NtQueryInformationProcess hook

[mrexodia]

https://forum.tuts4you.com/topic/40011-debugme-vmprotect-312-build-886-anti-debug-method-improved/#comment-192827

Affected functions:

• NtQuerySystemInformation
• NtQueryObject
• NtQueryInformationProcess

Basically in the kernel these functions put the ReturnLength = x after handling the rest so if we replace the actual result value where ReturnLength == ResultPointer it will return a different value.

[Mattiwatti]

Very interesting! In principle this can be fixed by checking if xxxInformation == ReturnLength and not acting if so. But this could still be circumvented by calling e.g. NtQuerySystemInformation with SystemProcessInformation and embedding ReturnLength somewhere in the output buffer (a bit far fetched maybe, but not that hard to do).

if (ReturnLength != nullptr &&
  (ULONG_PTR)ReturnLength >= (ULONG_PTR)xxxInformation &&
  (ULONG_PTR)ReturnLength <= (ULONG_PTR)xxxInformation + *ReturnLength))
{
  // it's a trick!
}

Does that catch everything?

[mrexodia]

My current solution in TitanHide is to just mimic the kernel and set the ReturnLength directly after the other buffers are changed...

Also, *ReturnLength in usermode might throw and I'm not sure how it's handled in ScyllaHide...

[Mattiwatti]

Yeah, assuming all of the NtQuery functions follow the same pattern of

1. Write to output buffer
2. If ReturnLength present, write to ReturnLength inside a try/catch block

that works for TitanHide.

ScyllaHide can't use SEH like TitanHide though, at least not the way it is currently mapped, so we can't just do a hail mary and try to write to random pointers like the kernel can (at least not in 64 bit mode - might work on x86). We would have to add a stub to call RtlAddInvertedFunctionTable to get SEH to work inside HookLibrary. At that point I think it would be easier to just add a ULONG TempReturnLength = *ReturnLength (assuming above criteria match), do our hook stuff and write back again with *ReturnLength = TempReturnLength.

[mrexodia]

Yeah I think for ScyllaHide the check is appropriate, assuming the kernel has already checked the ReturnLength and returns successfully it should work fine...

For TitanHide I think I'm going to write an abomination like this:

static NTSTATUS NTAPI HookNtQuerySystemInformation(
    IN SYSTEM_INFORMATION_CLASS SystemInformationClass,
    OUT PVOID SystemInformation,
    IN ULONG SystemInformationLength,
    OUT PULONG ReturnLength OPTIONAL)
{
    NTSTATUS ret = Undocumented::NtQuerySystemInformation(SystemInformationClass, SystemInformation, SystemInformationLength, ReturnLength);
    if(NT_SUCCESS(ret) && SystemInformation)
    {
        ULONG pid = (ULONG)PsGetCurrentProcessId();
        if(SystemInformationClass == SystemKernelDebuggerInformation)
        {
            if(Hider::IsHidden(pid, HideSystemDebuggerInformation))
            {
                ULONG TempReturnLength = 0;
                __try
                {
                    if(ARGUMENT_PRESENT(ReturnLength))
                        TempReturnLength = *ReturnLength;
                }
                __except(EXCEPTION_EXECUTE_HANDLER)
                {
                    //NOTE: this could probably happen if some race condition is introduced
                    //possibly: return GetExceptionCode(); ???
                }
                Log("[TITANHIDE] SystemKernelDebuggerInformation by %d\r\n", pid);
                typedef struct _SYSTEM_KERNEL_DEBUGGER_INFORMATION
                {
                    BOOLEAN DebuggerEnabled;
                    BOOLEAN DebuggerNotPresent;
                } SYSTEM_KERNEL_DEBUGGER_INFORMATION, *PSYSTEM_KERNEL_DEBUGGER_INFORMATION;
                SYSTEM_KERNEL_DEBUGGER_INFORMATION* DebuggerInfo = (SYSTEM_KERNEL_DEBUGGER_INFORMATION*)SystemInformation;
                __try
                {
                    DebuggerInfo->DebuggerEnabled = false;
                    DebuggerInfo->DebuggerNotPresent = true;
                    if(ARGUMENT_PRESENT(ReturnLength))
                        *ReturnLength = TempReturnLength; //restore the ReturnLength
                }
                __except(EXCEPTION_EXECUTE_HANDLER)
                {
                    ret = GetExceptionCode();
                }
            }
        }
    }
    return ret;
}

But it just smells like race condition bugs...