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.

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

feat: converted strings plugin to v1 and integrated strings eval #1323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jstucke
Copy link
Collaborator

@jstucke jstucke commented Jan 10, 2025

  • migrates the "printable_strings" plugin to the new base class
  • integrates the "string_evaluation" plugin into the "printable_strings" plugin
  • uses bootstrap-table to display the new result structure in the template

@jstucke jstucke requested a review from maringuu January 10, 2025 15:41
@jstucke jstucke self-assigned this Jan 10, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 99.55947% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.81%. Comparing base (ab95ac5) to head (dc02b8d).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...c/plugins/analysis/strings/internal/string_eval.py 98.52% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1323      +/-   ##
==========================================
- Coverage   92.48%   91.81%   -0.67%     
==========================================
  Files         379      374       -5     
  Lines       24115    20981    -3134     
==========================================
- Hits        22302    19264    -3038     
+ Misses       1813     1717      -96     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jstucke jstucke force-pushed the strings-v1-with-eval branch 3 times, most recently from f7e09d0 to ef8b2a8 Compare January 13, 2025 10:16
Copy link
Collaborator

@maringuu maringuu left a comment

Choose a reason for hiding this comment

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

Overall lgtm (The table is especially nice!). Just a few nits.

DICTIONARY = {'version', 'v.', 'http', 'ftp', 'usage', 'Usage', 'ssh', 'SSH', 'password', 'Version'}


def evaluate_string(string: str) -> float:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: What about renaming this to string_relevance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

string_relevance is lacking a verb. I renamed it to calculate_relevance_score

Schema=self.Schema,
)
)
)
self.regexes = self._compile_regexes()

def _compile_regexes(self) -> list[tuple[Pattern[bytes], str]]:
min_length = getattr(config.backend.plugin.get(self.NAME, {}), 'min-length', 8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This only works as long as we use AnalysisBasePluginAdapterMixin.
Instead this should use self.metadata.name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Totally overlooked that bit

metadata=(
self.MetaData(
name='printable_strings',
description='extracts strings and their offsets from the files consisting of printable characters',
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?

Suggested change
description='extracts strings and their offsets from the files consisting of printable characters',
description='Extracts printable strings from a file and assigns a relevance score based on a predefined ruleset.',

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think your change better reflects what the plugin does -> changed

Comment on lines 1 to 5
"""
evaluate relevance of strings

Credits:
Original version by Paul Schiffer created during Firmware Bootcamp WT16/17 at University of Bonn
Refactored and improved by Fraunhofer FKIE
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that doc comment is that useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the doc part and added a more informative docstring to the function instead.

@jstucke jstucke force-pushed the strings-v1-with-eval branch 3 times, most recently from a6fcef0 to 8486990 Compare January 16, 2025 13:11
* migrates the "printable_strings" plugin to the new base class
* integrates the "string_evaluation" plugin into the "printable_strings" plugin
* uses bootstrap-table to display the new result structure in the template
@jstucke jstucke force-pushed the strings-v1-with-eval branch from 8486990 to dc02b8d Compare January 16, 2025 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants