[pulse][taint] add java.sql.Connection SQL injection sinks - #2108
Open
jeremydubreil wants to merge 1 commit into
Open
[pulse][taint] add java.sql.Connection SQL injection sinks#2108jeremydubreil wants to merge 1 commit into
jeremydubreil wants to merge 1 commit into
Conversation
The Java taint test config declares java.sql.Statement.{addBatch, execute,
executeQuery, executeUpdate, executeLargeUpdate} as SQL sinks, but nothing on
java.sql.Connection. Preparing a statement or a call from a user-controlled
string is already an injection, even though the resulting PreparedStatement
would be safe if the query were a constant, so add prepareStatement and
prepareCall as SQLInjection sinks.
The new taint/Jdbc.java exercises them alongside two patterns the Java taint
tests did not cover at all:
- reaching a SQL sink through string concatenation. Services.java passes an
endpoint parameter straight to the sink, whereas real injections are
almost always built with '+', which javac compiles to
Object.makeConcatWithConstants. That is a sanitizer for
StringConcatenation only, a kind that no SQL policy lists, so these flows
are reported as expected.
- binding the user-controlled value as a parameter of a constant query,
which is the recommended mitigation and must not be reported.
The sinks are pinned to ArgumentPositions [0] rather than left to default to
AllArguments so that prepareStatementWithOptionsBad reports once, on the query,
and not also on the two int result-set options.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The Java taint test config declares
java.sql.Statement.{addBatch, execute, executeQuery, executeUpdate, executeLargeUpdate}as SQL sinks, but nothing onjava.sql.Connection. Preparing a statement or a call from a user-controlled string is already an injection, even though the resultingPreparedStatementwould be safe if the query were a constant. This kind of taint flow happen surprisingly often in practice. This PR addsprepareStatementandprepareCallas SQLInjection sinks.The new taint/Jdbc.java exercises them alongside two patterns the Java taint tests did not cover at all:
reaching a SQL sink through string concatenation. Services.java passes an endpoint parameter straight to the sink, whereas real injections are almost always built with '+', which javac compiles to Object.makeConcatWithConstants. That is a sanitizer for StringConcatenation only, a kind that no SQL policy lists, so these flows are reported as expected.
binding the user-controlled value as a parameter of a constant query, which is the recommended mitigation and must not be reported.
The sinks are pinned to
ArgumentPositions [0]rather than left to default to AllArguments so that prepareStatementWithOptionsBad reports once, on the query, and not also on the two int result-set options.