Skip to content
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

Merged
merged 13 commits into from
Dec 18, 2024
Merged

Add detection for Recursion in Java #14

merged 13 commits into from
Dec 18, 2024

Conversation

DarkaMaul
Copy link
Contributor

To prepare for the blogpost publication.

Copy link
Member

@mschwager mschwager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this LGTM. I added @GrosQuildu for review too.

This isn't a blocker, but I wonder if we can generalize the RecursiveCallOrderN functionality. It'd be nice to be able to specify arbitrary depth, and have the default value be 4. I think that would end up creating a cleaner query too, but probably requires some more advanced CodeQL functionality. Anyway, I was just curious if you considered this.

Also, have you done any MVRA runs to check for false positives and such?

java/src/docs/security/Recursion/Recursion.md Outdated Show resolved Hide resolved
@GrosQuildu
Copy link
Contributor

GrosQuildu commented Nov 15, 2024

I added more tests in one commit (please keep them).

In the second one, I proposed a much-simplified query that finds any recursive loops. From MVRA the query is speedy enough, but returns a lot of false positives. Would be great to limit FPs before publication with at least this heuristic: do not report if there is a depth/level argument (any number) that is changed for any call in the recursion call path.

Moreover, would be nice to print the offensive method and all methods in the call-path, so debugging is easier. We would need to use path-query instead of the problem one. Then we could also trim results by showing only one method from a cycle (e.g., for level1->level2->level3 now we show 3 results).

Added these TODOs in query code.

Lastly, finding unbounded recursive paths with high precision is probably a hard problem. Would be nice to figure additional heuristics to limit false positives. Probably the "call depth depends on user-controlled input" is the useful one, and we can model it in codeql relatively easily.

</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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

java/src/security/Recursion/Recursion.ql Outdated Show resolved Hide resolved
return directRecursiveDepth(depth - 1);
}

// todook: recursion is limited
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not make this one not be flagged :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need a local data flow; but that's not a very often pattern I guess

@DarkaMaul
Copy link
Contributor Author

I updated the query to solve massive performance issues (it was timing out on most repositories):

  1. The source is now more restricted : instead of trying to store MethodCall, we store Method and already check that they are part of recursions chains (this restricted sources from 600k results to 500 on Elasticsearch)
  2. Consider getCallee instead of getCaller to further reduce the number of candidates
  3. 😢 Remove the deduplication logic: I have tried several approaches, but they all failed for large codebase. I don't know if it's a bug with isBarrierIn/Out with FlowState, or something I don't understand about how the predicate is resolved.

@DarkaMaul
Copy link
Contributor Author

Of note, the CI is failing for a Go query :/

@GrosQuildu
Copy link
Contributor

LGTM; we can solve deduplication later. Please open an issue for it and please describe the unsuccessful attempts.

From curiosity, could you check what is the performance and false-positive rate of c3213e4 version?

CI is failing for another query, gonna fix it in a new PR.

@DarkaMaul DarkaMaul requested a review from fegge as a code owner December 18, 2024 13:07
@mschwager mschwager requested a review from GrosQuildu December 18, 2024 14:02
Copy link
Member

@mschwager mschwager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to move forward with external references.

@DarkaMaul DarkaMaul dismissed GrosQuildu’s stale review December 18, 2024 14:40

Changes acknowledged.

@DarkaMaul DarkaMaul merged commit fd6e8cd into main Dec 18, 2024
1 of 3 checks passed
@DarkaMaul DarkaMaul deleted the dm/java-recursion branch December 18, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants