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 timeout command to binwalk call. Related to FACT_core #820 #94

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Add timeout command to binwalk call. Related to FACT_core #820 #94

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a1e5a9
Add timeout command to binwalk call. Related to FACT_core #820
Sep 8, 2022
95ad81b
Changing timeout set to binwalk call.
Sep 21, 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
2 changes: 1 addition & 1 deletion fact_extractor/plugins/unpacking/generic_carver/code/generic_carver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def unpack_function(file_path, tmp_dir):
'''

logging.debug('File Type unknown: execute binwalk on {}'.format(file_path))
output = execute_shell_command(f'binwalk --extract --carve --signature --directory {tmp_dir} {file_path}')
output = execute_shell_command(f'timeout 580 binwalk --extract --carve --signature --directory {tmp_dir} {file_path}')
Copy link

Choose a reason for hiding this comment

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

what about turning this a config parameter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey! I'm agreeing with @dgutson here.

For transparency, it is probably for the best to convert this into a config param. @Oren-i, do you have capacities to address this change or shall our team allocate resources for this change internally? (keep in mind that it may take some time then)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @rhelmke, where should the configuration option should be located? I can do it if it is only a change to fact_extractor. If a change to fact_core is required I think it is best if your team do it, as it needs more coordination.

Copy link
Collaborator

@jstucke jstucke Sep 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

How did you get to the conclusion that the timeout for the docker image is 600? The call in FACT (FACT_core/src/unpacker/unpack_base.py:28) doesn't set a timeout and the default value (in FACT_core/src/helperFunctions/docker.py:41) is 300 seconds. So maybe it would make sense to also increase the timeout in FACT.
What is more, the "generic carver" is also called as a fallback if another unpacking method fails so it could make sense to increase the time buffer a bit.

As far as placement for the configuration is concerned, there currently is no place for putting configuration of unpacking plugins. In "FACT_core" you can add configuration for each plugin in main.cfg but the development of FACT_core and the extractor diverged quite a bit since the latter moved to its own project. We could rework how extraction plugins are structured and use a similar class-based approach as in FACT_core but this would definitely be out-of-scope for this PR. Therefore, IMHO it would probably be best to set it as a constant inside the plugin (fact_extractor/plugins/unpacking/generic_carver/code/generic_carver.py).

Copy link
Collaborator

Choose a reason for hiding this comment

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

What might be interesting for you: We identified a "bug" in the helper library common_helper_unpacking_classifier which led to unpacking taking far longer than necessary for large files because of an overly complicated stats generation (see fkie-cad/common_helper_unpacking_classifier#4). With that fix, the chance of timeouts happening should also be a lot smaller.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @jstucke, I will look at that fix. You are probably correct, I got the 600 seconds value from how long FACT ran in my test before showing the exception, the difference was probably due to uncompressing other files before the one that causes the error.

I changed the timeout to 280, and added the constant to the plugin.

Copy link
Author

Choose a reason for hiding this comment

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

Converting to draft as the changes are going do cause unneeded timeouts when using fact_extractor in standalone mode. I think the correct way to implement this should be by getting the timeout value from fact_core as a parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @jstucke, I will look at that fix.

After the merge of #95 the docker image of the extractor was rebuilt and pushed to DockerHub so you could simply pull the updated image to try it out. I would be very much interested in knowing how well that change in itself alleviates your timeout problems.


drop_underscore_directory(tmp_dir)
return {'output': output, 'filter_log': ArchivesFilter(tmp_dir).remove_false_positive_archives()}
Expand Down