add sequence statement for ordered matching#2816
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed
Summary of ChangesHello @mike-hunhoff, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the expressiveness of capa's rule engine by introducing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a sequence statement to enforce ordered matching, which is a valuable addition for creating more precise rules. The implementation is well-structured and includes good test coverage. I've identified a couple of potential issues: a duplicated enum member and the use of unique() on sequence children, which could lead to incorrect behavior. I've also suggested a refactoring to improve code clarity in the new Sequence class and pointed out a duplicated test case. Overall, this is a solid contribution.
| AND = "and" | ||
| OR = "or" | ||
| NOT = "not" | ||
| NOT = "not" |
There was a problem hiding this comment.
The NOT enum member is duplicated. Please remove the redundant line to avoid potential issues.
| elif key == "or": | ||
| return ceng.Or(unique(build_statements(dd, scopes) for dd in d[key]), description=description) | ||
| elif key == "sequence": | ||
| return ceng.Sequence(unique(build_statements(dd, scopes) for dd in d[key]), description=description) |
There was a problem hiding this comment.
Using unique() on the children of a sequence statement prevents a rule from specifying a sequence of identical features (e.g., two consecutive push instructions). Since the order and multiplicity of features are significant in a sequence, unique() should not be used here. This allows rules to match patterns with repeated elements in a specific order.
| return ceng.Sequence(unique(build_statements(dd, scopes) for dd in d[key]), description=description) | |
| return ceng.Sequence([build_statements(dd, scopes) for dd in d[key]], description=description) |
| if min_location is None: | ||
| min_location = valid_locations[0] | ||
| # Filter result to only include this location | ||
| results[-1] = Result(True, child, result.children, locations={min_location}) | ||
| else: | ||
| # Find the first location that is strictly greater than min_location | ||
| found = False | ||
| for loc in valid_locations: | ||
| if loc > min_location: | ||
| min_location = loc | ||
| found = True | ||
| results[-1] = Result(True, child, result.children, locations={min_location}) | ||
| break | ||
|
|
||
| if not found: | ||
| return Result(False, self, results) |
There was a problem hiding this comment.
The logic for finding the next valid location can be simplified to be more readable and concise by using a generator expression with next(). This avoids the use of a boolean flag (found) and repeated code, making the implementation easier to maintain.
if min_location is None:
# first child, pick the smallest location.
min_location = valid_locations[0]
results[-1] = Result(True, child, result.children, locations={min_location})
else:
# subsequent children, find the smallest location that is greater than the previous one.
new_min_location = next((loc for loc in valid_locations if loc > min_location), None)
if new_min_location is None:
return Result(False, self, results)
min_location = new_min_location
results[-1] = Result(True, child, result.children, locations={min_location})| # 1 before 2 before 3 (fail, 3 is early) | ||
| assert ( | ||
| bool( | ||
| Sequence([Number(1), Number(2), Number(3)]).evaluate( | ||
| {Number(1): {ADDR1}, Number(2): {ADDR4}, Number(3): {ADDR3}} | ||
| ) | ||
| ) | ||
| is False | ||
| ) | ||
|
|
||
| # 1 before 2 before 3 (fail, 2 is late) | ||
| assert ( | ||
| bool( | ||
| Sequence([Number(1), Number(2), Number(3)]).evaluate( | ||
| {Number(1): {ADDR1}, Number(2): {ADDR4}, Number(3): {ADDR3}} | ||
| ) | ||
| ) | ||
| is False | ||
| ) |
There was a problem hiding this comment.
This test case is duplicated. Additionally, the comment fail, 3 is early is a bit misleading for the provided data. I suggest replacing the duplicated blocks with a single, clearer test case that accurately reflects the scenario of an out-of-order feature.
| # 1 before 2 before 3 (fail, 3 is early) | |
| assert ( | |
| bool( | |
| Sequence([Number(1), Number(2), Number(3)]).evaluate( | |
| {Number(1): {ADDR1}, Number(2): {ADDR4}, Number(3): {ADDR3}} | |
| ) | |
| ) | |
| is False | |
| ) | |
| # 1 before 2 before 3 (fail, 2 is late) | |
| assert ( | |
| bool( | |
| Sequence([Number(1), Number(2), Number(3)]).evaluate( | |
| {Number(1): {ADDR1}, Number(2): {ADDR4}, Number(3): {ADDR3}} | |
| ) | |
| ) | |
| is False | |
| ) | |
| # 1 before 2, but 3 is before 2 (fail) | |
| assert ( | |
| bool( | |
| Sequence([Number(1), Number(2), Number(3)]).evaluate( | |
| {Number(1): {ADDR1}, Number(2): {ADDR3}, Number(3): {ADDR2}} | |
| ) | |
| ) | |
| is False | |
| ) |
|
neat experiment! i think this is a great excuse for us all to have a call to catch up 😀 |
|
good idea! |
|
@williballenthin @mr-tz this is an effort to address FPs highlighted in mandiant/capa-rules#1037 and #2627 (comment). It'd be great to discuss what's good or bad about this PR, and/or, alternative solutions that may be a better fit for capa. The end goal is to reduce FPs, primarily for dynamic scope, and enable downstream workflows that are presently blocked by the FP rate. @williballenthin agree this is a good excuse for us all to chat 😄 |
|
As discussed yesterday, I think we should make this work for static scope, too. And, before that, make span (of calls? of features?) work for static scope, first. So we'll need to do some work to figure that out (including performance implications). We also need to nail down whether sequences can match sequences, which might be a little tricky to reason about (how do sequences overlap?). We can start with a very strict and limited implementation (like sequence can only have call-scoped children) and then loosen it as we figure things out/have a need. Ultimately, we should move forward with this research because it brings us closer to directly extracting indicators with capa, not just capabilities. |
3 participants
This PR, thanks to vibe coding, introduces a
sequencestatement to capa's rule language, enabling the enforcement of strict temporal or spatial ordering for matched features. Unlikeand, which requires only the presence of features,sequencemandates that child statements occur in increasing address order (e.g., sequential API calls in dynamic analysis or ordered instructions in static analysis). The implementation leverages a greedy matching strategy to satisfy constraints.In practice, a rule looks like:
and matches like:
versus current results: