Skip to content

PS: Fix FPs on powershell/microsoft/public/sql-injection #249

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

Merged
merged 10 commits into from
Jun 20, 2025

Conversation

MathiasVP
Copy link
Collaborator

This PR fixes two FPs on the powershell/microsoft/public/sql-injection query:

  1. We were missing calls to Invoke-Sqlcmd with an InputFile named argument, and because the logic of the sink is "if we cannot find the right argument to the call to Invoke-Sqlcmd then just use the first argument" then we incorrectly marked the first argument of Invoke-Sqlcmd as the sink when it should have been the InputFile named argument. See the FP in the first commit for an example of this.
    This has been fixed in c18db91.

  2. The next FP fix requires some explanation. Consider this example (in some simplified language):

x.f = source();
sink(x);

normally there wouldn't be any path from source() to sink(x) in the above since we never read f off the x object before passing it to sink. 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.

@LWSimpkins LWSimpkins merged commit 6ab05cd into main Jun 20, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants