C++: prototype for off-by-one in array-typed field

[rdmarsh2]

No description provided.

[MathiasVP]

Looks good so far! It's great to see that you managed to build the correct Linux and find the CVE we expected 🎉.

[MathiasVP]

The metadata suggests that this is a path-problem but you've not imported DataFlow::PathGraph, and the columns selected also doesn't match the format expected for path-problems.

[MathiasVP]

The point of this conjunct is to ensure that the computed address is actually dereferenced, right? If so, it seems this could use the same interprocedural dataflow config we created in https://github.com/github/codeql/pull/10438/files#diff-f245ae0a243ae41fb7d507426991139388edd94c3dee54575550955762664b3cR200, right?

On the other hand, the set of sources in this query might be so large that it's not feasible to do global dataflow.

So if we do want to keep it local, I suggest we speed it up by a forward and backward pruning step before computing the transitive closure.

[MathiasVP]

Looks like there's good progress here! Do you think it's time to take it out of draft soon?

[MathiasVP]

This will remove the Code Scanning alert (but you obviously need to accept all the test changes as well in that case).

Suggested change

	"This pointer arithmetic may have an off-by-" + (delta + 1) + " error allowing it to overrun $@ at this $@",
	"This pointer arithmetic may have an off-by-" + (delta + 1) + " error allowing it to overrun $@ at this $@.",

[MathiasVP]

I was confused for a second about why you didn't use b for anything (since I just assumed it would be any SemBound), but now I see that b is a SemZeroBound. I think it would have been clear for me from the start if this line had instead been:

Suggested change

	semBounded(getSemanticExpr(pai.getRight()), b, bound, true, _) and
	semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound zero), bound, true, _) and

(and then obviously not included b in the list of existentially-quantified variables).

[MathiasVP]

LGTM!

[github-actions]

QHelp previews:

cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.qhelp

Constant array overflow

The program performs an out-of-bounds read or write operation. In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.

Recommendation

Ensure that pointer dereferences are properly guarded to ensure that they cannot be used to read or write past the end of the allocation.

Example

The first example uses a for loop which is improperly bounded by a non-strict less-than operation and will write one position past the end of the array. The second example bounds the for loop properly with a strict less-than operation.

#define MAX_SIZE 1024

struct FixedArray {
  int buf[MAX_SIZE];
};

int main(){
  FixedArray arr;

  for(int i = 0; i <= MAX_SIZE; i++) {
    arr.buf[i] = 0; // BAD
  }

  for(int i = 0; i < MAX_SIZE; i++) {
    arr.buf[i] = 0; // GOOD
  }
}

References

• CERT C Coding Standard: ARR30-C. Do not form or use out-of-bounds pointers or array subscripts.
• OWASP: Buffer Overflow.

[MathiasVP]

LGTM otherwise

[MathiasVP]

Also just a minor question about the title of the PR: There's nothing "prototypy" about this query anymore, right? That is, this is a fully-fledged query that we expect to promote to Code Scanning this coming quarter, correct?