C++: Use flow states in cpp/command-line-injection

[jketema]

Rework cpp/command-line-injection to use flow states. Only thing I'm not sure about is whether the location of a dataflow node uniquely determines the node, which I depend upon:

class ExecState extends DataFlow::FlowState {
  DataFlow::Node fst;
  DataFlow::Node snd;

  ExecState() {
    this = "ExecState (" + fst.getLocation() + ", " + snd.getLocation() + ")" and
    interestingConcatenation(fst, snd)
  }
...

[rdmarsh2]

whether the location of a dataflow node uniquely determines the node, which I depend upon

It doesn't - there's a lot of cases with pointer arguments where we have input and output nodes for the pointed-to values that share a location with the argument.

[jketema]

It doesn't - there's a lot of cases with pointer arguments where we have input and output nodes for the pointed-to values that share a location with the argument.

Good to know. Then the question is, whether it's unique enough for what we're doing here, or whether there's something that better identifies a data flow node.

[hvitved]

It doesn't - there's a lot of cases with pointer arguments where we have input and output nodes for the pointed-to values that share a location with the argument.

Good to know. Then the question is, whether it's unique enough for what we're doing here, or whether there's something that better identifies a data flow node.

Note that in this case it will only be a problem if you have two snd nodes with the same location that are also in interestingConcatenation(_, snd), which is probably unlikely in practice? If not, appending the toString() representation to the location may be enough to disambiguate.

[hvitved]

Since you only care about the snd node, there should be no need to record the fst node in the flow state as well.

[jketema]

Indeed. I'm probably unnecessarily conservative there.

[jketema]

I retract my statement. Turns out there is a case where fst does matter. See the test19 I added. When I omit fst, 3 paths instead of 2 paths are reported involving l. 220. The same happens when I move interestingConcatenation from ExecState to isAdditionalTaintStep.

[jketema]

If not, appending the toString() representation to the location may be enough to disambiguate.

Indeed, also because that + the location is what the user eventually will see.

[MathiasVP]

whether the location of a dataflow node uniquely determines the node, which I depend upon

It doesn't - there's a lot of cases with pointer arguments where we have input and output nodes for the pointed-to values that share a location with the argument.

And more generally, PostUpdateNodes piggyback on their pre update nodes for locations as well.

If the idOf trick worked on IPA types we could use that here, but sadly that feature is still unavailable.

If not, appending the toString() representation to the location may be enough to disambiguate.

Indeed, also because that + the location is what the user eventually will see.

Yeah, that's probably good enough. A couple of notes on this:

• Many nodes will definitely have the same toString because we try quite hard to reduce the number of unique strings for performance reasons, but when combined with locations it's probably okay.
• We don't like having query logic depend on the implementation of the toString. We even have a ql-for-ql query for this. So it's a bit of a shame that we have to do this.

[jketema]

DCA Wireshark observations without any of the discussed changes around toString:

• cpp/cleartext-storage-buffer and cpp/command-line-injection get faster. The two DCA experiments show about the same numbers here, although the data from the second experiment if very messy.
• cpp/tainted-format-string get slower (goes from 12 to 20 seconds)

Also:

• The false positives in vim that are due to the execution stacks not matching up on the old version of the query disappear

[jketema]

Many nodes will definitely have the same toString because we try quite hard to reduce the number of unique strings for performance reasons, but when combined with locations it's probably okay.

Updated accordingly.

We don't like having query logic depend on the implementation of the toString. We even have a ql-for-ql query for this. So it's a bit of a shame that we have to do this.

Is this run on PRs? I didn't explicitly write toString() and don't see a warning.

[MathiasVP]

Is this run on PRs? I didn't explicitly write toString() and don't see a warning.

It's only at @precision medium - so it's not part of the Code Scanning suite.

[MathiasVP]

(I accidentally committed my own suggestion. Sorry about that!)

In any case, this LGTM!

[MathiasVP]

The Code Scanning errors are unrelated. So I'll just go ahead and merge this.