Mark sources of Possible JDBC injection as safe

[apetrelli]

Description

With the various incarnations of "Possible JDBC Injection" bug pattern, it would be nice to mark the sources of this possible
bug as safe.
For example: I see warning about usage of Commons Lang 3 "StringUtils.repeat" with constant strings that is considered unsafe. For example:

builder.append(StringUtils.repeat("?", ", ", listOfParams.size());

is considered unsafe.
Or even a private method that constructs a dynamic sql query with constant pieces. For example:

if (input.getValue() != null) {
    builder.append(" AND value = ?");
    listOfParams.add(input.getValue());
}

The only way to get rid of the problem is to mark the method in which the SQL query is executed with SuppressFBWarnings.

It would be nice something, probably an annotation, that marks a method safe, to avoid suppressing too much and having real problems obscured by the suppression of SpotBugs warning.

Thanks a lot!

[h3xstream]

Good idea
We should define the behavior of StringUtils.repeat to allow other taint related api to benifit from it.

[jbindel]

Without having to add SuppressFBWarnings annotations, you may also add taint configurations with the findsecbugs.taint.customconfigfile System property. The file would contain taint configurations.

The standard taint configurations are missing entries for EnumMap, and I include them in a custom taint configuration file.

java/util/EnumMap.<init>(Ljava/lang/Class;)V:0#1,2
java/util/EnumMap.<init>(Ljava/util/Map;)V:0#1,2
java/util/EnumMap.<init>(Ljava/util/EnumMap;)V:0#1,2

@apetrelli , you may add similar taint configuration to avoid having to change your code with annotations.

org/apache/commons/lang3/StringUtils.repeat(Ljava/lang/String;Ljava/lang/String;I)Ljava/lang/String;:1,2

[apetrelli]

@jbindel thanks for the advice, it works!
However this configuration is a bit difficult to manage and its documentation is a bit lacking, especially in the Maven part. I had to use some imagination and looking at existing configurations to understand it, in my case to use the SAFE keyword in a SQL-generating code. This is indeed useful for compiled code.
An annotation that marks code as safe, or to mark it as a point of attention for SQL injections. These supposed annotation could be used by library providers to mark their code, instead of relying on you to create the correct configuration file.

If I only had the time to write some code for it...

[jbindel]

Yes, it may take digging through source code to find out how to configure the taint configuration. Lacking an annotation like you suggest, I have added "mark-as-safe" method calls to apply the SAFE taint on such things that the programmer knows are safe. An annotation could likely be made to do something similar.

package com.example.util;

public class TaintUtil {
    /** This does nothing at runtime, but the the taint is "safe" for SQL after calling this. */
    public static String markTrusted(final String s) {
        return s;
    }
}

The application code passed the strings through markTrusted(String) before making a SQL statement.
edit: Only call markTrusted() in the places you know the result is trusted.. Also get the fix noted in comments below to avoid false positives from String concatenation in JDK > 8.

builder.append(StringUtils.repeat("?", ", ", listOfParams.size());
...
// The query can be used to create a statement without
// warning because we have forced it to be SQL_INJECTION_SAFE.
String query = TaintUtil.markSafe(builder.toString());

Then in custom-taint-config.txt have a line like this (I use it for path traversal safety so that's included):

com.example.util.TaintUtil.markTrusted(Ljava/lang/String;)Ljava/lang/String;:0|+PATH_TRAVERSAL_SAFE,+SQL_INJECTION_SAFE

The Maven configuration for the plugin looks something like this:

<plugin>
  <groupId>com.github.spotbugs</groupId>
  <artifactId>spotbugs-maven-plugin</artifactId>
  <configuration>
    <systemPropertyVariables>
      <findsecbugs.taint.customconfigfile>${project.basedir}/src/findsecbugs/custom-taint-config.txt</findsecbugs.taint.customconfigfile>
    </systemPropertyVariables>
  </configuration>
</plugin>

[jbindel]

Also note that the taint propagation of string concatenation does not work for JDK > 8, and the StringUtils.repeat(String, String, int) method does use concatenation. If you use the markTrusted approach I've noted above, it will report many false positives without the fix I've submitted (see below). It should fix other false positives as well that arise from concatenating string constants.

StringUtils.java line 6267: https://github.com/apache/commons-lang/blob/acfad1e24a40306cfd1a79e6bc6b4421e49e4315/src/main/java/org/apache/commons/lang3/StringUtils.java#L6267C9-L6267C63

final String result = repeat(str + separator, repeat);

I have submitted a pull request that fixes that for my build using Java 11, and it fixes many "unfixable" taint problems that arise from the bug. #713

[apetrelli]

thanks @jbindel but the solution you provided seems a bit dangerous to me. I prefer to mark one by one the pieces that I think are ok, and thanks for the heads up about the related bug.

[jbindel]

It can be dangerous, and markTrusted should be called only in the same places one would add the annotation so I think it's only as dangerous as that would be if used that way. Like the existing @SuppressFBWarnings it requires code audits to make sure it is used only where appropriate.

[jbindel]

@apetrelli Thinking about your original request, would it make sense to have a type annotation like this?

builder.append((@FSBSafe(safe="SQL_INJECTION_SAFE") String) StringUtils.repeat("?", ", ", listOfParams.size());

That would be similar to the markTrusted() method, and it nicer because it doesn't add a do-nothing method, and it is an explicit annotation only where you want it.

edit: I don't know whether Java's type annotations can be used that way.

[apetrelli]

@jbindel an annotation in a cast operation? seems impossible to me.

[jbindel]

With JSR308 (not really new anymore but still new to me) TYPE_USE annotations are a thing and can be used in casts, but getting at them has to be done via bytecode analyzers. The Checker Framework makes use of them. You can see where the cast annotation is stored in the class bytecode with javap.

https://checkerframework.org/

[apetrelli]

@jbindel wow never heard of it, thanks! however I cannot understand what can be done with SpotBugs+findsecbugs. AFAICT SpotBugs does not support these annotations. Or yes?
Probably at this point I should use the tainting checker besides SpotBugs, maybe with the correct combination of @Tainted and @Untainted annotations.

[jbindel]

It looks interesting, and I don't have experience with it. I was unaware of the feature until stumbling upon it researching your bug report.