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
Fix OpcodeStack to handle propagation of taints properly in case of string concatenation in Java 11 and above #2195
Conversation
…tring concatenation in Java 9 and above Instead of using StringBuffer or StringBuilder internally, Java 11 and above uses a dynamic call to makeConcatWithConstants() to append strings. Previously, `OpcodeStackDetector` did not handle the taint propagation properly in case of this dyanamic call which led to false negative such as the one described in issue [spotbugs#2184](spotbugs#2184). This PR fixes such issues by adding code to `OpcodeStackDetector` to handle this case as well.
No false positives and no crashes on some open-source projects we used to test our PRs. |
A side remark: I put the tests into the |
servletRequestParameterTainted = true; | ||
} | ||
Object sVal = i.getConstant(); | ||
if (sVal != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"if not...else" is a double negative, could refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that new test fails on the commit 8c5e193. Thanks for your contribution!
Instead of using StringBuffer or StringBuilder internally, Java 11 and above uses a dynamic call to makeConcatWithConstants() to append strings. Previously,
OpcodeStackDetector
did not handle the taint propagation properly in case of this dyanamic call which led to false negative such as the one described in issue #2184. This PR fixes such issues by adding code toOpcodeStackDetector
to handle this case as well.Make sure these boxes are checked before submitting your PR -- thank you!
CHANGELOG.md
if you have changed SpotBugs code