Security - Potential JDBC Injection (false positive)

[Tony 79]

SonarQube 5.6.5

sonar-java-plugin-4.14.0.11784.jar

I have many reports from developers for the rule in question.

Below the source code:

public class StringConnectionCallback implements ConnectionCallback {

 

protected String testo;

private String query;

 

public StringConnectionCallback(String id) {

this.testo = id;

}

 

public StringConnectionCallback(String id, String callString) {

this.testo = id;

this.query = callString;

}

 

public Object doInConnection(Connection connection) throws SQLException {

PreparedStatement statement = null;

try {

statement = connection.prepareCall(query);

This use of Connection.prepareCall(...) can be vulnerable to SQL injection   

statement.setString(1, testo);

statement.execute();

} finally {

JdbcUtils.closeStatement(statement);

}

return null;

}

}

[Tony 79]

Please help....

I'm wrong to figure something out or it's better to turn off the rule?

[G. Ann Campbell]

Hi,

Your initial report wasn't terribly clear, which is likely why you didn't get any replies.

I'm guessing that an issue is raised on 

statement = connection.prepareCall(query);

From the rest of your code, it appears that (this.)query is set blindly in a setter. So the rule is picking up on the fact that you've accepted any random string (Little Bobby Tables, anyone?) to execute against your DB. And it's telling you that's not a good idea.

Ann

P.S. Please be aware that the standard courtesies (Hi, Thanks, ...) are appreciated in this group.

[Tony 79]

Meanwhile, I thank you for your attention...

My consideration is as follows:

Vulnerable Code:

Connection conn = [...];

Statement stmt = con.createStatement();

ResultSet rs = stmt.executeQuery("update COFFEES set SALES = "+nbSales+" where COF_NAME = '"+coffeeName+"'");

Solution:

Connection conn = [...];

conn.prepareStatement("update COFFEES set SALES = ? where COF_NAME = ?");

updateSales.setInt(1, nbSales);

updateSales.setString(2, coffeeName);

when I use the instruction:

statement.setString (1, text);

it should mean that I am in the "case" of the "Solution"

I'm wrong?

[andrew murren]

Hi Tony,

I maybe missing something but did you do any validation of coffeeName prior to passing it to update.Sales?  An inject could still be a valid string. Did you check to see if there is a cheat sheet on the OWASP site that helps with this? Like I said I might be something but that is my first thought. 

Andy

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/e4944a85-6d68-4fb8-a472-195cc5b3c3f6%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Tony 79]

Hello,

I am attaching another example

[andrew murren]

Hi Tony,

Your sample looks the same as the example suggested on the OWASP website.  

https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet#Defense_Option_1:_Prepared_Statements_.28with_Parameterized_Queries.29

The sample does say the input should be validated which your sample code does not do.  That may or may not be the reason the rule hits.  Take a look at the validator library from Apache to help with validation.

https://commons.apache.org/proper/commons-validator/ 

I will defer to others on the exact internal mechanism that causes this rule to hit.

Andy

On Wed, Dec 6, 2017 at 06:22 Tony 79 <tonys...@gmail.com> wrote:

Hello,

I am attaching another example

--

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/def0be10-2f18-4f3b-9a63-312d85599a80%40googlegroups.com.

[dinesh.bo...@sonarsource.com]

The rule "(S2077) SQL binding mechanisms should be used" detects cases when a non-constant SQL query string is passed to the database.

Use prepared SQL statements whenever possible instead.

Now, this rule enforces a best practice. If some issues are found in your project, it does not necessarily mean that you have real SQL Injection vulnerabilities.

If your developers have determined, after reviewing the issues, that this rule is noisy, feel free to disable it.

The default severity of this rule will be lowered from BLOCKER down to MAJOR in an upcoming version of the SonarJava plugin.

Thank you.

[Tony 79]

Thank you for the support, so to conclude down from BLOCKER to MAJOR

I just wanted to suggest for the rule in question that if you use the prepared SQL statements and then there is a "prepareStatement.set..." statement not to highlight the potential problem

[G. Ann Campbell]

Hi,

To follow up on this, as I suspected the issue is being raised because the value you're passing in to prepareCall is provided by a parameter, rather than being a hard-coded string. Since we don't yet do taint analysis (which looks at the origins of values passed to methods) we can't verify that some user didn't provide Little Bobby Tables as the parameter to ConnectionCallback. We'll get there (soon!) but until we do, we'll continue to raise issue here. For the record, since this rule is security-related, we currently accept a higher rate of false positives with it than we do for other rules. We figure that if it looks fishy to us (and this does!) we should flag it for a human to sort out.

Ann