-
Notifications
You must be signed in to change notification settings - Fork 233
add word boundaries to regex patterns to reduce false positives #1125
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ rule: | |
| features: | ||
| - or: | ||
| - api: EnumDeviceDrivers | ||
| - string: /driverquery(\.exe)?/i | ||
| - string: /\bdriverquery(\.exe)?\b/i | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how did you decide when to have a trailing \b and when not to?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The \b was intended to prevent partial matches like driverqueryhost. I agree it’s messy, so I propose switching to dual substring entries instead (e.g., driverquery and driverquery.exe). It provides the same protection with much better readability. What do you think? @williballenthin
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you have one \b for some strings and two \b in other strings?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The inconsistency was an oversight during the update. Switching to the proposed dual substring approach will fix this entirely and ensure consistent whole-word matching across all rules. |
||
| - and: | ||
| - or: | ||
| - match: query or enumerate registry key | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this change, the patterns are much harder to read as a human, which is unfortunate.
this isn't a comment on the quality of the PR, just about what we're considering.
it seems many of these regexes are regexes versus substrings so that we can match the optional extension in a single pass. we could replace these with something like:
this would be much easier to read (the \b would happen behind the scenes), though presumably runtime would be longer, because there are twice as many regexes to run.
with a little bit of benchmarking we could show this is or is not acceptable. thoughts @mr-tz @mike-hunhoff @Shaktisinhchavda ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the regexes are hard to audit. I propose refactoring them into simple substring pairs for the tool names and extensions. This prioritizes clarity while maintaining the same logic. I’m happy to update the PR this way if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is what i proposed above. please suggest a plan that includes benchmarking capa before/after to show that the runtime doesn't change substantially when introducing additional substring features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the scenario that runtime is worse, we can imagine an optimization to capa that translates an
or(substring, substring, substring)into a single regex that matches in a single pass. so i don't think this is insurmountable. but we should still check the data before committing to the changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed that data should drive the decision. Here is my plan for benchmarking the performance impact:
--Test Set: Run capa against the complete capa-testfiles repository.
Evaluation: Compare average wall-clock runtime across three passes for the current regex rules versus the proposed substring refactor.
--Verification: Confirm that match results and hit counts remain identical for all samples.
I’ll share the results here once complete. I also agree that if we do see a performance hit, an engine-level optimization to merge OR-ed substrings would be an excellent long-term solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed the benchmarking using a 3-pass average against capa-testfiles. Results confirmed identical hits but showed a ~55% increase in average runtime (from 1.04s up to 1.62s per file).
While the delta is ~0.6s, I believe the significant gain in readability and auditability justifies the overhead. We could potentially recover this performance later via the engine-level optimization mentioned. Is this trade-off acceptable to the team? @williballenthin