From 49231366f1cbb800f296c60a2cb99f97466e2e33 Mon Sep 17 00:00:00 2001 From: Aayush Goel <81844215+Aayush-Goel-04@users.noreply.github.com> Date: Wed, 6 Mar 2024 16:09:21 +0530 Subject: [PATCH] Handles circular dependencies while getting rules and dependencies (#2014) * Remove test for scope "unspecified" * raise error on circular dependency * test for circular dependency --- capa/ida/plugin/view.py | 2 +- capa/rules/__init__.py | 19 ++++----- scripts/lint.py | 18 +++------ tests/test_rules.py | 89 ++++++++++++++++++----------------------- 4 files changed, 54 insertions(+), 74 deletions(-) diff --git a/capa/ida/plugin/view.py b/capa/ida/plugin/view.py index bbb8287a2..b76f4e5ad 100644 --- a/capa/ida/plugin/view.py +++ b/capa/ida/plugin/view.py @@ -196,7 +196,7 @@ def load_preview_meta(self, ea, author, scope): f" - {author}", " scopes:", f" static: {scope}", - " dynamic: unspecified", + " dynamic: unsupported", " references:", " - ", " examples:", diff --git a/capa/rules/__init__.py b/capa/rules/__init__.py index 530c8424c..a15507c97 100644 --- a/capa/rules/__init__.py +++ b/capa/rules/__init__.py @@ -152,14 +152,6 @@ def from_dict(self, scopes: Dict[str, str]) -> "Scopes": if scopes_["dynamic"] == "unsupported": scopes_["dynamic"] = None - # unspecified is used to indicate a rule is yet to be migrated. - # TODO(williballenthin): this scope term should be removed once all rules have been migrated. - # https://github.com/mandiant/capa/issues/1747 - if scopes_["static"] == "unspecified": - scopes_["static"] = None - if scopes_["dynamic"] == "unspecified": - scopes_["dynamic"] = None - if (not scopes_["static"]) and (not scopes_["dynamic"]): raise InvalidRule("invalid scopes value. At least one scope must be specified") @@ -849,7 +841,7 @@ def get_dependencies(self, namespaces): """ fetch the names of rules this rule relies upon. these are only the direct dependencies; a user must - compute the transitive dependency graph themself, if they want it. + compute the transitive dependency graph themself, if they want it. Args: namespaces(Dict[str, List[Rule]]): mapping from namespace name to rules in it. @@ -1229,11 +1221,17 @@ def get_rules_and_dependencies(rules: List[Rule], rule_name: str) -> Iterator[Ru namespaces = index_rules_by_namespace(rules) rules_by_name = {rule.name: rule for rule in rules} wanted = {rule_name} + visited = set() - def rec(rule): + def rec(rule: Rule): wanted.add(rule.name) + visited.add(rule.name) + for dep in rule.get_dependencies(namespaces): + if dep in visited: + raise InvalidRule(f'rule "{dep}" has a circular dependency') rec(rules_by_name[dep]) + visited.remove(rule.name) rec(rules_by_name[rule_name]) @@ -1602,7 +1600,6 @@ def filter_rules_by_meta(self, tag: str) -> "RuleSet": apply tag-based rule filter assuming that all required rules are loaded can be used to specify selected rules vs. providing a rules child directory where capa cannot resolve dependencies from unknown paths - TODO handle circular dependencies? TODO support -t=metafield """ rules = list(self.rules.values()) diff --git a/scripts/lint.py b/scripts/lint.py index 73bb0efff..fa85bf2db 100644 --- a/scripts/lint.py +++ b/scripts/lint.py @@ -166,9 +166,7 @@ def check_rule(self, ctx: Context, rule: Rule): class MissingStaticScope(Lint): name = "missing static scope" - recommendation = ( - "Add a static scope for the rule (file, function, basic block, instruction, or unspecified/unsupported)" - ) + recommendation = "Add a static scope for the rule (file, function, basic block, instruction, or unsupported)" def check_rule(self, ctx: Context, rule: Rule): return "static" not in rule.meta.get("scopes") @@ -176,7 +174,7 @@ def check_rule(self, ctx: Context, rule: Rule): class MissingDynamicScope(Lint): name = "missing dynamic scope" - recommendation = "Add a dynamic scope for the rule (file, process, thread, call, or unspecified/unsupported)" + recommendation = "Add a dynamic scope for the rule (file, process, thread, call, or unsupported)" def check_rule(self, ctx: Context, rule: Rule): return "dynamic" not in rule.meta.get("scopes") @@ -184,9 +182,7 @@ def check_rule(self, ctx: Context, rule: Rule): class InvalidStaticScope(Lint): name = "invalid static scope" - recommendation = ( - "For the static scope, use either: file, function, basic block, instruction, or unspecified/unsupported" - ) + recommendation = "For the static scope, use either: file, function, basic block, instruction, or unsupported" def check_rule(self, ctx: Context, rule: Rule): return rule.meta.get("scopes").get("static") not in ( @@ -194,14 +190,13 @@ def check_rule(self, ctx: Context, rule: Rule): "function", "basic block", "instruction", - "unspecified", "unsupported", ) class InvalidDynamicScope(Lint): name = "invalid static scope" - recommendation = "For the dynamic scope, use either: file, process, thread, call, or unspecified/unsupported" + recommendation = "For the dynamic scope, use either: file, process, thread, call, or unsupported" def check_rule(self, ctx: Context, rule: Rule): return rule.meta.get("scopes").get("dynamic") not in ( @@ -209,7 +204,6 @@ def check_rule(self, ctx: Context, rule: Rule): "process", "thread", "call", - "unspecified", "unsupported", ) @@ -219,8 +213,8 @@ class InvalidScopes(Lint): recommendation = "At least one scope (static or dynamic) must be specified" def check_rule(self, ctx: Context, rule: Rule): - return (rule.meta.get("scopes").get("static") in ("unspecified", "unsupported")) and ( - rule.meta.get("scopes").get("dynamic") in ("unspecified", "unsupported") + return (rule.meta.get("scopes").get("static") == "unsupported") and ( + rule.meta.get("scopes").get("dynamic") == "unsupported" ) diff --git a/tests/test_rules.py b/tests/test_rules.py index 9551387c4..15fc27c74 100644 --- a/tests/test_rules.py +++ b/tests/test_rules.py @@ -475,40 +475,6 @@ def test_meta_scope_keywords(): ) ) - # its also ok to specify "unspecified" - for static_scope in static_scopes: - _ = capa.rules.Rule.from_yaml( - textwrap.dedent( - f""" - rule: - meta: - name: test rule - scopes: - static: {static_scope} - dynamic: unspecified - features: - - or: - - format: pe - """ - ) - ) - for dynamic_scope in dynamic_scopes: - _ = capa.rules.Rule.from_yaml( - textwrap.dedent( - f""" - rule: - meta: - name: test rule - scopes: - static: unspecified - dynamic: {dynamic_scope} - features: - - or: - - format: pe - """ - ) - ) - # but at least one scope must be specified with pytest.raises(capa.rules.InvalidRule): _ = capa.rules.Rule.from_yaml( @@ -540,22 +506,6 @@ def test_meta_scope_keywords(): """ ) ) - with pytest.raises(capa.rules.InvalidRule): - _ = capa.rules.Rule.from_yaml( - textwrap.dedent( - """ - rule: - meta: - name: test rule - scopes: - static: unspecified - dynamic: unspecified - features: - - or: - - format: pe - """ - ) - ) def test_lib_rules(): @@ -1627,3 +1577,42 @@ def test_invalid_com_features(): """ ) ) + + +def test_circular_dependency(): + rules = [ + capa.rules.Rule.from_yaml( + textwrap.dedent( + """ + rule: + meta: + name: test rule 1 + scopes: + static: function + dynamic: process + lib: true + features: + - or: + - match: test rule 2 + - api: kernel32.VirtualAlloc + """ + ) + ), + capa.rules.Rule.from_yaml( + textwrap.dedent( + """ + rule: + meta: + name: test rule 2 + scopes: + static: function + dynamic: process + lib: true + features: + - match: test rule 1 + """ + ) + ), + ] + with pytest.raises(capa.rules.InvalidRule): + list(capa.rules.get_rules_and_dependencies(rules, rules[0].name))