C++: Improve and promote cpp/overflow-buffer

[geoffw0]

Improve and promote "Call to memory access function may overflow buffer" (cpp/overflow-buffer).

• fix an issue where references to array expressions inside offsetof expressions were being misinterpreted as accesses.
• promote the query onto security-extended (by increasing the severity to warning and setting the precision to medium).
• added some test cases along the way, inspired by real world results / FPs found using MRVA.
  • I actually found three classes of false positives in the real world results, but only decided to fix one of them. For the other two, I believe it will be more challenging to determine which are TP vs FP cases. If we want to promote the query to high precision (code scanning suite) at some point, we will probably need to do something with these.

PR Overview

This PR improves and promotes the "Call to memory access function may overflow buffer" query by fixing its handling of array expressions within offsetof and adjusting its severity and precision for the security-extended suite.

• Updated change note in cpp/ql/src documenting the security-extended promotion of cpp/overflow-buffer.
• Added a change note in cpp/ql/lib for fixing the getBufferSize predicate issue.

Reviewed Changes

File	Description
cpp/ql/src/change-notes/2025-02-20-overflow-buffer.md	Added change note to promote cpp/overflow-buffer to security-extended.
cpp/ql/lib/change-notes/2025-02-20-getbuffersize.md	Documented the fix for getBufferSize misinterpreting array expressions.

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

[jketema]

Did you look at the results flagged up by DCA and their quality?

[geoffw0]

DCA

• failed due to WebKit-JavaScriptCore; it shouldn't have been run, but probably my DCA was still the build from https://github.com/github/codeql-dca/pull/2418 so it did run. Irrelevant to this PR.
• 1164 results on dsp-testing__samate-c-cpp
  • sample were all in the bad test cases, i.e. these results look great!
• a light scatter of results on other projects
  • most of which are of the "worth checking what's going on here" rather than "definitely wrong" type; accessing offset pointers using negative indices (update: looking again, at least some of these are TP) and clearing consecutive elements of a struct (update: on closer inspection the latter results look to be doing something unsafe after all) being common patterns. This is probably OK as we're only targetting medium precision and there isn't a huge amount of noise - but I'm going to add a couple more test cases and see if I can narrow down results a little more.

[geoffw0]

I think this is ready to merge, but I'd really appreciate a second opinion on at least some of the real world results.

[jketema]

This is what I'm seeing:

• neovim: likely a FP, but hard to tell because there's quite a lot of pointer manipulation going on
• PHP: These look like FPs. One of the patterns here is memset(p->free_map, 0, sizeof(p->free_map) + sizeof(p->map)) Are we handling sizeof correctly (see also below)?
• OTP: The alert in erl_bif_info.c looks like a TP. The hashing ones are difficult to follow so not sure.
• TinyUSB: These all look like FPs of the form memset(arr, 0, sizeof(arr)). Are we handling sizeof correctly?

[geoffw0]

Thanks for taking a look.

PHP: These look like FPs. One of the patterns here is memset(p->free_map, 0, sizeof(p->free_map) + sizeof(p->map)) Are we handling sizeof correctly (see also below)?

I've just pushed a change that fixes this FP result in PHP (amongst others), and the cost of losing some TPs as well. I think this is a good tradeoff given the query is a bit noisy, and one we could improve in future by refining the existing getRootType predicate in Buffer.qll to cope with a wider range of things.

OTP: The alert in erl_bif_info.c looks like a TP. The hashing ones are difficult to follow so not sure.

The hashing ones are also eliminated by the commit I just pushed (the clear TP result remains).

[geoffw0]

TinyUSB: These all look like FPs of the form memset(arr, 0, sizeof(arr)). Are we handling sizeof correctly?

This was caused by a variable with multiple types in the database. Probably shouldn't happen, but I've hardened the query to this.

I should do another DCA run now with all the changes... [started]

[jketema]

With this we seem to be losing good results for cpp/static-buffer-overflow. This change is also somewhat unexpected to me, but maybe I do not completely understand what is going on here. What I thought I was seeing in the DCA data was cases where the value of sizeof(...) could be statically determined and, hence, would be in the AST, and we should be using that value but somehow weren't. But your comment on TinyUSB "This was caused by a variable with multiple types in the database." suggests that I might be misunderstanding what was going on?

[geoffw0]

I've fixed two issues since the first DCA run. One was that occasionally a variable has multiple types associated with it in the database, and those types have different sizes (and presumably there are different results on the sizeof side of the equation as well but I didn't check). This led to a multitude of false positive results on the same lines (e.g. if the sizes and sizeofs in the database for a single variable were 4, 8 and 12 you'd see it comparing every combination and getting query results where it compares 8 > 4, 12 > 4 and 12 > 8). I fixed this with the unique you see here - though in retrospect putting it on the sizeof side of the equation would probably have worked equally well. I don't think it matters and I can do both if you'd prefer.

The other issue I fixed was to do with writes that are intended overwrite (typically zero) multiple fields of a struct. For example the struct might be

struct foo {
  int a;
  int b;
  int c;
}

and we might write memset(&(foo.b), 0, sizeof(b) + sizeof(c)). There's some existing special logic for this in Buffer.qll for one of the cases, which looks for the outermost struct the member variable is in and calculates how much room there is in following fields. The change I've made is using the same logic for the size of array members, for example in:

struct foo {
  int a;
  int b[10];
  int c;
}

we would now allow memset(&(foo.b), 0, sizeof(int) * 11). This gets rid of a lot of very dubious query results, but also a few good ones because the "special logic" I referred to above isn't particularly robust and sometimes doesn't produce an answer, so we don't produce a query result.

we seem to be losing good results

That's an expected price to making a query less noisy (and thus promotable) while only investing a few days development time - better to have the query enabled with less results than having more results that nobody sees. But if you can see a quick fix that is more precise I'll be happy to try it.

Are there any specific good results you're uncomfortable with losing?

[jketema]

Thanks a lot for the explanation!

Are there any specific good results you're uncomfortable with losing?

I thought so, but looking at the 2nd DCA run again and diving into the lost SAMATE results, I don't think that anymore. It seems that all the results we lose are ones where we overwrite more than one member of a struct, which SAMATE apparently doesn't like, but for which there are valid use-cases as we observed.

So, I'm happy with this. It does seem we have some more test results that need to be updated (CPP language tests are currently failing in CI).

[jketema]

Do we need a change note for the lost cpp/static-buffer-overflow results by the way?

[geoffw0]

Added [another] change note.

[geoffw0]

One of the tests is from the internal repo so I'm moving it to here before accepting the differences. I think I'll also need to merge main (after the old copy of the test is deleted) before all tests actually pass here...

[jketema]

This seems new. Do we need another DCA experiment for this?

[geoffw0]

Might be best. Some of the results we lost were due to the formula including padding at the end of a struct / class, which it now doesn't, so we may get a few results back.

[jketema]

It looks like we now get some incorrect new results when unions are involved. For example, for cpp/static-buffer-overflow in systemd:

#define BTRFS_SUBVOL_NAME_MAX 		4039

struct btrfs_ioctl_vol_args_v2 {
	__s64 fd;
	__u64 transid;
	__u64 flags;
	union {
		struct {
			__u64 size;
			struct btrfs_qgroup_inherit *qgroup_inherit;
		};
		__u64 unused[4];
	};
	union {
		char name[BTRFS_SUBVOL_NAME_MAX + 1];
		__u64 devid;
		__u64 subvolid;
	};
};

...
        struct btrfs_ioctl_vol_args_v2 vol_args = {
                .flags = flags & BTRFS_SNAPSHOT_READ_ONLY ? BTRFS_SUBVOL_RDONLY : 0,
                .fd = old_fd,
        };
...
        strncpy(vol_args.name, subvolume, sizeof(vol_args.name)-1);

we get "Potential buffer-overflow: 'name' has size -32 not 4039.".

[jketema]

The new results on Kamalio similarly involve unions embedded in structs.

[geoffw0]

I'll take a look. If this goes any deeper I may need to cut back on the ambition of this PR.

[jketema]

If this goes any deeper I may need to cut back on the ambition of this PR.

Makes sense. It's fine with me to drop this specific change if that's the fastest way to get this merged.

[geoffw0]

Fixed by replacing getAField() with getDeclaringType*().

[jketema]

Thanks for the fixes. I'm happy with this once DCA comes showing that we no longer unexpectedly lose or gain results.

[geoffw0]

Hmm, I'm still seeing some cases with negative sizes in the latest DCA run. :(

[jketema]

Hmm, I'm still seeing some cases with negative sizes in the latest DCA run. :(

Shall we just back out the changes from 1354beb (and the fix you did on top of that)?

[geoffw0]

My Friday brain appears to have come up with an "obvious" better solution, so we'll see how we do with that.

The problem with backing out the changes is having spend time looking into the results I'm not really sure any more that the query should be promoted without both improvements. So that leaves us with the offsetof bug fix and not a lot else. We shall see...