Rule: java:S2077
Version used: 9.9 Datacenter Edition
For our 9.9 upgrade I am looking into getting rid of non-SonarQube provided rules that are currently used in 8.9. One of that rules is findbugs:SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE which basically checks if SQL statements are concatenated somehow before they are executed. I looked at the rules that SonarQube offers and S2077 looked like a good match. Some of the obvious cases are identified, but I see 1 pattern constantly being used in various of our applications, which is currently not detected by S2077.
here is a sample code:
public String getUser(Connection conn, String uid) throws SQLException {
String query = "SELECT name FROM user where uid = '" + uid + "'";
execSql(conn, query);
try(PreparedStatement stmt = conn.prepareStatement(query)) { // detected by java:S2077
try(ResultSet rs = stmt.executeQuery()) {
if(rs.next()) {
return rs.getString(1);
}
}
}
return null;
}
public String execSql(Connection conn, String query) throws SQLException {
try(Statement stmt = conn.createStatement()) {
try(ResultSet rs = stmt.executeQuery(query)) { // not detected by java:S2077
if(rs.next()) {
return rs.getString(1);
}
}
}
return null;
}
the obvious concatenation in getUser is detected, but if the exec and the concat is not happening in the same method it is not detected. Is this something S2077 should be able to cover?
another case that is not detected by S2077.
here all happens in the same method, but not by concatenating strings, but appending to a StringBuffer. Its the same for StringBuilder.
public String getUser(Connection conn, String table) throws SQLException {
StringBuffer buf = new StringBuffer();
buf.append("SELECT name FROM ")
.append(table)
.append("where uid = ?");
try(PreparedStatement stmt = conn.prepareStatement(buf.toString())) { // not detected by java:S2077
try(ResultSet rs = stmt.executeQuery()) {
if(rs.next()) {
return rs.getString(1);
}
}
}
}
does this qualify as “dynamically formatted SQL query” ?
Hello Roman,
S2077 is a non-interprocedural rule. That means, it is not able to track strings across different functions. As such, the first case is not something the rule can cover, unfortunately. For a more advanced analysis, we have S3649 which uses taint analysis to track user input throughout the application. This rule is not available in the Community Edition though.
I understand. We use S3649 as well to identify SQLI’s but it is tough to identify all potential taint sources in hundreds of applications.
We want to identify the bad coding when people are concatenating SQL strings. after a manual review some of the cases are ok (for example: concatenating with schema names from configuration), but there are also cases where values from unknown sources are concatenated and where it should be easy to fix them.
So for now we still have to rely on the 2 FindBugs rules that are currently doing this job for us:
findbugs:SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE
findbugs:SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING
Unfortunately, for us, these rules still find thousands of issues.
Thanks four your support