-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark sources of Possible JDBC injection as safe #709
Comments
Good idea |
Without having to add The standard taint configurations are missing entries for
@apetrelli , you may add similar taint configuration to avoid having to change your code with annotations.
|
@jbindel thanks for the advice, it works! If I only had the time to write some code for it... |
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.
The application code passed the strings through
Then in
The Maven configuration for the plugin looks something like this:
|
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 StringUtils.java line 6267: https://github.com/apache/commons-lang/blob/acfad1e24a40306cfd1a79e6bc6b4421e49e4315/src/main/java/org/apache/commons/lang3/StringUtils.java#L6267C9-L6267C63
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 |
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. |
It can be dangerous, and |
@apetrelli Thinking about your original request, would it make sense to have a type annotation like this?
That would be similar to the edit: I don't know whether Java's type annotations can be used that way. |
@jbindel an annotation in a cast operation? seems impossible to me. |
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 |
@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? |
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. |
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:
is considered unsafe.
Or even a private method that constructs a dynamic sql query with constant pieces. For example:
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!
The text was updated successfully, but these errors were encountered: