Add detection for Recursion in Java (#14)

Author: DarkaMaul

.codeqlmanifest.json

@@ -1,6 +1,7 @@
 {
     "provide": [
         "cpp/*/qlpack.yml",
-        "go/*/qlpack.yml"
+        "go/*/qlpack.yml",
+        "java/*/qlpack.yml"
     ]
 }

.github/workflows/test.yml

@@ -16,3 +16,4 @@ jobs:
         run: |
           ${{ steps.init.outputs.codeql-path }} test run ./cpp/test/
           ${{ steps.init.outputs.codeql-path }} test run ./go/test/
+          ${{ steps.init.outputs.codeql-path }} test run ./java/test/

README.md

@@ -66,6 +66,14 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
 |[Missing MinVersion in tls.Config](./go/src/docs/security/MissingMinVersionTLS/MissingMinVersionTLS.md)|This rule finds cases when you do not set the `tls.Config.MinVersion` explicitly for servers. By default version 1.0 is used, which is considered insecure. This rule does not mark explicitly set insecure versions|error|medium|
 |[Trim functions misuse](./go/src/docs/security/TrimMisuse/TrimMisuse.md)|Finds calls to `string.{Trim,TrimLeft,TrimRight}` with the 2nd argument not being a cutset but a continuous substring to be trimmed|error|low|
 
+### Java-kotlin
+
+#### Security
+
+| Name | Description | Severity | Precision  |
+| ---  | ----------- | :----:   | :--------: |
+|[Recursive functions](./java-kotlin/src/docs/security/Recursion/Recursion.md)|Detects recursive calls|warning|low|
+
 ## Query suites
 
 CodeQL queries are grouped into "suites". To execute queries from a specific suit add its name after a colon: `trailofbits/cpp-queries:codeql-suites/tob-cpp-full.qls`.
@@ -89,7 +97,7 @@ echo "--search-path '$PWD/codeql-queries'" > "${HOME}/.config/codeql/config"
 
 Check that CodeQL CLI detects the new qlpacks:
 ```sh
-codeql resolve qlpacks | grep trailofbits
+codeql resolve packs | grep trailofbits
 ```
 
 #### Before committing
@@ -99,6 +107,7 @@ Run tests:
 cd codeql-queries
 codeql test run ./cpp/test
 codeql test run ./go/test
+codeql test run ./java/test
 ```
 
 Update dependencies:
@@ -115,4 +124,5 @@ Generate markdown query help files
 ```sh
 codeql generate query-help ./cpp/src/ --format=markdown --output ./cpp/src/docs
 codeql generate query-help ./go/src/ --format=markdown --output ./go/src/docs
+codeql generate query-help ./java/src/ --format=markdown --output ./java/src/docs
 ```

java/src/codeql-pack.lock.yml

@@ -0,0 +1,28 @@
+---
+lockVersion: 1.0.0
+dependencies:
+  codeql/dataflow:
+    version: 1.1.5
+  codeql/java-all:
+    version: 4.2.0
+  codeql/mad:
+    version: 1.0.11
+  codeql/rangeanalysis:
+    version: 1.0.11
+  codeql/regex:
+    version: 1.0.11
+  codeql/ssa:
+    version: 1.0.11
+  codeql/threat-models:
+    version: 1.0.11
+  codeql/tutorial:
+    version: 1.0.11
+  codeql/typeflow:
+    version: 1.0.11
+  codeql/typetracking:
+    version: 1.0.11
+  codeql/util:
+    version: 1.0.11
+  codeql/xml:
+    version: 1.0.11
+compiled: false

java/src/codeql-suites/tob-java-code-scanning.qls

@@ -0,0 +1,5 @@
+- description: Security queries for Java
+- queries: 'security'
+  from: trailofbits/java-queries
+- exclude:
+    tags contain: experimental

java/src/codeql-suites/tob-java-full.qls

@@ -0,0 +1,3 @@
+- description: Queries for Java
+- queries: '.'
+  from: trailofbits/java-queries

java/src/docs/security/Recursion/Recursion.md

@@ -0,0 +1,39 @@
+# Recursive functions
+Recursive functions are methods that call themselves either directly or indirectly through other functions. While recursion can be a powerful programming technique, unbounded recursion on user inputs can lead to stack overflow errors and program crashes, potentially enabling denial of service attacks. This query detects recursive patterns up to order 4.
+
+
+## Recommendation
+Review recursive functions and ensure that they are either: - Not processing user-controlled data - The data has been properly sanitized before recursing - The recursion has an explicit depth limit
+
+Consider replacing recursion with iterative alternatives where possible.
+
+
+## Example
+
+```java
+// From https://github.com/x-stream/xstream/blob/dfa1d35462fe84412ee72a9b0cf5b5c633086520/xstream/src/java/com/thoughtworks/xstream/io/binary/BinaryStreamReader.java#L165
+private Token readToken() {
+    // ...
+    try {
+        final Token token = tokenFormatter.read(in);
+        switch (token.getType()) {
+        case Token.TYPE_MAP_ID_TO_VALUE: // 0x2
+            idRegistry.put(token.getId(), token.getValue());
+            return readToken(); // Next one please.
+        default:
+            return token;
+        }
+    } catch (final IOException e) {
+        throw new StreamException(e);
+    }
+    // ...
+}
+```
+In this example, a binary stream reader processes tokens recursively.
+
+For each new token \`0x2\`, the parser will create a new recursive call. If this stream is user-controlled, an attacker can generate too many stackframes and crash the application with a `StackOverflow` error.
+
+
+## References
+* Trail Of Bits white paper: [Input-Driven Recursion](https://resources.trailofbits.com/input-driven-recursion-white-paper)
+* CWE-674: [Uncontrolled Recursion](https://cwe.mitre.org/data/definitions/674.html)

java/src/qlpack.yml

@@ -0,0 +1,12 @@
+---
+name: trailofbits/java-queries
+description: CodeQL queries for Java developed by Trail of Bits
+authors: Trail of Bits
+version: 0.0.1
+license: AGPL
+library: false
+extractor: java-kotlin
+dependencies:
+  codeql/java-all: "*"
+suites: codeql-suites
+defaultSuiteFile: codeql-suites/tob-java-code-scanning.qls

java/src/security/Recursion/Recursion.qhelp

@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+        Recursive functions are methods that call themselves either directly or indirectly through other functions.
+        While recursion can be a powerful programming technique, unbounded recursion on user inputs can lead
+        to stack overflow errors and program crashes, potentially enabling denial of service attacks.
+
+        This query detects recursive patterns up to order 4.
+        </p>
+
+    </overview>
+    <recommendation>
+        <p>
+        Review recursive functions and ensure that they are either:
+            - Not processing user-controlled data
+            - The data has been properly sanitized before recursing
+            - The recursion has an explicit depth limit
+        </p>
+        <p>
+        Consider replacing recursion with iterative alternatives where possible.
+        </p>
+    </recommendation>
+    <example>
+        <sample src="RecursiveCall.java" />
+        <p>In this example, a binary stream reader processes tokens recursively.</p>
+        <p>For each new token `0x2`, the parser will create a new recursive call.
+        If this stream is user-controlled, an attacker can generate too many stackframes
+        and crash the application with a <code>StackOverflow</code> error.</p>
+    </example>
+    <references>
+        <li>
+            Trail Of Bits white paper: <a href="https://resources.trailofbits.com/input-driven-recursion-white-paper">Input-Driven Recursion</a>
+        </li>
+        <li>
+            CWE-674: <a href="https://cwe.mitre.org/data/definitions/674.html">Uncontrolled Recursion</a>
+        </li>
+    </references>
+</qhelp>

java/src/security/Recursion/Recursion.ql

@@ -0,0 +1,81 @@
+/**
+ * @name Recursive functions
+ * @id tob/java/unbounded-recursion
+ * @description Detects possibly unbounded recursive calls
+ * @kind path-problem
+ * @tags security
+ * @precision low
+ * @problem.severity warning
+ * @security-severity 3.0
+ * @group security
+ */
+
+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 RecursionSource extends Method {
+  RecursionSource() {
+    not isTestPackage(this.getDeclaringType()) and
+    this.calls+(this)
+  }
+}
+
+/**
+ * Check if the Expr uses directly an argument of the enclosing function
+ */
+class ParameterOperation extends Expr {
+  ParameterOperation() {
+    (this instanceof BinaryExpr or this instanceof UnaryAssignExpr) and
+    exists(VarAccess va | va.getVariable() = this.getEnclosingCallable().getAParameter() |
+      this.getAChildExpr+() = va
+    )
+  }
+}
+
+module RecursiveConfig implements DataFlow::StateConfigSig {
+  class FlowState = Method;
+
+  predicate isSource(DataFlow::Node node, FlowState firstMethod) {
+    node.asExpr().(MethodCall).getCallee() instanceof RecursionSource and
+    firstMethod = node.asExpr().(MethodCall).getCallee()
+  }
+
+  predicate isSink(DataFlow::Node node, FlowState firstMethod) {
+    node.asExpr().(MethodCall).getCallee().calls(firstMethod) and
+    firstMethod.calls+(node.asExpr().(MethodCall).getCaller())
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    exists(MethodCall ma |
+      ma = node.asExpr() and
+      exists(Expr e | e = ma.getAnArgument() and e instanceof ParameterOperation)
+    )
+  }
+  // /**
+  //  * Weird but useful deduplication logic
+  //  */
+  // predicate isBarrierOut(DataFlow::Node node, FlowState state) {
+  //   node.asExpr().(MethodCall).getCallee().getName() > state.getName()
+  // }
+}
+
+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)
+ */
+
+from RecursiveFlow::PathNode source, RecursiveFlow::PathNode sink
+where RecursiveFlow::flowPath(source, sink)
+select sink.getNode(), source, sink, "Found a recursion: "