C++: Replace field -> object taint rule with ArrayContent dataflow

[MathiasVP]

This PR uses field flow to implement precise modeling of array writes and reads for IR dataflow.

With these changes to the C++ dataflow library, we can now remove one of the taint rules that causes field -> object taint flow (which we want to avoid before we embark on https://github.com/github/codeql-c-analysis-team/issues/103 to avoid field-to-unrelated-field flow).

[MathiasVP]

CPP-differences for 78b24b7: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1428/

The failing tests are from the internal repo, which I'll create a PR for when we're happy with this PR.

[jbj]

Overall I'm very excited about this PR. We'll have to figure out how it fits into the collection taint modelling we're doing these days.

[jbj]

These rules seem overly syntactic, but we can always tweak them later if there's a need. The important thing is they're guaranteed not to overlap with getWrittenField.

[MathiasVP]

I agree. The PointerAdd case I think we can live with, but requiring the presence of a CopyValue instruction as a direct successor to the Load instruction is quite fragile. For instance, for this store:

int source();
void f(char* s) {
  *(int*)s = source();
}

there's a Convert instruction in between the Load and the CopyValue, which means that this is not recognized as a store step by arrayStoreStepChi.

[jbj]

This definition seems overly broad. Can we restrict the address operand to particular syntactic forms like in arrayStoreStepChi and ensure there is no overlap with fieldReadStep?

[MathiasVP]

This was also my main worry when I was CPP-difference, but luckily didn't appear to be a problem. For explicit array and pointer reads (i.e., x[i]) we could restrict it by requiring that the source address instruction is a PointerAdd instruction. But for an example like:

void f(int* s) {
  *s = source();
}

void test() {
  int x;
  f(&x);
  sink(x);
}

(where the read step is currently the load of x in sink(x)) I don't see how to syntactically restrict the predicate.

[jbj]

But can you add something like and not fieldReadStep(node1, _, _) to ensure there is no overlap?

[MathiasVP]

Hm, I was hoping your solution would Just Work, but the following shows that we need to do something else, I think:

void f(int* px) {
  *px = source();
}

void test_outer_with_ptr() {
  Point p;
  f(&p.x);
  sink(p.x);
}

Here, the load of p.x in sink(p.x) actually looks like a fieldReadStep, but (as *px = source() matches arrayStoreStepChi), we actually need an arrayReadStep in this case.

[jbj]

We discussed this and decided it doesn't need to block this PR.

[MathiasVP]

As reported in #4298 this is a source of field conflation (which I would've discovered if I had tried to replace p.x with p.y in the example above 😢). This is fixed by #4302.

[jbj]

I think you can remove the same rule from IR TaintTrackingUtil.qll.

[MathiasVP]

Good point. I've removed it in 0b97a4a.

[MathiasVP]

I didn't notice until now that there's actually a result change in our tests for UncontrolledFormatStringl.ql in argvLocal.c, where we no longer report these two results after 78b24b7 (which removed array element -> array taint):

// BAD: i5 value comes from argv
char i5[5012];
i5[0] = argv[1][0];
printf(i5);
printWrapper(i5);
	
// BAD: i5 value comes from argv
printf(i5 + 1);
printWrapper(i5 + 1);

Which are legitimate uses of field -> object flow.

To keep this PR uncontroversial, I now think that we should:

• Undo that particular commit, and only remove the t instanceof UnknownType case from DefaultTaintTracking
• Undo 0b97a4a which removed the same rule from TaintTrackingUtil that was removed in )
  The good thing is that now those two rules will be completely identical. Once we have the field-to-object-flow-at-sink flow as mentioned in https://github.com/github/codeql-c-analysis-team/issues/103 we can remove the rule completely.

How does this sound?

[jbj]

Which are legitimate uses of field -> object flow.

How so? I don't see any fields there, so I'd classify it as element -> array.

How does this sound?

SGTM.

[MathiasVP]

How so? I don't see any fields there, so I'd classify it as element -> array.

Right. I meant element -> array. Good catch!

[MathiasVP]

This PR should be good to go now! The remaining test failures are accepted in the internal repo (which is green).