SQL injection hotspot: would like exception list parameter

Rule java:S2095 (“Resources should be closed”) is parameterizable and allows one to add an exception list: a set of classes that won’t trigger the rule.

I’d like something similar for rule java:S2077 or whatever its equivalent is. (I saw old messages saying it was deprecated so don’t know if it’s been replaced by something else, but S2077 is the only thing that comes up when I type SQL into the rule search box while the language filter has been set to Java.)

We have a bunch of code where there’s a line like:

String stmt = “SELECT " + Foo.bar() + " FROM ?”;

and then stmt is used in a preparedStatement. SonarQube can’t tell that foo.bar() is safe so flags the line as a potential injection path.

(At least I assume that’s what’s going on – correct me if I’m wrong. I’ve determined experimentally that the hotspot isn’t triggered if the parameter to preparedStatement() is a String constant – even if it is made by adding a few literals or static String constants together. Does SQ go beyond that in trying to determine whether a given prepared statement is safe?)

So if we know that class Foo is immune to injection pathways (because all its strings are internal) then it would be great to add Foo to an exception so it won’t mark this statement a hotspot.

Of course, this exceptioning should work on an “and” basis, so even if we add Foo to the exception list, we’d still want something like

String stmt = "SELECT " + Foo.bar() + " FROM " + couldBeUserInput;

to be flagged.

Now, like I said above, SQ is clearly doing some flow analysis, because, while it is flagging a call to preparedStatement(), it is tracking the String parameter to that constructor back to the statement that creates that String.

I called api/hotspots/show to see what I could find about a particular hotspot, and it only gives the line number of the call to preparedStatement(). If it would also give the line number of the statement that creates the String, I could do the analysis in a script, in lieu of the feature I’m requesting here.

Hello @MisterPi

There is another rule (S3649), available starting SQ developer edition (or free for open-source projects on SonarCloud), which is able to detect precisely SQL injections.

S2077 on the other hand will be triggered when the SQL query is dynamically formatted/concatenated, the goal is to highlight the code when probably modern database libraries like prepare statements and/or ORM are not used or used incorrectly, and therefore can cause performance, maintainability issues and increase the risk of SQL injections as well.

Eric

So did rule S2077 previously (e.g., in 7.0) make some attempt to determine whether an inject path exists? Because when we upgraded from 7.0 to 8.6, our count for S2077 in one file went up from 3 to 19. Prepared statements are used, but the issue seems to be that the string that forms the prepared statement in each case has some concatenation involving substrings generated by a method call to another class. That other class is entirely internal and never gets any user input. But this is true for all 19 instances – I don’t see any substantial difference between them.

So I’d like to know whether S2077 in 7.0 tried to determine if an injection path was possible, and successfully determined in 16 of the 19 cases that no path was possible, but this capability has been removed from later versions and now only happens in rule S3649.

I confirmed experimentally that if I replace those class methods with string literals (so I’m just concatenating literals), the issue isn’t flagged by 8.6, so 8.6 isn’t just flagging ANY situation where concatenation occurs.

S2077 was never designed to detect SQL injections directly, as you noticed it only detects when the SQL query is formatted / concatenated (no matter from what kind of values, user-input or not).

We agree, the rule is a bit limited because even with prepare statements sometimes we don’t have other choices than to concatenate some parts of the query like columns to select or the table name (prepare statements can’t help for that) but on the other hand the rule can maybe help to have better queries, more maintainable, less dynamical and thus with less risk of SQL injections.

Eric

“… it only detects when the SQL query is formatted / concatenated (no matter from what kind of values, user-input or not).”

Actually, it will detect simple cases. If the parts being concatenated are literals or constants, SQ doesn’t complain. I have verified this experimentally.

I’m still trying to figure out why the number of cases went way up moving from 7.0 to 8.6. For instance, in one file, there were only 3 instances reported by 7.0, but the same file has 19 issues (hotspots) with 8.6. I don’t see any difference between the 3 that were detected before and the 16 that weren’t. In both cases, the string is formed by concatenating several constants and one or two calls to a method in a different class, that returns a string from a list. That’s why I thought the old implementation was attempting to trace those calls to see if there was a user injection path.

In our case, we could filter out close to all of these by adding those method calls to an “exception list” like the one that java:S2095 has.