Java: Use controlledString predicate in ExecUnescaped.ql to reduce false positives#5
Closed
mrigankpawagi wants to merge 1 commit into
Closed
Java: Use controlledString predicate in ExecUnescaped.ql to reduce false positives#5mrigankpawagi wants to merge 1 commit into
mrigankpawagi wants to merge 1 commit into
Conversation
…lse positives Replace the weak local saneString predicate with the comprehensive controlledString predicate from ControlledString.qll. This reduces false positives for expressions involving numeric types, enum constants, class names, and other programmer-controlled values.
github-actions
Bot
added
documentation
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves the
java/concatenated-command-linequery (ExecUnescaped.ql) by replacing the ad-hocsaneStringpredicate with the sharedcontrolledStringpredicate fromsemmle.code.java.security.ControlledString.Problem
The query's local
saneStringpredicate was overly simplistic — it only recognizedStringLiteral,NullLiteral, and variables assigned from those. This caused false positives for many expressions that are clearly programmer-controlled, such as:Integer.toString(port)).class.getName())MRVA on top-100 Java repositories confirmed these patterns appearing frequently as false positives.
Changes
Replaced the ad-hoc
saneStringpredicate body with a call to the sharedcontrolledStringpredicate, which already handles all the above cases and is maintained centrally for the entire Java CodeQL library.Why this is correct
The
controlledStringpredicate inControlledString.qllis specifically designed to recognize expressions whose values are determined at compile time or are otherwise programmer-controlled. It subsumes everything the originalsaneStringdid and adds many more patterns. This is the same predicate already used bySqlConcatenated.qlfor the same purpose.