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

@@ -196,7 +196,7 @@ class CapaExplorerRulegenPreview(QtWidgets.QTextEdit):
             f"      - {author}",
             "    scopes:",
             f"      static: {scope}",
-            "      dynamic: unspecified",
+            "      dynamic: unsupported",
             "    references:",
             "      - <insert_references>",
             "    examples:",

capa/rules/__init__.py

@@ -152,14 +152,6 @@ class 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 @@ class Rule:
         """
         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 @@ class 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 <k>
         """
         rules = list(self.rules.values())

scripts/lint.py

@@ -166,9 +166,7 @@ class MissingScopes(Lint):
 
 class MissingStaticScope(Lint):
     name = "missing static scope"
-    recommendation = (
+    recommendation = "Add a static scope for the rule (file, function, basic block, instruction, or unsupported)"
-        "Add a static scope for the rule (file, function, basic block, instruction, or unspecified/unsupported)"
-    )
 
     def check_rule(self, ctx: Context, rule: Rule):
         return "static" not in rule.meta.get("scopes")
@@ -176,7 +174,7 @@ class MissingStaticScope(Lint):
 
 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 @@ class MissingDynamicScope(Lint):
 
 class InvalidStaticScope(Lint):
     name = "invalid static scope"
-    recommendation = (
+    recommendation = "For the static scope, use either: file, function, basic block, instruction, or unsupported"
-        "For the static scope, use either: file, function, basic block, instruction, or unspecified/unsupported"
-    )
 
     def check_rule(self, ctx: Context, rule: Rule):
         return rule.meta.get("scopes").get("static") not in (
@@ -194,14 +190,13 @@ class InvalidStaticScope(Lint):
             "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 @@ class InvalidDynamicScope(Lint):
             "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 (
+        return (rule.meta.get("scopes").get("static") == "unsupported") and (
-            rule.meta.get("scopes").get("dynamic") in ("unspecified", "unsupported")
+            rule.meta.get("scopes").get("dynamic") == "unsupported"
         )
 
 

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))