C++: Modernize cpp/unclear-array-index-validation

[MathiasVP]

Pull Request checklist

This PR modernizes the old low precision cpp/unclear-array-index-validation by getting rid of the old DefaultTaintTracking inherited barriers and replacing them with a more precise BarrierGuard instantiation, and uses of SimpleRangeAnalysis.

I've started two DCA runs:

• A nightly one to test the impact of the changes to SimpleRangeAnalysis
• A run with just cpp/unclear-array-index-validation to test the changes to that query.

Since this query isn't included in any security suite I'm not sure how much you care about the result changes on cpp/unclear-array-index-validation?

I did a diff MRVA run to test the results, and they look reasonable.

Commit-by-commit review recommended.

The new results are TPs. They essentially look like:

c = fgetc(fin);
while(c != EOF) {
  if(... && (c = ... || c == EOF) {
    // ...
  }
}

and we now see that c == EOF is always false.

All query authors

• A change note is added if necessary. See the documentation in this repository.
• All new queries have appropriate .qhelp. See the documentation in this repository.
• QL tests are added if necessary. See Testing custom queries in the GitHub documentation.
• New and changed queries have correct query metadata. See the documentation in this repository.

Internal query authors only

• Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
• Changes are validated at scale (internal access required).
• Adding a new query? Consider also adding the query to autofix.

[jketema]

First round of comments. I still need to have a look at the DCA results, and have another look at the new barriers.

[jketema]

Doesn't dropping this lead to FNs?

[MathiasVP]

Oh, true. I should add this back in. I don't think this really removed anything in practice, but we may as well not regress here.

[MathiasVP]

Fixed in 61a012f

[jketema]

You're passing in a character pointer, not a file pointer?

[MathiasVP]

Yeah, I know. I tried to do a proper test with fopen, but it turns out I couldn't get taint through that. So I was planning on opening another PR that adds flow through fopen.

(I'm actually quite surprised we don't have taint through fopen, but I can't find any taint model for it so I guess it makes sense)

[jketema]

Oh, I see we have an fopen model but that doesn't extend TaintFunction.

[MathiasVP]

Yeah. Let me quickly whip up a PR to try that out

[MathiasVP]

PR for this: #17715

[jketema]

This is good, because getc either returns a value which fits in an unsigned char or -1? Correct?

[MathiasVP]

Correct. See the Return value section at https://en.cppreference.com/w/c/io/fgetc

[jketema]

Some of the alert changes on the nightly suite seem to suggest that this might be violated on some platforms?

[jketema]

Oh, sorry I should have read your top comment more closely. That clarifies things.

[MathiasVP]

Are you referring to the Kamailio alerts? I think they're TPs. They come from the EOF check here: https://github.com/kamailio/kamailio/blob/b02c247023ea3ea0ef9753efdb04aff7d5d3bbb4/src/modules/db_text/dbt_file.c#L143, I think

[MathiasVP]

So the bound for fgetc is initially [-1, 255], and then the != check refines it to [0, 255]. So the == EOF check never holds.

[jketema]

Yup, I see that now.

[jketema]

This LGTM. I'm not exactly sure how to judge the DCA result changes for the query. The query was low precision, and it still is, but we get somewhat different results now. We might just be better prepared now if we want to improve the precision of the query in the future.