From c042c9d9cbb22f1717f0496eade8593a79f5fb17 Mon Sep 17 00:00:00 2001 From: Alexis Date: Mon, 18 Nov 2024 14:52:13 +0100 Subject: [PATCH] Update query to path-query --- java/src/security/Recursion/Recursion.ql | 56 +++++++++---- .../security/Recursion/Recursion.expected | 78 ++++++++++++++++++- .../security/Recursion/Recursion.java | 14 +++- 3 files changed, 130 insertions(+), 18 deletions(-) diff --git a/java/src/security/Recursion/Recursion.ql b/java/src/security/Recursion/Recursion.ql index 23bb16c..3fe3a11 100644 --- 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,6 +11,7 @@ */ import java +import semmle.code.java.dataflow.DataFlow predicate isTestPackage(RefType referenceType) { referenceType.getPackage().getName().toLowerCase().matches("%test%") or @@ -18,20 +19,49 @@ predicate isTestPackage(RefType referenceType) { 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; + +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 index e44e7f4..a758686 100644 --- 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 | \ No newline at end of file +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 index 4c70b5b..f5414c9 100644 --- a/java/test/query-tests/security/Recursion/Recursion.java +++ b/java/test/query-tests/security/Recursion/Recursion.java @@ -117,7 +117,7 @@ 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; @@ -125,7 +125,17 @@ public boolean directRecursiveDepth(int depth) { 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;