add word boundaries to regex patterns to reduce false positives#1125
add word boundaries to regex patterns to reduce false positives#1125Shaktisinhchavda wants to merge 2 commits intomandiant:masterfrom
Conversation
|
@mr-tz please review. |
mr-tz
left a comment
mr-tz
left a comment
There was a problem hiding this comment.
have you also looked at the substring features?
| - string: /(?<!\w)ida[gqtuw]?(\.exe)?$/i | ||
| - string: /ida[gqtuw]?64(\.exe)?$/i |
There was a problem hiding this comment.
Thanks for the review, @mr-tz!
-- IDA Regexes (Lines 27-28): Good catch. I missed these because they had slightly different structures (specifically the lookbehind and end-of-string anchor). I've updated them to "/\bida[gqtuw]?(.exe)?\b/i" and "/\bida[gqtuw]?64(.exe)?\b/i" for consistency with the rest of the file.
-- Substring Features: I primarily focused on existing regex string patterns in this pass. However, searching the ruleset, I see several substring features (like ifconfig, TouchSocket, and CHttpModule) that would definitely benefit from word boundaries to reduce false positives. I'll start converting these to regex string features with \b boundaries as suggested.
I've pushed an update with the IDA fixes. Should I include the substring conversions in this PR, or would you prefer a separate one focused on that?
| - or: | ||
| - api: EnumDeviceDrivers | ||
| - string: /driverquery(\.exe)?/i | ||
| - string: /\bdriverquery(\.exe)?\b/i |
There was a problem hiding this comment.
how did you decide when to have a trailing \b and when not to?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
why do you have one \b for some strings and two \b in other strings?
There was a problem hiding this comment.
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.
| - string: /\brekall(\.exe)?\b/i | ||
| - string: /\btcpdump(\.exe)?\b/i | ||
| - string: /\bwindasm(\.exe)?\b/i | ||
| - string: /\bx32dbgn(\.exe)?\b/i |
There was a problem hiding this comment.
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:
- substring: foo
- substring: foo.exethis 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.
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.
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.
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.
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.
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
3 participants
closes #1109