Potential fix for code scanning alert no. 45: Query built from user-controlled sources #7
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.