Skip to content
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

CPP: Convert SQL tainted away from away from DefaultTaintTracking. #13985

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

alexet
Copy link
Contributor

@alexet alexet commented Aug 16, 2023

This converts away from DefaultTaintTracking towards the newer API.

This does end up with some changes I noticed:

  • argv is repeated multiple times. This is at often 3 times (argv, *argv, **argv) + sometimes more (sometimes there seems multiple versions of some indirections?).
  • The alert location moves from the argv use to the argv argument.

I'm not sure about node.asConvertedExpr() vs asExpr for the sink. The issue with using asExpr is that multiple levels of conversions are reachable without going through each over due to the references. So having fewer possible sinks seems better.

I tried setting ArgvSource to use this.asParameter(2) instead of this.asParameter(_) and it seems to fix these issues, but I am not sure of the consequences.

@github-actions github-actions bot added the C++ label Aug 16, 2023
@alexet alexet force-pushed the ir-tainted-sql branch 2 times, most recently from 53812fc to 5c24159 Compare August 21, 2023 11:29
@alexet alexet added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 21, 2023
@alexet alexet force-pushed the ir-tainted-sql branch 2 times, most recently from b784e02 to c76b8cf Compare August 22, 2023 11:17
@alexet alexet marked this pull request as ready for review August 22, 2023 11:17
@alexet alexet requested a review from a team as a code owner August 22, 2023 11:17
@MathiasVP
Copy link
Contributor

  • argv is repeated multiple times. This is at often 3 times (argv, *argv, **argv) + sometimes more (sometimes there seems multiple versions of some indirections?).

The "multiple versions" issue is indeed annoying. It's a problem that should be fixed inside dataflow, and I'm currently in the process of doing this. It definitely shouldn't block this PR.

I'm not sure about node.asConvertedExpr() vs asExpr for the sink. The issue with using asExpr is that multiple levels of conversions are reachable without going through each over due to the references. So having fewer possible sinks seems better.

Indeed, this is related to the issue above. Ideally, we should be using asExpr in most places (since we often don't want to deal with conversions), but that's currently necessary to avoid these duplicated results.

I tried setting ArgvSource to use this.asParameter(2) instead of this.asParameter(_) and it seems to fix these issues, but I am not sure of the consequences.

Yeah, I think we should try to do this change in a separate PR 👍.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than a small comment.

cpp/ql/src/Security/CWE/CWE-089/SqlTainted.ql Outdated Show resolved Hide resolved
@alexet alexet added the no-change-note-required This PR does not need a change note label Aug 22, 2023
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MathiasVP MathiasVP merged commit 6cf9968 into github:main Aug 23, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants