boofuzz/sessions.py is too long and complicated, let's fix that.

[hackathi]

Proposal

I've said it a couple times already, and every time I have a look at that file I'm instantly lost. Thus, I'll propose to split this monstrosity in smaller chunks.

Generally, there are three things I want to address here:

One class per file

sessions.py is well over 1500 LoC. This warrants splitting the file into a subpackage; Import paths should not change. I'd consider this a low impact, non-breaking change.

Simplify the Constructor

Most of the constructor consists of making sure things are set, sanitizing them and/or providing defaults from the constants module. I think this could be made much cleaner by implementing these in a SessionOptions class where each valid option has a property that does sanitation and default-setting in a setter and have computed options in a (cached) getter.

As a stub, I'd propose something like this:

class SessionOptions:
    def __init__(self, *args, **kwargs):
        self.options = {}

        for key, val in kwargs.items():
            if hasattr(self, key):
                setattr(self, key, val)
            else:
                # log invalid option supplied

    @property
    def web_address(self):
        return self.options["web_address"] if "web_address" in self.options else ('127.0.0.1', 6300)
        
    @property.setter
    def web_address(self, value: tuple[str, int]):
        self.options["web_address"] = value

and then remove most keyword arguments from session. This is a breaking change, and we should discuss how to handle this gracefully. One option would be to just pass all kwargs supplied to Session to the SessionOptions should session detect more than the expected number of kwargs, and then issue a FutureWarning. The more nuclear option would be to just remove them; after all, the code change in user code isn't too great.

This would also be the point where I'd remove the callbacks we deprecated in 2020; I think two years are enough and moving these to a new configuration system seems questionable to me.

An added benefit of such a SessionOption approach is that configuration can be easily loaded from a file. Some of the options, like web ports or whatever, might make sense to set project-independent in a boofuzzrc; but that's just a dream for later at this point.

Separate concerns

One reason why session is so large is that, in my opinion, it does too much. A prime example (and the lowest hanging fruit IMHO) is that the whole fuzzing loop is hard-coded into session. As one step towards a smaller session class I'd propose a rework of how the fuzzing loop works, abstracting it down to "for each test case, run through the steps in this ordered list".

This means that all current steps become classes following a common interface; these can be included or excluded at will. This also makes some more callbacks obsolete in favor of pluggable class steps that can be chained anywhere in the test case execution pipeline.

Obviously, this will be a huge shift in paradigm and probably introduce breaking changes. However, like I've done with the connections, it's possible to gracefully shim the existing API to use this pluggable chain instead. Additionally, many of the functions affected by such a rework are underscore function anyways, which I consider fair game to move without worrying about breaking someones code.

Use-Case

Use-Case: keep my sanity while developing, improve readability and maintainability.

Anything else?

What are your thoughts on this? I'll start a PR to implement all of the above and mark it as WIP. If you have comments about the architecture of how things should work, please share them. Most of this is only a high-level thought I have at the moment. Especially reworking the session options might not even be necessary to this extent once the test case pipeline is in place.

[hackathi]

I'll also understand that this will probably create some merge conflicts with #624. At the moment though my goal is to move code, not to edit it, so these should be resolvable fairly easily.

[hackathi]

On further consideration, it seems to me that many options will simply be unnecessary in the first place once the pipeline is dynamic and pluggable. Take for example this excerpt:

boofuzz/boofuzz/sessions.py

Lines 1396 to 1402 in 2e1d91a

	if (
	self.num_cases_actually_fuzzed
	and self.restart_interval
	and self.num_cases_actually_fuzzed % self.restart_interval == 0
	):
	self._fuzz_data_logger.open_test_step("restart interval of %d reached" % self.restart_interval)
	self._restart_target(self.targets[0])

These options can simply be set when creating the step instance for the pipeline, and thus don't need to be set in the sessions constructor.

Obviously, boofuzz should make it easy to get some set of default pipelines with a subset of configuration options for common use cases (think from boofuzz.pipelines import default_pipeline).

[kamakazikamikaze]

I'll also understand that this will probably create some merge conflicts with #624. At the moment though my goal is to move code, not to edit it, so these should be resolvable fairly easily.

Right now there's no foreseeable date on when I'll get approval to release my contributions to the public. Regardless, I agree that the merge conflict that may rise from this will be easy to resolve.

[MetinSAYGIN]

Is this ticket still open ? I probably would like to contribute for it.
Metin

[jtpereyda]

@mistressofjellyfish Thanks for the issue! My apologies as I've been way behind on open source work.

I like the overall idea. 100% agree on splitting out classes.

I would say that my bias is heavily toward backwards-compatibility for imports. It's easy to use aliases to preserve importable names, so it won't create a huge code overhead.

I'd lean the same way with regard to SessionOptions. Keep the kwargs and use them to create a SessionOptions within the Session constructor. You can use a helper method if it helps readability in Session. I actually think the Session constructor, though long, is relatively easily understood.

Re fuzzing loop -- I definitely agree there is room for improvement, though I'm not entirely sure how. The code used to be more procedural, and I refactored it to be a little less stateful. The logic is confusing, but a key attribute is that it enables multiple mutations on one message. That's a property I would want it to keep. For starters, just moving this logic outside Session might be a good step.

I'll check out the PR too.

[jtpereyda]

@MetinSAYGIN #638 has some minor changes requested that somebody could take over! Feel free to build on that PR and fix the minor issues.

[hackathi]

@jtpereyda @MetinSAYGIN

So, it happens that I have some code lying around that has this done, more or less; but without the SessionOptions object. I'm still in the process of getting permission to opensource it, but it's 100% on me that it has been stale for this long.

However, my code depends on #638; and since I'm currently in the middle of moving houses I probably won't get around to finish that anytime soon. So if someone could pick up there, I can maybe allocate some time in the near future to get the other code in a PR.

Just a word of warning though; the particular way I chose to split the Session class was heavily influenced by my research needs. It might need some more work before being ready to merge and as it currently stands, I'm unsure whether I can allocate the time neccessary to do so. Originally, I planned to hire a student assistant to finish this PR and aid my research, but so far this hasn't come to fruition.

[jtpereyda]

@mistressofjellyfish thanks for the update -- if you get it approved, an in-progress PR is plenty welcome. Good luck!