Replace internal CreateThread calls with an internal implementation to avoid monitor detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress #95

kevoreilly · 2025-04-17T09:47:01Z

No description provided.

…o avoid monitor detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress

gemini-code-assist

Hello @kevoreilly, I'm Gemini Code Assist¹! 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 addresses a potential monitor detection issue by replacing direct CreateThread calls with an internal implementation (our_createthread). The internal implementation uses CreateTimerQueueTimer to execute the thread's start routine, which avoids direct detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress. This change affects the unhook.c and misc.c files, with corresponding header updates in misc.h.

Highlights

Security Enhancement: Replaces direct CreateThread calls to prevent monitor detection via NtQueryInformationThread.
Internal Thread Creation: Introduces our_createthread function to handle thread creation using CreateTimerQueueTimer.
Code Modification: Modifies unhook.c to use our_createthread for creating several threads, including unhook detection, watcher, terminate event, process name watch, and watchdog threads.

Changelog

Click here to see the changelog

misc.c
- Introduces thread_callback function to execute the thread's start routine.
- Implements our_createthread function using CreateTimerQueueTimer.
- The thread_callback function now sets the thread ID and signals an event upon completion.
misc.h
- Adds the definition of the THREAD_CONTEXT structure to store thread-related data.
- Declares the our_createthread function.
- Adds function prototype for our_createthread.
unhook.c
- Replaces CreateThread calls with our_createthread for g_unhook_thread_handle, g_watcher_thread_handle, g_terminate_event_thread_handle, g_procname_watch_thread_handle, and the watchdog thread.
- Updates thread creation logic to use the new our_createthread function.

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 thread starts in the queue,
A timer ticks, a different view.
No direct start,
Avoiding the chart,
A stealthy path, anew.

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. ↩

gemini-code-assist

Code Review

The pull request introduces a custom thread creation function (our_createthread) to avoid detection by NtQueryInformationThread. The approach uses timer queues to execute the thread's start routine asynchronously. While this may achieve the intended goal of evading detection, it's important to consider the implications of this approach on resource management, error handling, and the overall thread lifecycle. The code changes look good overall, but I have a few suggestions for improvement.

Summary of Findings

Resource Management: The our_createthread function uses CreateTimerQueueTimer which creates a timer. The timer handle is not explicitly managed, and the lifetime of the timer is tied to the event handle returned. It's important to ensure that the timer is properly cleaned up to avoid resource leaks.
Error Handling in our_createthread: The our_createthread function returns the event handle, but the actual thread execution is asynchronous via a timer. If the timer creation fails, the event handle is closed and memory is freed. However, if the timer creation succeeds, the caller receives the event handle but has no direct way to manage the timer's lifecycle. This could lead to confusion and potential resource leaks if the caller expects a traditional thread handle.
Thread ID Retrieval: The thread ID is retrieved within the thread_callback function using GetCurrentThreadId(). This ID is then stored in the context->thread_id pointer. However, the thread might terminate before the ID is retrieved, leading to a potential race condition or incorrect thread ID being stored.

Merge Readiness

The pull request introduces significant changes to thread creation. While the approach may address the stated goal, it's important to carefully consider the implications of this approach on resource management, error handling, and the overall thread lifecycle. I would recommend addressing the high and medium severity issues before merging. I am unable to directly approve this pull request, and users should have others review and approve this code before merging.

gemini-code-assist · 2025-04-17T09:47:23Z

misc.c

+    if (!CreateTimerQueueTimer(&timer, NULL, thread_callback, context, 0, 0, WT_EXECUTEDEFAULT)) {
+        CloseHandle(context->event);
+        free(context);
+        return NULL;


The CreateTimerQueueTimer function creates a timer. The timer handle is not explicitly managed. It's important to ensure that the timer is properly cleaned up to avoid resource leaks. Consider using DeleteTimerQueueTimer to remove the timer from the queue and release associated resources. It's also important to add a comment explaining why these functions are called.

Suggested change

if (!CreateTimerQueueTimer(&timer, NULL, thread_callback, context, 0, 0, WT_EXECUTEDEFAULT)) {

CloseHandle(context->event);

free(context);

return NULL;

CloseHandle(context->event);

free(context);

return NULL;

}

// Timer created successfully, but ensure it's cleaned up if the function fails later

context->timer = timer; // Store the timer handle in the context

gemini-code-assist · 2025-04-17T09:47:23Z

misc.c

+    SetEvent(context->event);
+    CloseHandle(context->event);
+    free(context);


Consider adding error handling around SetEvent and CloseHandle to ensure that these resources are properly managed. Although these functions are unlikely to fail, it's good practice to check for errors, especially in a system-level context. It would also be good to add a comment explaining why these functions are called.

Suggested change

SetEvent(context->event);

CloseHandle(context->event);

free(context);

if (!SetEvent(context->event)) { // Signal that the thread has completed

// Log the error, but continue cleanup

DWORD err = GetLastError();

DebugOutput("SetEvent failed with error: %d\n", err);

}

if (!CloseHandle(context->event)) { // Close the event handle

// Log the error, but continue cleanup

DWORD err = GetLastError();

DebugOutput("CloseHandle failed with error: %d\n", err);

}

gemini-code-assist · 2025-04-17T09:47:23Z

misc.c

+        return NULL;
+    }
+
+    return context->event;


The function returns the event handle, but the actual thread execution is asynchronous via a timer. If the timer creation fails, the event handle is closed and memory is freed. However, if the timer creation succeeds, the caller receives the event handle but has no direct way to manage the timer's lifecycle. This could lead to confusion and potential resource leaks if the caller expects a traditional thread handle. Consider returning the timer handle instead of the event handle, or providing a separate function to manage the timer's lifecycle.

Suggested change

return context->event;

// Returning the timer handle instead of the event handle

return timer;

gemini-code-assist · 2025-04-17T09:47:23Z

misc.c

+    if (context->thread_id)
+        *context->thread_id = GetCurrentThreadId();


The thread ID is retrieved within the thread_callback function using GetCurrentThreadId(). This ID is then stored in the context->thread_id pointer. However, the thread might terminate before the ID is retrieved, leading to a potential race condition or incorrect thread ID being stored. Consider using a synchronization mechanism to ensure that the thread ID is retrieved before the thread terminates, or use a different approach to retrieve the thread ID.

Suggested change

if (context->thread_id)

*context->thread_id = GetCurrentThreadId();

DWORD threadId = GetCurrentThreadId(); // Get the current thread ID

if (context->thread_id) {

*context->thread_id = threadId; // Store the thread ID

}

Replace internal CreateThread calls with an internal implementation t…

c181d3b

…o avoid monitor detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress

gemini-code-assist bot reviewed Apr 17, 2025

View reviewed changes

gemini-code-assist bot suggested changes Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace internal CreateThread calls with an internal implementation to avoid monitor detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress #95

Replace internal CreateThread calls with an internal implementation to avoid monitor detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress #95

Uh oh!

kevoreilly commented Apr 17, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Apr 17, 2025

Uh oh!

gemini-code-assist bot Apr 17, 2025

Uh oh!

gemini-code-assist bot Apr 17, 2025

Uh oh!

gemini-code-assist bot Apr 17, 2025

Uh oh!

Uh oh!

-    SetEvent(context->event);
-    CloseHandle(context->event);
-    free(context);
+    if (!SetEvent(context->event)) { // Signal that the thread has completed
+        // Log the error, but continue cleanup
+        DWORD err = GetLastError();
+        DebugOutput("SetEvent failed with error: %d\n", err);
+    }
+    if (!CloseHandle(context->event)) { // Close the event handle
+        // Log the error, but continue cleanup
+        DWORD err = GetLastError();
+        DebugOutput("CloseHandle failed with error: %d\n", err);
+    }

	return context->event;
	// Returning the timer handle instead of the event handle
	return timer;

		if (context->thread_id)
		*context->thread_id = GetCurrentThreadId();

-    if (context->thread_id)
-        *context->thread_id = GetCurrentThreadId();
+    DWORD threadId = GetCurrentThreadId(); // Get the current thread ID
+    if (context->thread_id) {
+        *context->thread_id = threadId; // Store the thread ID
+    }

Replace internal CreateThread calls with an internal implementation to avoid monitor detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress #95

Are you sure you want to change the base?

Replace internal CreateThread calls with an internal implementation to avoid monitor detection by NtQueryInformationThread using ThreadQuerySetWin32StartAddress #95

Uh oh!

Conversation

kevoreilly commented Apr 17, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

gemini-code-assist bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!