Skip to content

Conversation

@alexet
Copy link

@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.

@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!

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