-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add detection for Recursion in Java #14
Open
DarkaMaul
wants to merge
10
commits into
main
Choose a base branch
from
dm/java-recursion
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e25b3e4
Add detection for Recursion in Java
DarkaMaul e4e3042
java recursion more tests
GrosQuildu a0e61b4
java recursion simpler query
GrosQuildu c3213e4
update recursion query to path-problem, deduplicate results
GrosQuildu c042c9d
Update query to path-query
DarkaMaul b505f25
Update test
DarkaMaul e4c26e3
Merge branch 'dm/java-recursion' of github.com:trailofbits/codeql-que…
DarkaMaul 512a659
Update query
DarkaMaul 06cae62
Format query
DarkaMaul e930265
Use Code Review suggestions
DarkaMaul File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
{ | ||
"provide": [ | ||
"cpp/*/qlpack.yml", | ||
"go/*/qlpack.yml" | ||
"go/*/qlpack.yml", | ||
"java/*/qlpack.yml" | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
- description: Security queries for Java | ||
- queries: 'security' | ||
from: trailofbits/java-queries | ||
- exclude: | ||
tags contain: experimental |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
- description: Queries for Java | ||
- queries: '.' | ||
from: trailofbits/java-queries |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 Blog: [Low-effort denial of service with recursion](https://blog.trailofbits.com/2024/05/16/TODO/) | ||
* CWE-674: [Uncontrolled Recursion](https://cwe.mitre.org/data/definitions/674.html) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 Blog: <a href="https://blog.trailofbits.com/2024/05/16/TODO/">Low-effort denial of service with recursion</a> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO |
||
</li> | ||
<li> | ||
CWE-674: <a href="https://cwe.mitre.org/data/definitions/674.html">Uncontrolled Recursion</a> | ||
</li> | ||
</references> | ||
</qhelp> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/** | ||
* @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 MethodCall { | ||
RecursionSource() { not isTestPackage(this.getCaller().getDeclaringType()) } | ||
|
||
override string toString() { | ||
result = this.getCaller().toString() + " calls " + this.getCallee().toString() | ||
} | ||
} | ||
|
||
/** | ||
* Check if the Expr uses directly an argument of the enclosing function | ||
*/ | ||
class ParameterOperation extends Expr { | ||
ParameterOperation() { | ||
DarkaMaul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(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() instanceof RecursionSource and | ||
firstMethod = node.asExpr().(MethodCall).getCaller() | ||
} | ||
|
||
predicate isSink(DataFlow::Node node, FlowState firstMethod) { | ||
node.asExpr() instanceof RecursionSource and | ||
firstMethod.calls+(node.asExpr().(MethodCall).getCaller()) and | ||
node.asExpr().(MethodCall).getCallee().calls(firstMethod) | ||
} | ||
|
||
predicate isBarrier(DataFlow::Node node) { | ||
exists(MethodCall ma | | ||
ma = node.asExpr() and | ||
exists(Expr e | e = ma.getAnArgument() and e instanceof ParameterOperation) | ||
// or exists( | ||
// VarAccess e| | ||
// e = ma.getAnArgument() | | ||
// e.getVariable().getAnAssignedValue().getAChildExpr() instanceof ParameterOperation | ||
// ) | ||
) | ||
} | ||
|
||
/** | ||
* Weird but useful deduplication logic | ||
*/ | ||
predicate isBarrierIn(DataFlow::Node node, FlowState state) { | ||
not node.asExpr() instanceof MethodCall or | ||
node.asExpr().(MethodCall).getCaller().getLocation().getStartLine() > | ||
state.getLocation().getStartLine() | ||
} | ||
} | ||
|
||
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: " |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// 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); | ||
} | ||
// ... | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
--- | ||
name: trailofbits/java-tests | ||
authors: Trail of Bits | ||
license: AGPL | ||
extractor: java-kotlin | ||
tests: . | ||
dependencies: | ||
trailofbits/java-queries: "*" |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO