Add Support Functions for Fuzzing Attached Processes and Fix a False Hang issue in attached processes #61
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Windows/debugger.cpp
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay, this makes sense. I will adapt these changes into the Jackalope files then.
Windows/debugger.h
Outdated
| protected: | ||
|
|
||
| DWORD processID; |
common.cpp
Outdated
| @@ -20,6 +20,12 @@ limitations under the License. | |||
| #include <chrono> | |||
|
|
|||
| #include "common.h" | |||
| #include <windows.h> | |||
There was a problem hiding this comment.
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)
debugger.cpp
Outdated
| @@ -0,0 +1,1917 @@ | |||
| /* | |||
There was a problem hiding this comment.
Git thinks entire debugger.cpp / debugger.h is new. What happened here? Did you change the newline convention for the entire file? In any case, please revert this as it makes the PR impossible to review.
|
Sorry about the problems with the file structure of the repository. I was trying to commit my edits to the files and I instead uploaded the file to the repository in the incorrect location. Let me know how these edits looks and then I'll start working on fixing the Jackalope implementation. |
Windows/debugger.cpp
Outdated
| @@ -1810,19 +1808,25 @@ DebuggerStatus Debugger::Continue(uint32_t timeout) { | |||
| dbg_last_status = DEBUGGER_TARGET_START; | |||
| return dbg_last_status; | |||
| } | |||
| if (script != NULL) { | |||
There was a problem hiding this comment.
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.
Windows/debugger.cpp
Outdated
| CloseHandle(child_thread_handle); | ||
| child_handle = NULL; | ||
| child_thread_handle = NULL; | ||
| if (!attach_mode) { |
There was a problem hiding this comment.
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.
common.cpp
Outdated
| @@ -20,6 +20,14 @@ limitations under the License. | |||
| #include <chrono> | |||
|
|
|||
| #include "common.h" | |||
| #if defined(WIN32) || defined(_WIN32) || defined(__WIN32) | |||
| #include <windows.h> | |||
There was a problem hiding this comment.
IIRC windows.h is already included from common.h so it's not needed here.
|
I think I fixed all the code here. I added a new commit on the Jackalope repository that moves most of the code needed for fuzzing to that repository. |
Windows/debugger.h
Outdated
| @@ -77,6 +77,8 @@ class Debugger { | |||
| return last_exception; | |||
| } | |||
|
|
|||
| char * script; | |||
There was a problem hiding this comment.
This variable is now unused and can be removed, right?
|
I just pushed a commit that removed this variable. Let me know how the rest of the edits are. Thanks! |
|
Cool, thanks! I think TinyInst side looks good now, except you could also completely revert changes to |
| return entry.th32ProcessID; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The function should return something (zero?) if the process wasn't found.
To add support to Jackalope for fuzzing attached processes, I needed to add a helper function FindProcessId to common.cpp. The second issue I found was in the function DebugLoop. When getting coverage by attaching to a running process, the DebugLoop would always report a hang because the running process wouldn't exit. For many running processes, the program does not exit event after a testcase is sent to it, so this would result in false hangs (Ex. fuzzing a mail server).