Target restart behavior

[omnifocal]

Stopping the target process before it is restarted has been made optional via the instance variable stop_first in boofuzz.session.Session

[jtpereyda]

Can you help me understand the use case? Why/when do you want to enable stop first?

It might be cleaner to implement this in the procmon itself. We could give it a .restart method, and then Session could altogether stop worrying about stop_first.

[omnifocal]

This was just a small change that assisted me with a particular target that I'm fuzzing. I simply needed to be able to control the value of the stop_first parameter of Session.restart_target()

I implemented it in Session as opposed to procmon in order to try to keep as much configuration as possible on the fuzzing host rather than the procmon host

[jtpereyda]

Thanks for the PR btw. Are you able to share your motivation for wanting to change the stop_first value (if not, that's fine)? Was the restart procedure failing otherwise? Did you just want to make sure you had a clean run each time? Etc.

As for the design, I'm taking a second look at the layout, maybe you're right. The sessions file used to look a lot uglier, and my goal for a while has been to get stuff out of it. Unfortunately, it's still basically the catch-all, which is less than ideal from a design standpoint (e.g., Single Responsibility Principle).

[omnifocal]

To put a bit of context to this, I'm currently working on a research project involving fuzzing. As part of this I'm attempting to extend the Unix procmon's ability to act as fuzzing instrumentation. This is happening over in my fork.

Without going to extensive detail, I needed to be able to manipulate stop_first programmatically. I submitted this PR basically because I thought the configurability of stop_first would be a good addition to the base repo. The default value of the parameter (False) has remained unchanged. It's simply available for users to set upon instantiating Session.

[omnifocal]

And yeah, the Session class is definitely something of a catch-all as you said, however personally I feel that a major refactor in that area isn't really worth the effort at this time. It's come a long, long way since sulley (well done btw) and is easy enough to work with as is

[jtpereyda]

Recording my design decision here for reference:

Single Responsibility Principle

One of the background refactoring goals is to move toward observation of the Single Responsibility Principle (SRP), in which each class would be conceptually responsible for one thing. Session is a huge violation currently.

The Real World

However, given the state of the project, separating concerns can be a real pain. This is because the program has been previously developed in violation of the SRP and Separation of Concerns.

As you said, a proper refactor would be a comparatively huge effort to the proposed change. It'd be frustrating and anti-pragmatic to block this feature since it's currently so easy to implement.

Future

If you're trying to expand the procmon's capabilities, and if you need to give it various options, consider sending options via procmon_options, if you haven't already. Example from here:

target.procmon_options =  {
    "proc_name" : "smallftpd.exe",
    "stop_commands" : ['wmic process where (name="smallftpd.exe") delete"'],
    "start_commands" : ['C:\\Documents and Settings\\Administrator\\Desktop\\smallftpd-1.0.3-fix\\smallftpd.exe'],
}

This allows the configuration to stay on the fuzzing host. It is transmitted to the procmon host over RPC.

I think the configurability of stop_first is a good addition. Thanks for taking the time to submit. I'll try to look into things a bit more before being a stickler in the future. Best wishes on your research project and happy fuzzing!

[jtpereyda]

@omnifocal Session.fuzz() also calls restart_target. Would you want it to stop_first in this case as well?

[omnifocal]

@jtpereyda Thanks, I hadn't noticed that call to restart_target. I'll add a commit that passes stop_first to the restart_target call in fuzz. That way the default can be set in Session's __init__() and overridden on instantiation.

[omnifocal]

When taking a closer look at Session it appears that the default behaviour of restart_target is actually to stop the target process first. In the current main branch this is overridden by _process_failures but not by fuzz.

Restarting the target in fuzz serves a different purpose to doing so in _process_failures, that is, it is intended to refresh the state of the target process in the course of normal fuzzing. In _process_failures it is obviously needed to recover the target after a fault.

At this stage to allow full configurability one would need to pass two separate options into Session. One for restart behaviour in fuzz, and another for _process_failures

[jtpereyda]

Excellent observation. It sounds like the usage in fuzz is intended to always restart. If it's OK with you, revert the March 13 commit so that we end up with:

• Restart in fuzz always stops first.
• Restart in _process_failures is configurable.

Thinking about it again, it seems like the default of not stopping first in _process_failures makes sense if every time there is a failure, the process is already stopped. This assumption may no longer be valid.

Eventually, the best solution may be to supplement procmon's start and stop methods with a single restart. That way the procmon itself can stop the UUT if and only if it needs to. Let me know if you have any thoughts, and thank you again for the contribution and patience with the review.

[jtpereyda]

@omnifocal Gentle bump. Are you OK with reverting the March 13th commit (see previous post)? If you're not able/interested anymore, just let me know.

[jtpereyda]

Closing due to lack of updates. Anyone is welcome to open a new PR.