Potential fix for code scanning alert no. 45: Query built from user-controlled sources#7
Open
davewichers wants to merge 1 commit intomainfrom
Open
Potential fix for code scanning alert no. 45: Query built from user-controlled sources#7davewichers wants to merge 1 commit intomainfrom
davewichers wants to merge 1 commit intomainfrom
Conversation
…ontrolled sources Fix SQLi in Benchmark00603.java:64 Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
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
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.
Potential fix for https://github.com/aspectsecurity/TestCodeQL/security/code-scanning/45
In general, the problem is that user-controlled data (
bar) is concatenated into an SQL statement string that is then executed usingjava.sql.Statement. To fix this, we should switch to using aPreparedStatementwith parameter placeholders (?) for any dynamic values, and then bind the untrusted value via the appropriatesetXxxmethod. This way, the database driver treats the password as data, not as part of the SQL syntax, preventing SQL injection.The best fix here, without changing existing functionality, is to change the construction and execution of the query in
Benchmark00603.doPostso that:?placeholder instead of concatenatingbar.java.sql.PreparedStatementis obtained from the same connection helper (by callinggetSqlConnection()or similar). Since we are not allowed to change helpers we haven’t seen, we will obtain ajava.sql.Connectionusingorg.owasp.benchmark.helpers.DatabaseHelper.getSqlConnection()(which is a standard helper in this project) and create aPreparedStatementfrom it.barvalue is bound to the placeholder usingpreparedStatement.setString(1, bar);.executeQuery()orexecuteUpdate()as appropriate; to maintain similar behavior (and to keepprintResultsusage aligned), we can useexecuteQuery()and pass the original SQL string (with the literal?placeholder) toprintResults.Concretely, in
src/main/java/org/owasp/benchmark/testcode/Benchmark00603.java, we will:String sql = ...line to use a placeholder instead of concatenation.java.sql.Statement statement = ...; statement.execute(...);block with code that:java.sql.ConnectionfromDatabaseHelper.getSqlConnection().connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)).setString(1, bar).PreparedStatement(which implementsStatement) and the SQL string toDatabaseHelper.printResults.Thing1.javadoes not need changes for the SQLi fix; it simply passes through the value and has no database logic.Suggested fixes powered by Copilot Autofix. Review carefully before merging.