Update query to path-query

trailofbits · Nov 18, 2024 · c042c9d · c042c9d
1 parent a0e61b4
commit c042c9d
Show file tree

Hide file tree

Showing 3 changed files with 130 additions and 18 deletions.
diff --git a/java/src/security/Recursion/Recursion.ql b/java/src/security/Recursion/Recursion.ql
@@ -2,7 +2,7 @@
  * @name Recursive functions
  * @id tob/java/unbounded-recursion
  * @description Detects possibly unbounded recursive calls
- * @kind problem
+ * @kind path-problem
  * @tags security
  * @precision low
  * @problem.severity warning
@@ -11,27 +11,57 @@
  */
 
 import java
+import semmle.code.java.dataflow.DataFlow
 
 predicate isTestPackage(RefType referenceType) {
   referenceType.getPackage().getName().toLowerCase().matches("%test%") or
   referenceType.getPackage().getName().toLowerCase().matches("%benchmark%") or
   referenceType.getName().toLowerCase().matches("%test%")
 }
 
-class RecursiveMethod extends Method {
-  RecursiveMethod() {
-    exists(Method m | this.calls+(m) and this = m)
+class RecursionSource extends MethodCall {
+  RecursionSource() { not isTestPackage(this.getCaller().getDeclaringType()) }
+
+  override string toString() {
+    result = this.getCaller().toString() + " calls " + this.getCallee().toString()
   }
 }
 
-/**
- * TODO ideas:
- *   - limit results to methods that take any user input 
- *   - check if recursive calls are bounded (takes argument that is decremented for every call)
+module RecursiveConfig implements DataFlow::StateConfigSig {
+  class FlowState = Method;
+
+  predicate isSource(DataFlow::Node node, FlowState state) {
+    node.asExpr() instanceof RecursionSource and
+    state = node.asExpr().(MethodCall).getCaller()
+  }
+
+  predicate isSink(DataFlow::Node node, FlowState state) {
+    node.asExpr() instanceof RecursionSource and
+    state.calls+(node.asExpr().(MethodCall).getCaller()) and
+    node.asExpr().(MethodCall).getCallee().calls(state)
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node.asExpr() instanceof MethodCall and
+    exists(Expr arg | arg = node.asExpr().(MethodCall).getAnArgument() |
+      arg instanceof BinaryExpr or
+      exists(BinaryExpr b | DataFlow::localFlow(DataFlow::exprNode(b), DataFlow::exprNode(arg)))
+    )
+  }
+}
+
+module RecursiveFlow = DataFlow::GlobalWithState<RecursiveConfig>;
+
+import RecursiveFlow::PathGraph
+
+/*
+ * TODO: This query could be improved with the following ideas:
+ *   - limit results to methods that take any user input
  *   - do not return methods that have calls to self (or unbounded recursion) that are conditional
- *   - gather and print whole call graph (list of calls from recursiveMethod to itself) 
+ *   - gather and print whole call graph (list of calls from recursiveMethod to itself)
  */
-from RecursiveMethod recursiveMethod
-where
-  not isTestPackage(recursiveMethod.getDeclaringType())
-select recursiveMethod, "Method $@ has unbounded, possibly infinite recursive calls", recursiveMethod, recursiveMethod.toString()
+
+from RecursiveFlow::PathNode source, RecursiveFlow::PathNode sink
+where RecursiveFlow::flowPath(source, sink)
+// TODO(dm): de-duplicate results
+select sink.getNode(), source, sink, "Found a recursion: "
diff --git a/java/test/query-tests/security/Recursion/Recursion.expected b/java/test/query-tests/security/Recursion/Recursion.expected
@@ -1,3 +1,75 @@
-| Recursion.java:8:16:8:20 | bar(...) | $@ | Recursion.java:8:16:8:20 | bar(...) | Recursion(2): bar(2) -> foo(7) -> bar |
-| Recursion.java:12:16:12:32 | directRecursive(...) | $@ | Recursion.java:12:16:12:32 | directRecursive(...) | Recursion(1): directRecursive(11) -> directRecursive |
-| Recursion.java:31:16:31:23 | level0(...) | $@ | Recursion.java:31:16:31:23 | level0(...) | Recursion(3): level0(15) -> level1(21) -> level2(27) -> level0 |
+edges
+| Recursion.java:53:33:53:55 | readToken calls read : Token | Recursion.java:59:24:59:28 | token : Token | provenance |  |
+| Recursion.java:57:24:57:34 | readToken calls readToken : Token | Recursion.java:57:24:57:34 | readToken calls readToken | provenance |  |
+| Recursion.java:57:24:57:34 | readToken calls readToken : Token | Recursion.java:57:24:57:34 | readToken calls readToken : Token | provenance |  |
+| Recursion.java:59:24:59:28 | token : Token | Recursion.java:57:24:57:34 | readToken calls readToken | provenance |  |
+| Recursion.java:59:24:59:28 | token : Token | Recursion.java:57:24:57:34 | readToken calls readToken : Token | provenance |  |
+| Recursion.java:71:29:71:33 | bar calls foo : Boolean | Recursion.java:72:16:72:24 | fooResult : Boolean | provenance |  |
+| Recursion.java:71:29:71:33 | bar calls foo : Boolean | Recursion.java:72:16:72:24 | fooResult : Boolean | provenance |  |
+| Recursion.java:72:16:72:24 | fooResult : Boolean | Recursion.java:76:16:76:20 | foo calls bar | provenance |  |
+| Recursion.java:72:16:72:24 | fooResult : Boolean | Recursion.java:76:16:76:20 | foo calls bar : Boolean | provenance |  |
+| Recursion.java:72:16:72:24 | fooResult : Boolean | Recursion.java:76:16:76:20 | foo calls bar : Boolean | provenance |  |
+| Recursion.java:76:16:76:20 | foo calls bar : Boolean | Recursion.java:71:29:71:33 | bar calls foo | provenance |  |
+| Recursion.java:76:16:76:20 | foo calls bar : Boolean | Recursion.java:71:29:71:33 | bar calls foo : Boolean | provenance |  |
+| Recursion.java:76:16:76:20 | foo calls bar : Boolean | Recursion.java:71:29:71:33 | bar calls foo : Boolean | provenance |  |
+| Recursion.java:81:16:81:32 | directRecursive calls directRecursive : Boolean | Recursion.java:81:16:81:32 | directRecursive calls directRecursive | provenance |  |
+| Recursion.java:81:16:81:32 | directRecursive calls directRecursive : Boolean | Recursion.java:81:16:81:32 | directRecursive calls directRecursive : Boolean | provenance |  |
+| Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | Recursion.java:101:16:101:23 | level2 calls level0 | provenance |  |
+| Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | provenance |  |
+| Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | provenance |  |
+| Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | provenance |  |
+| Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | Recursion.java:89:16:89:23 | level0 calls level1 | provenance |  |
+| Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | provenance |  |
+| Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | provenance |  |
+| Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | provenance |  |
+| Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | Recursion.java:95:16:95:23 | level1 calls level2 | provenance |  |
+| Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | provenance |  |
+| Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | provenance |  |
+| Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | provenance |  |
+| Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth : Boolean | Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth | provenance |  |
+| Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth : Boolean | Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth : Boolean | provenance |  |
+nodes
+| Recursion.java:53:33:53:55 | readToken calls read : Token | semmle.label | readToken calls read : Token |
+| Recursion.java:57:24:57:34 | readToken calls readToken | semmle.label | readToken calls readToken |
+| Recursion.java:57:24:57:34 | readToken calls readToken : Token | semmle.label | readToken calls readToken : Token |
+| Recursion.java:59:24:59:28 | token : Token | semmle.label | token : Token |
+| Recursion.java:71:29:71:33 | bar calls foo | semmle.label | bar calls foo |
+| Recursion.java:71:29:71:33 | bar calls foo : Boolean | semmle.label | bar calls foo : Boolean |
+| Recursion.java:71:29:71:33 | bar calls foo : Boolean | semmle.label | bar calls foo : Boolean |
+| Recursion.java:72:16:72:24 | fooResult : Boolean | semmle.label | fooResult : Boolean |
+| Recursion.java:72:16:72:24 | fooResult : Boolean | semmle.label | fooResult : Boolean |
+| Recursion.java:76:16:76:20 | foo calls bar | semmle.label | foo calls bar |
+| Recursion.java:76:16:76:20 | foo calls bar : Boolean | semmle.label | foo calls bar : Boolean |
+| Recursion.java:76:16:76:20 | foo calls bar : Boolean | semmle.label | foo calls bar : Boolean |
+| Recursion.java:81:16:81:32 | directRecursive calls directRecursive | semmle.label | directRecursive calls directRecursive |
+| Recursion.java:81:16:81:32 | directRecursive calls directRecursive : Boolean | semmle.label | directRecursive calls directRecursive : Boolean |
+| Recursion.java:89:16:89:23 | level0 calls level1 | semmle.label | level0 calls level1 |
+| Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | semmle.label | level0 calls level1 : Boolean |
+| Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | semmle.label | level0 calls level1 : Boolean |
+| Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | semmle.label | level0 calls level1 : Boolean |
+| Recursion.java:95:16:95:23 | level1 calls level2 | semmle.label | level1 calls level2 |
+| Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | semmle.label | level1 calls level2 : Boolean |
+| Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | semmle.label | level1 calls level2 : Boolean |
+| Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | semmle.label | level1 calls level2 : Boolean |
+| Recursion.java:101:16:101:23 | level2 calls level0 | semmle.label | level2 calls level0 |
+| Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | semmle.label | level2 calls level0 : Boolean |
+| Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | semmle.label | level2 calls level0 : Boolean |
+| Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | semmle.label | level2 calls level0 : Boolean |
+| Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth | semmle.label | directRecursiveNoDepth calls directRecursiveNoDepth |
+| Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth : Boolean | semmle.label | directRecursiveNoDepth calls directRecursiveNoDepth : Boolean |
+subpaths
+#select
+| Recursion.java:57:24:57:34 | readToken calls readToken | Recursion.java:53:33:53:55 | readToken calls read : Token | Recursion.java:57:24:57:34 | readToken calls readToken | Found a recursion:  |
+| Recursion.java:57:24:57:34 | readToken calls readToken | Recursion.java:57:24:57:34 | readToken calls readToken | Recursion.java:57:24:57:34 | readToken calls readToken | Found a recursion:  |
+| Recursion.java:57:24:57:34 | readToken calls readToken | Recursion.java:57:24:57:34 | readToken calls readToken : Token | Recursion.java:57:24:57:34 | readToken calls readToken | Found a recursion:  |
+| Recursion.java:71:29:71:33 | bar calls foo | Recursion.java:71:29:71:33 | bar calls foo | Recursion.java:71:29:71:33 | bar calls foo | Found a recursion:  |
+| Recursion.java:71:29:71:33 | bar calls foo | Recursion.java:71:29:71:33 | bar calls foo : Boolean | Recursion.java:71:29:71:33 | bar calls foo | Found a recursion:  |
+| Recursion.java:76:16:76:20 | foo calls bar | Recursion.java:76:16:76:20 | foo calls bar | Recursion.java:76:16:76:20 | foo calls bar | Found a recursion:  |
+| Recursion.java:76:16:76:20 | foo calls bar | Recursion.java:76:16:76:20 | foo calls bar : Boolean | Recursion.java:76:16:76:20 | foo calls bar | Found a recursion:  |
+| Recursion.java:81:16:81:32 | directRecursive calls directRecursive | Recursion.java:81:16:81:32 | directRecursive calls directRecursive | Recursion.java:81:16:81:32 | directRecursive calls directRecursive | Found a recursion:  |
+| Recursion.java:81:16:81:32 | directRecursive calls directRecursive | Recursion.java:81:16:81:32 | directRecursive calls directRecursive : Boolean | Recursion.java:81:16:81:32 | directRecursive calls directRecursive | Found a recursion:  |
+| Recursion.java:89:16:89:23 | level0 calls level1 | Recursion.java:101:16:101:23 | level2 calls level0 : Boolean | Recursion.java:89:16:89:23 | level0 calls level1 | Found a recursion:  |
+| Recursion.java:95:16:95:23 | level1 calls level2 | Recursion.java:89:16:89:23 | level0 calls level1 : Boolean | Recursion.java:95:16:95:23 | level1 calls level2 | Found a recursion:  |
+| Recursion.java:101:16:101:23 | level2 calls level0 | Recursion.java:95:16:95:23 | level1 calls level2 : Boolean | Recursion.java:101:16:101:23 | level2 calls level0 | Found a recursion:  |
+| Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth | Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth | Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth | Found a recursion:  |
+| Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth | Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth : Boolean | Recursion.java:115:16:115:54 | directRecursiveNoDepth calls directRecursiveNoDepth | Found a recursion:  |
diff --git a/java/test/query-tests/security/Recursion/Recursion.java b/java/test/query-tests/security/Recursion/Recursion.java
@@ -117,15 +117,25 @@ public boolean directRecursiveNoDepth(int anything, int depth) {
 }
 
 class RecursiveCallLimited {
-    // todook: recursion is limited
+    // ok: recursion is limited
     public boolean directRecursiveDepth(int depth) {
         if (depth == 0) {
             return true;
         }
         return directRecursiveDepth(depth - 1);
     }
 
-    // todook: level0->level1->level2->level0 with bound
+    // ok: recursion is limited
+    public boolean directRecursiveComputedDepth(int depth) {
+        if (depth == 0) {
+            return true;
+        }
+
+        int newDepth = depth - 2;
+        return directRecursiveComputedDepth(newDepth);
+    }
+
+    // ok: level0->level1->level2->level0 with bound
     public boolean level0D(int depth) {
         if (depth == 0) {
             return true;