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

Jump to bottom

Update to make boofuzz more compatible #594

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

Update to make boofuzz more compatible #594

wants to merge 12 commits into from

Conversation

hegusung
Copy link

I had these 3 issues when using boofuzz:

  • I could not change processmonitor.py listening interface
  • processmonitor.py was not compatible with Windows, multiple function used in boofuzz were not available for Windows.
  • Some processes to fuzz needs a lot of time before starting up, so the hardcoded 3 seconds delay were not sufficient. A parameter "restart_wait" in Session class has been added to solve this issue

@jtpereyda
Copy link
Owner

@hegusung Sounds great! I'll put this on my queue to review. Which operating systems have you tested on so far with these changes?

@hegusung
Copy link
Author

Hi, So boofuzz script was tested on both a Kali linux and a Debian. Process_monitor.py was tested on a Windows 10.

there shouldn't be an issue with process_monitor.py on linux because the linux way is always done, il only switches to windows way when an exeption is detected.

@hegusung
Copy link
Author

I don't understand what is wrong with stickler-ci

@SR4ven
Copy link
Collaborator

SR4ven commented Jan 28, 2022

I don't understand what is wrong with stickler-ci

Seems like it runs into a timeout. Nothing u need to worry about.
If you want to check for code style issues, you can run tox or tox -e lint (not exactly sure about the second one). Also note that it outputs all findings, not just the ones in your changes.

SR4ven
SR4ven requested changes Feb 27, 2022
View reviewed changes
Copy link
Collaborator

@SR4ven SR4ven left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @hegusung!

I left some comments below.

process_monitor.py Outdated Show resolved Hide resolved
boofuzz/sessions.py Outdated Show resolved Hide resolved
jtpereyda
jtpereyda requested changes Mar 14, 2022
View reviewed changes
Copy link
Owner

@jtpereyda jtpereyda left a comment

Choose a reason for hiding this comment

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

@hegusung Thanks for this work! This was a significant gap in boofuzz.

After this PR, it wouldn't be much work to integrate the two procmon scripts, since the Simple debugger will work with both Windows and Unix.

Thanks @SR4ven for the review!

boofuzz/sessions.py Outdated Show resolved Hide resolved
process_monitor.py Outdated Show resolved Hide resolved
@hegusung
Copy link
Author

hegusung commented May 3, 2022

Hi, I see that this PR is not accepted.
Which changes should I do in order for you guys to accept it ?

@hegusung
Copy link
Author

hegusung commented May 3, 2022

I took care of your remarks,

I also patched the following bug:

  • when the second or more mutant fails 3 times, the skipped value will be 0. therefore the test case number is incorrect.
  • This is due to self.mutant_index is necessary bigger than self.fuzz_node.mutant.get_num_mutations() at the second and more fuzzed mutant

@hegusung hegusung requested review from jtpereyda and SR4ven May 3, 2022 20:08
SR4ven
SR4ven requested changes May 31, 2022
View reviewed changes
Copy link
Collaborator

@SR4ven SR4ven left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review.

Thanks for implementing our comments!
I've added one more tiny suggestion.

Could you add some lines about those fixes to the CHANGELOG.rst please?

boofuzz/fuzzable.py
@@ -50,6 +50,8 @@ def __init__(self, name=None, default_value=None, fuzzable=True, fuzz_values=Non
Fuzzable.name_counter += 1
self._name = "{0}{1}".format(type(self).__name__, Fuzzable.name_counter)

self.index_mutation = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about mutation_index?

Suggested change
self.index_mutation = 0
self.mutation_index = 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SR4ven SR4ven SR4ven requested changes

@jtpereyda jtpereyda Awaiting requested review from jtpereyda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hegusung @jtpereyda @SR4ven @stickler-ci