C++: Only consider the maximum buffer size for badly bounded write

[jketema]

No description provided.

[MathiasVP]

Does this need a change note?

[jketema]

Does this need a change note?

Yes, I think so.

[jketema]

Change note added.

[geoffw0]

Are there any good projects for testing this on? (I tried MRVA top 100 but didn't find much)

[jketema]

Let me run some MRVA top 1000.

[geoffw0]

Let me run some MRVA top 1000.

I'm already on it (with a variant of the query that should reveal differences)...

[geoffw0]

I ran this variant, intended to spot cases where we would have a result with either version of the query and examine potentially multiple buffer sizes:

import semmle.code.cpp.security.BufferWrite

int candidateResult(BufferWrite bw) {
  bw.hasExplicitLimit() and // has an explicit size limit
  result = getBufferSize(bw.getDest(), _)
}

from BufferWrite bw
where
  bw.getExplicitLimit() > candidateResult(bw)
select bw, strictconcat(candidateResult(bw).toString(), ", "), bw.getExplicitLimit().toString()

I ran this on the MRVA top 1000. It hasn't finished every single project yet, but I think it's about time I commented.

• 1 result in gozfree/gear-lib, buffer size 8192, explicit limit 16384. It looks like it's deliberately overrunning the buffer into another variable on the stack!
• 1 result for OpenXRay/xray-16, buffer size 33, explicit limit 65. Click through to code didn't work.
• 1 result for libretro/RetroArch, buffer size 32, explicit limit 45. This appears to be a fairly simple case of hardcoding the limit not matching the actual buffer size. The code is 'drm' related though, so it could be doing something very weird or deliberately surprising.

No results with multiple candidate results (i.e. multiple calculated buffer sizes), which is disappointing, but also reassuring. The change in this PR is likely to fix the intended case but won't affect most other results. 👍