-
Notifications
You must be signed in to change notification settings - Fork 12
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
[STAL-2195] Initial implementation of intra-method taint analysis in Java #493
Merged
Conversation
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
jasonforal
force-pushed
the
jf/STAL-2635
branch
2 times, most recently
from
August 27, 2024 14:47
1e9bcb9
to
c8023ba
Compare
jasonforal
force-pushed
the
jf/STAL-2195
branch
from
August 27, 2024 16:52
a5f3b9f
to
97d9ee5
Compare
juli1
approved these changes
Aug 30, 2024
jasonforal
force-pushed
the
jf/STAL-2635
branch
from
September 3, 2024 15:20
c8023ba
to
5344bd6
Compare
…of tree-sitter nodes.
jasonforal
force-pushed
the
jf/STAL-2195
branch
from
September 3, 2024 15:41
97d9ee5
to
4903f2b
Compare
* @param {TreeSitterNode} parent An ancestor of {@link nodes} which should receive any tainted return values. | ||
* @param {Array<TreeSitterNode>} nodes | ||
*/ | ||
_visitExprStmtList(parent, nodes) { |
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What problem are you trying to solve?
Some significant security vulnerabilities (e.g. SQL injection) can be caught with taint analysis.
Currently, writing accurate rules to detect these vulnerabilities is difficult because we can currently only very crudely approximate the flow of variables through a program. To address this, we will build "native support" for (presently: intra-method) taint analysis for Java.
What is your solution?
Note
This is a large PR. All commits are intentionally organized and purely additive to each other. It's likely easier to review commits individually, in sequential order.
Preface
Our goal is to implement simple taint analysis in a way that builds on our current CST-based technique.
Methodology
Note
Terminology:
Source: A place in a source code where data originates (e.g. the headers from an HTTP request).
Sink: A place in a source code where that data can end up (and particularly that this data could have harmful side effects -- e.g. a SQL statement).
MethodFlow
class recursively traverses that containing method, iteratively constructing a directed graph that represents a backwards flow analysis. This graph has CST nodes for vertices, and assignment/dependence relationships for edges.TaintFlow
, and so the rule can perform its own logic. This unit test shows how these APIs are used and what they expose.Limitations
Some limitations are artificial and were chosen for simplicity. These can be addressed in future iterations:
field_access
)Out of Scope
Features/abstractions this PR does not implement:
RuleResult
and in the SARIF output.Alternatives considered
What the reviewer should know
parenthesized_expression
).TreeSitterNode.type
toTreeSitterNode.cstType
was overdue and so was squeezed in here.cstType
, we still do it for implementation simplicity.TsLanguageContext
implements, but the abstraction is not 1:1 for this use case, so it would require a small refactor -- deemed out of scope for this PR).