Improve log args for a few hooks: #94
Conversation
- Log Process ID in NtReadVirtualMemory/NtWriteVirtualMemory/ReadProcessMemory/WriteProcessMemory if the process handle used does not represent the current running process - Remove a dupe arg from NtOpenThread - Better logging for NtGetContextThread; logs have appropriate arguments based on the context flags - Cloned this logic to NtSetContextThread as well - Added PID/TID to NtGet/SetContextThread Changes inspired by trying to detect https://research.checkpoint.com/2025/waiting-thread-hijacking/ I've found it's quite difficult to keep track of all all of the handles being mapped to various identifiers (PID/TID) from the various APIs in a signature without these improvements.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @KillerInstinct, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on improving the logging capabilities of several hook functions related to process and thread memory access. The primary goal is to enhance the ability to track handles and identifiers (PID/TID) across different APIs, which is crucial for detecting advanced exploitation techniques like thread hijacking. The changes involve adding process IDs to log messages when the process handle used doesn't represent the current running process, removing duplicate arguments from NtOpenThread logging, and providing more detailed logging for NtGetContextThread and NtSetContextThread based on context flags. The changes affect hook_process.c and hook_thread.c.
Highlights
- Enhanced Logging for Memory Access Hooks: The
NtReadVirtualMemory,ReadProcessMemory,NtWriteVirtualMemory, andWriteProcessMemoryhooks now log the process ID when the handle refers to a different process than the current one. This helps in identifying cross-process memory operations. - Improved Thread Context Logging: The
NtGetContextThreadandNtSetContextThreadhooks now log relevant register values (Instruction Pointer, RAX, RBX, RCX, RDX, RSP) and process/thread IDs based on the context flags, providing more detailed information for debugging and security analysis. - NtOpenThread Logging: The logging for
NtOpenThreadhas been corrected to remove a duplicate argument.
Changelog
- hook_process.c
- Modified
NtReadVirtualMemoryto log the process ID if the process handle doesn't belong to the current process (lines 901-912). - Modified
ReadProcessMemoryto log the process ID if the process handle doesn't belong to the current process (lines 937-947). - Modified
NtWriteVirtualMemoryto log the process ID if the process handle doesn't belong to the current process (lines 977-988). - Modified
WriteProcessMemoryto log the process ID if the process handle doesn't belong to the current process (lines 1025-1036).
- Modified
- hook_thread.c
- Removed a duplicate argument from the
LOQ_ntstatuscall inNtOpenThread(lines 289-290). - Enhanced logging in
NtGetContextThreadto include register values and process/thread IDs based on context flags (lines 309-385). - Enhanced logging in
NtSetContextThreadto include register values and process/thread IDs based on context flags (lines 440-516).
- Removed a duplicate argument from the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A handle's a key,
To memory's domain we see,
Logs whisper the tale,
Of reads that prevail,
In process's vast decree.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request aims to enhance the logging capabilities of several hook functions, providing more detailed information for debugging and analysis, particularly in the context of thread hijacking detection. The changes generally improve the clarity and usefulness of the logs. However, there are some areas where the logic can be simplified and potential issues addressed.
Summary of Findings
- Redundant logging in conditional blocks: The
LOQ_*macros are called in both theifandelseblocks whenpid != GetCurrentProcessId(). This results in redundant logging, as the only difference is the inclusion of theProcessIdargument in theifblock. This can be simplified by moving theLOQ_*call outside the conditional and conditionally including theProcessIdargument. - Incorrect function call in NtWriteVirtualMemory/WriteProcessMemory: The code uses
GetCurrentProcess()instead ofGetCurrentProcessId()in the conditional check forNtWriteVirtualMemoryandWriteProcessMemory. This is incorrect asGetCurrentProcess()returns a pseudo-handle, not the process ID. This needs to be corrected to ensure the conditional logic works as intended. - Inconsistent logging format in ReadProcessMemory: The
LOQ_boolmacro inReadProcessMemoryincludes theProcessIdas the last argument, while the other memory read/write functions include it earlier in the argument list. This inconsistency should be addressed for better readability and maintainability.
Merge Readiness
The pull request introduces valuable logging improvements that aid in debugging and analysis. However, the identified issues regarding redundant logging, incorrect function calls, and inconsistent formatting should be addressed before merging. I am unable to directly approve this pull request, and other reviewers should also review this code before merging. At a minimum, the high severity issues should be addressed before merging.
hook_process.c
Outdated
| if (pid != GetCurrentProcess()) { | ||
| LOQ_ntstatus( |
There was a problem hiding this comment.
The code uses GetCurrentProcess() instead of GetCurrentProcessId(). GetCurrentProcess() returns a pseudo-handle, not the process ID. This is incorrect.
| if (pid != GetCurrentProcess()) { | |
| LOQ_ntstatus( | |
| if (pid != GetCurrentProcessId()) { |
hook_process.c
Outdated
| @@ -970,8 +1024,27 @@ HOOKDEF(BOOL, WINAPI, WriteProcessMemory, | |||
|
|
|||
| pid = pid_from_process_handle(hProcess); | |||
|
|
|||
| LOQ_bool("process", "ppBhs", "ProcessHandle", hProcess, "BaseAddress", lpBaseAddress, | |||
| "Buffer", lpNumberOfBytesWritten, lpBuffer, "BufferLength", *lpNumberOfBytesWritten, "StackPivoted", is_stack_pivoted() ? "yes" : "no"); | |||
| if (pid != GetCurrentProcess()) { | |||
There was a problem hiding this comment.
The code uses GetCurrentProcess() instead of GetCurrentProcessId(). GetCurrentProcess() returns a pseudo-handle, not the process ID. This is incorrect.
| if (pid != GetCurrentProcess()) { | |
| if (pid != GetCurrentProcessId()) { |
| if (pid != GetCurrentProcessId()) { | ||
| LOQ_ntstatus( | ||
| "process", "piphB", | ||
| "ProcessHandle", ProcessHandle, | ||
| "ProcessId", pid, | ||
| "BaseAddress", BaseAddress, | ||
| "Size", NumberOfBytesToRead, | ||
| "Buffer", NumberOfBytesRead, Buffer | ||
| ); | ||
| } | ||
| else { | ||
| LOQ_ntstatus( | ||
| "process", "piphB", | ||
| "ProcessHandle", ProcessHandle, | ||
| "BaseAddress", BaseAddress, | ||
| "Size", NumberOfBytesToRead, | ||
| "Buffer", NumberOfBytesRead, Buffer | ||
| ); | ||
| } |
There was a problem hiding this comment.
The LOQ_ntstatus calls are identical in both the if and else blocks, except for the inclusion of ProcessId in the if block. This redundancy can be eliminated by moving the LOQ_ntstatus call outside the conditional and conditionally including the ProcessId argument in the format string.
| if (pid != GetCurrentProcessId()) { | |
| LOQ_ntstatus( | |
| "process", "piphB", | |
| "ProcessHandle", ProcessHandle, | |
| "ProcessId", pid, | |
| "BaseAddress", BaseAddress, | |
| "Size", NumberOfBytesToRead, | |
| "Buffer", NumberOfBytesRead, Buffer | |
| ); | |
| } | |
| else { | |
| LOQ_ntstatus( | |
| "process", "piphB", | |
| "ProcessHandle", ProcessHandle, | |
| "BaseAddress", BaseAddress, | |
| "Size", NumberOfBytesToRead, | |
| "Buffer", NumberOfBytesRead, Buffer | |
| ); | |
| } | |
| DWORD pid = pid_from_process_handle(ProcessHandle); | |
| LOQ_ntstatus( | |
| "process", pid != GetCurrentProcessId() ? "piphB" : "pphB", | |
| "ProcessHandle", ProcessHandle, | |
| pid != GetCurrentProcessId() ? "ProcessId" : NULL, pid != GetCurrentProcessId() ? pid : 0, | |
| "BaseAddress", BaseAddress, | |
| "Size", NumberOfBytesToRead, | |
| "Buffer", NumberOfBytesRead, Buffer | |
| ); |
| if (pid != GetCurrentProcessId()) { | ||
| LOQ_bool( | ||
| "process", "pphB", | ||
| "ProcessHandle", hProcess, | ||
| "BaseAddress", lpBaseAddress, | ||
| "Size", nSize, | ||
| "Buffer", lpNumberOfBytesRead, lpBuffer, | ||
| "ProcessId", pid | ||
| ); | ||
| } | ||
| else { | ||
| LOQ_bool( | ||
| "process", "pphB", | ||
| "ProcessHandle", hProcess, | ||
| "BaseAddress", lpBaseAddress, | ||
| "Size", nSize, | ||
| "Buffer", lpNumberOfBytesRead, lpBuffer, | ||
| "ProcessId", pid | ||
| ); |
There was a problem hiding this comment.
Similar to the previous comment, the LOQ_bool calls are redundant. Move the call outside the conditional and conditionally include the ProcessId argument.
| if (pid != GetCurrentProcessId()) { | |
| LOQ_bool( | |
| "process", "pphB", | |
| "ProcessHandle", hProcess, | |
| "BaseAddress", lpBaseAddress, | |
| "Size", nSize, | |
| "Buffer", lpNumberOfBytesRead, lpBuffer, | |
| "ProcessId", pid | |
| ); | |
| } | |
| else { | |
| LOQ_bool( | |
| "process", "pphB", | |
| "ProcessHandle", hProcess, | |
| "BaseAddress", lpBaseAddress, | |
| "Size", nSize, | |
| "Buffer", lpNumberOfBytesRead, lpBuffer, | |
| "ProcessId", pid | |
| ); | |
| DWORD pid = pid_from_process_handle(hProcess); | |
| LOQ_bool( | |
| "process", pid != GetCurrentProcessId() ? "pphB" : "pphB", | |
| "ProcessHandle", hProcess, | |
| "BaseAddress", lpBaseAddress, | |
| "Size", nSize, | |
| "Buffer", lpNumberOfBytesRead, lpBuffer, | |
| pid != GetCurrentProcessId() ? "ProcessId" : NULL, pid | |
| ); |
| LOQ_ntstatus( | ||
| "process", "pipBhs", | ||
| "ProcessHandle", ProcessHandle, | ||
| "ProcessId", pid, | ||
| "BaseAddress", BaseAddress, | ||
| "Buffer", NumberOfBytesWritten, Buffer, | ||
| "BufferLength", is_valid_address_range((ULONG_PTR)NumberOfBytesWritten, 4) ? *NumberOfBytesWritten : 0, | ||
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | ||
| ); | ||
| } | ||
| else { | ||
| LOQ_ntstatus( | ||
| "process", "pipBhs", | ||
| "ProcessHandle", ProcessHandle, | ||
| "BaseAddress", BaseAddress, | ||
| "Buffer", NumberOfBytesWritten, Buffer, | ||
| "BufferLength", is_valid_address_range((ULONG_PTR)NumberOfBytesWritten, 4) ? *NumberOfBytesWritten : 0, | ||
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | ||
| ); |
There was a problem hiding this comment.
Similar to the previous comments, the LOQ_ntstatus calls are redundant. Move the call outside the conditional and conditionally include the ProcessId argument in the format string.
| LOQ_ntstatus( | |
| "process", "pipBhs", | |
| "ProcessHandle", ProcessHandle, | |
| "ProcessId", pid, | |
| "BaseAddress", BaseAddress, | |
| "Buffer", NumberOfBytesWritten, Buffer, | |
| "BufferLength", is_valid_address_range((ULONG_PTR)NumberOfBytesWritten, 4) ? *NumberOfBytesWritten : 0, | |
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | |
| ); | |
| } | |
| else { | |
| LOQ_ntstatus( | |
| "process", "pipBhs", | |
| "ProcessHandle", ProcessHandle, | |
| "BaseAddress", BaseAddress, | |
| "Buffer", NumberOfBytesWritten, Buffer, | |
| "BufferLength", is_valid_address_range((ULONG_PTR)NumberOfBytesWritten, 4) ? *NumberOfBytesWritten : 0, | |
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | |
| ); | |
| pid = pid_from_process_handle(ProcessHandle); | |
| LOQ_ntstatus( | |
| "process", pid != GetCurrentProcessId() ? "pipBhs" : "ppBhs", | |
| "ProcessHandle", ProcessHandle, | |
| pid != GetCurrentProcessId() ? "ProcessId" : NULL, pid != GetCurrentProcessId() ? pid : 0, | |
| "BaseAddress", BaseAddress, | |
| "Buffer", NumberOfBytesWritten, Buffer, | |
| "BufferLength", is_valid_address_range((ULONG_PTR)NumberOfBytesWritten, 4) ? *NumberOfBytesWritten : 0, | |
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | |
| ); |
| LOQ_bool( | ||
| "process", "pipBhs", | ||
| "ProcessHandle", hProcess, | ||
| "ProcessId", pid, | ||
| "BaseAddress", lpBaseAddress, | ||
| "Buffer", lpNumberOfBytesWritten, lpBuffer, | ||
| "BufferLength", *lpNumberOfBytesWritten, | ||
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | ||
| ); | ||
| } | ||
| else { | ||
| LOQ_bool( | ||
| "process", "ppBhs", | ||
| "ProcessHandle", hProcess, | ||
| "BaseAddress", lpBaseAddress, | ||
| "Buffer", lpNumberOfBytesWritten, lpBuffer, | ||
| "BufferLength", *lpNumberOfBytesWritten, | ||
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | ||
| ); |
There was a problem hiding this comment.
Similar to the previous comments, the LOQ_bool calls are redundant. Move the call outside the conditional and conditionally include the ProcessId argument.
| LOQ_bool( | |
| "process", "pipBhs", | |
| "ProcessHandle", hProcess, | |
| "ProcessId", pid, | |
| "BaseAddress", lpBaseAddress, | |
| "Buffer", lpNumberOfBytesWritten, lpBuffer, | |
| "BufferLength", *lpNumberOfBytesWritten, | |
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | |
| ); | |
| } | |
| else { | |
| LOQ_bool( | |
| "process", "ppBhs", | |
| "ProcessHandle", hProcess, | |
| "BaseAddress", lpBaseAddress, | |
| "Buffer", lpNumberOfBytesWritten, lpBuffer, | |
| "BufferLength", *lpNumberOfBytesWritten, | |
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | |
| ); | |
| pid = pid_from_process_handle(hProcess); | |
| LOQ_bool( | |
| "process", pid != GetCurrentProcessId() ? "pipBhs" : "ppBhs", | |
| "ProcessHandle", hProcess, | |
| pid != GetCurrentProcessId() ? "ProcessId" : NULL, pid != GetCurrentProcessId() ? pid : 0, | |
| "BaseAddress", lpBaseAddress, | |
| "Buffer", lpNumberOfBytesWritten, lpBuffer, | |
| "BufferLength", *lpNumberOfBytesWritten, | |
| "StackPivoted", is_stack_pivoted() ? "yes" : "no" | |
| ); |
|
I don't have a cape rig to actually test changes, but you can use this PoC I made on public CAPE :) |
Changes inspired by trying to detect https://research.checkpoint.com/2025/waiting-thread-hijacking/
I've found it's quite difficult to keep track of all all of the handles being mapped to various identifiers (PID/TID) from the various APIs in a signature without these improvements.