PS: Fix FPs on powershell/microsoft/public/sql-injection
#249
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.
This PR fixes two FPs on the
powershell/microsoft/public/sql-injection
query:We were missing calls to
Invoke-Sqlcmd
with anInputFile
named argument, and because the logic of the sink is "if we cannot find the right argument to the call toInvoke-Sqlcmd
then just use the first argument" then we incorrectly marked the first argument ofInvoke-Sqlcmd
as the sink when it should have been theInputFile
named argument. See the FP in the first commit for an example of this.This has been fixed in c18db91.
The next FP fix requires some explanation. Consider this example (in some simplified language):
normally there wouldn't be any path from
source()
tosink(x)
in the above since we never readf
off thex
object before passing it tosink
. However, sometimes we want to have a result in those scenarios. So in taint-tracking we specify certain kinds of "implicit reads" that happen at sinks.For PowerShell I decided at some point that any kind of field could be read off implicitly at sinks. However, this gives a FP when a specific field of an object is tainted, and we only want an alert when that specific field is tainted. See 25d94fa for an example FP from this.
In 1486200 I've fixed this FP by only allowing implicit reads by default on "array index"-like values. This matches what other CodeQL languages.
Now, this means that we have to add such implicit reads at sinks specifically where we need them. So in 1ff04d9 I added a false negative which requires these implicit reads, and in 1486200 I've added support for such implicit reads.