Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
48f4449
Add files via upload
parikhakshat Aug 19, 2022
8417ab3
Add files via upload
parikhakshat Aug 19, 2022
1a406fe
Remove unused variable and implement windows specific checks
parikhakshat Aug 30, 2022
7489c6d
Revert "Remove unused variable and implement windows specific checks"
parikhakshat Aug 31, 2022
e7fc4aa
Fix debug loop, remove unused variable, and implement windows specifi…
parikhakshat Aug 31, 2022
2fce951
Remove some extraneous code and move it to the Jackalope repository
parikhakshat Sep 6, 2022
d2f1619
Update tinyinst repository by removing extraneous code
parikhakshat Sep 6, 2022
fade015
Remove unused script variable
parikhakshat Sep 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions Windows/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,7 @@ DebuggerStatus Debugger::DebugLoop(uint32_t timeout, bool killing)
uint64_t time_elapsed = end_time - begin_time;
timeout = ((uint64_t)timeout >= time_elapsed) ? timeout - (uint32_t)time_elapsed : 0;

// printf("timeout: %u\n", timeout);
//printf("timeout: %u\n", timeout);
// printf("time: %lld\n", get_cur_time_us());

if (wait_ret) {
Expand All @@ -1480,7 +1480,13 @@ DebuggerStatus Debugger::DebugLoop(uint32_t timeout, bool killing)
dbg_continue_needed = false;
}

if (timeout == 0) return DEBUGGER_HANGED;
if (timeout == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think TinyInst should report incorrect status (process exit instead of timeout). If the calling application wants to treat hang as the process exit, then that's the responsibility of the calling application.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a valid concern, but in my testing of this particular mode, there are many targets that do not exit after a testcase has been successfully inserted and processed. For example, if someone were to fuzz the windows rdp server, the server would not exit after an rdp packet is processed. This would result in a false hang detection by the fuzzer and leave the tool effectively useless for targets like these. Would it be possible to incorporate a cli flag in fuzzer.cpp of jackalope to incorporate this functionality?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you want to accomplish, my point is just that this is not the right place to implement it. Instead of TinyInst claiming the process exited when in fact it did not, it should be the function using it, e.g. TinyInstInstrumentation::Attach or TinyInstInstrumentation::Run that should adapt its behavior.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this makes sense. I will adapt these changes into the Jackalope files then.

if(attach_mode) {
return DEBUGGER_PROCESS_EXIT;
} else {
return DEBUGGER_HANGED;
}
}

if (!wait_ret) {
//printf("WaitForDebugEvent returned 0\n");
Expand All @@ -1491,7 +1497,7 @@ DebuggerStatus Debugger::DebugLoop(uint32_t timeout, bool killing)

thread_id = DebugEv->dwThreadId;

// printf("eventCode: %x\n", DebugEv->dwDebugEventCode);
//printf("eventCode: %x\n", DebugEv->dwDebugEventCode);

switch (DebugEv->dwDebugEventCode)
{
Expand Down Expand Up @@ -1739,11 +1745,10 @@ DebuggerStatus Debugger::Kill() {
// attaches to an active process
DebuggerStatus Debugger::Attach(unsigned int pid, uint32_t timeout) {
attach_mode = true;
processID = pid;

if (!DebugActiveProcess(pid)) {
DWORD error_code = GetLastError();


if(error_code == 5) {
HANDLE hToken = NULL;
LUID luid;
Expand Down Expand Up @@ -1810,19 +1815,25 @@ DebuggerStatus Debugger::Continue(uint32_t timeout) {
dbg_last_status = DEBUGGER_TARGET_START;
return dbg_last_status;
}
if (script != NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is also something that should be implemented in Jackalope rather than TinyInst. IIUC "script" is used for fuzzing sample delivery, and should thus be implemented in Jackalope by subclassing the SampleDelivery class.

HANDLE thread_handle = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)system, script, 0, NULL);
CloseHandle(thread_handle);
}

dbg_last_status = DebugLoop(timeout);

if (dbg_last_status == DEBUGGER_PROCESS_EXIT) {
CloseHandle(child_handle);
CloseHandle(child_thread_handle);
child_handle = NULL;
child_thread_handle = NULL;
if (!attach_mode) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes need to be reverted (IIUC they were related to the DEBUGGER_HANGED change). Basically I think all changes to debugger.cpp / debugger.h can be reverted at this point.

CloseHandle(child_handle);
CloseHandle(child_thread_handle);
child_handle = NULL;
child_thread_handle = NULL;
}
}

return dbg_last_status;
}

// initializes options from command line
void Debugger::Init(int argc, char **argv) {
have_thread_context = false;
Expand Down
4 changes: 4 additions & 0 deletions Windows/debugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,12 @@ class Debugger {
return last_exception;
}

char * script;
Copy link
Collaborator

@ifratric ifratric Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is now unused and can be removed, right?


protected:

DWORD processID;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is unused


enum MemoryProtection {
READONLY,
READWRITE,
Expand Down
25 changes: 25 additions & 0 deletions common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ limitations under the License.
#include <chrono>

#include "common.h"
#include <windows.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These includes and the FindProcessId function below are Windows-specific, but TinyInst is multiplatform. Merging as is would cause build to break on other platforms (as you can see in the status of the pull request) Any Windows-specific code must be wrapped inside
#if defined(WIN32) || defined(_WIN32) || defined(__WIN32)

#include <tlhelp32.h>
#include <iostream>
#include <string>
#include <codecvt>
#include <locale>

uint64_t GetCurTime(void) {
auto duration = std::chrono::system_clock::now().time_since_epoch();
Expand Down Expand Up @@ -96,6 +102,25 @@ int GetIntOption(const char *name, int argc, char** argv, int default_value) {
return (int)strtol(option, NULL, 0);
}

DWORD FindProcessId(char * process_name)
{
PROCESSENTRY32 entry;
entry.dwSize = sizeof(PROCESSENTRY32);

HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, NULL);

if (Process32First(snapshot, &entry) == TRUE)
{
while (Process32Next(snapshot, &entry) == TRUE)
{
if (stricmp(entry.szExeFile, process_name) == 0)
{
CloseHandle(snapshot);
return entry.th32ProcessID;
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function should return something (zero?) if the process wasn't found.

}

//quoting on Windows is weird
size_t ArgvEscapeWindows(char *in, char *out) {
Expand Down
1 change: 1 addition & 0 deletions common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ uint64_t GetCurTime(void);
char *GetOption(const char *name, int argc, char** argv);
void GetOptionAll(const char *name, int argc, char** argv, std::list<char *> *results);
bool GetBinaryOption(const char *name, int argc, char** argv, bool default_value);
DWORD FindProcessId(char * process_name);
int GetIntOption(const char *name, int argc, char** argv, int default_value);

char *ArgvToCmd(int argc, char** argv);
Expand Down