cpp: Add query to detect unsigned integer to signed integer conversio…

[JordyZomer]

Hi!

I added a query to detect unsigned integer to signed integer conversions used in pointer arithmetic. This is a variant analysis of the recent 'Sequoia' bug.

So what we do here is obtain a FunctionCall to a Function with any parameter that requires a signed integer. Following that, we look for any function calls that provide an unsigned number to this function despite the fact that it expects a signed integer. After that, we will use the DataFlow library to “taint track” any use of this argument in pointer arithmetic. Running this query on the Linux kernel database successfully identifies the Sequoia vulnerability as well as hundreds of additional instances that may be vulnerable.

Because there are so many results, I decided to refine the query slightly, so I added three filters to narrow down the criteria.

Establish whether there is a size check where the source is more than something
Determine whether the sink is smaller than something
Identify whether the source is a constant.
I configured it such that it only displayed results if none of these filters matched.

[rdmarsh2]

Thanks for this submission, it looks like a quite valuable query. I've suggested a few changes, and I'll be happy to merge this once those are made.

[JordyZomer]

Thanks @rdmarsh2 ! Made the changes :D

[geoffw0]

Hi,

Thanks for the contribution, it looks really promising. I've just run the query on 132 projects: https://lgtm.com/query/5086856576900256424/ .

The majority of results seem to be on calls to operator+ / operator- on iterators. My initial thought was to directly exclude this pattern with something like not f.getName().matches("operator%"), but I suspect some of these results may turn out to be true positives. Does anyone have any thoughts on this?

[sj]

@JordyZomer seems to have forgotten to mention that he wrote up a detailed blog post about this query and how it found critical results on the Linux kernel codebase. It's an amazing story 🚀! tl;dr: this is the result of variant analysis on CVE-2021-33909 a.k.a. "the sequoia bug".

[rdmarsh2]

The majority of results seem to be on calls to operator+ / operator- on iterators. My initial thought was to directly exclude this pattern with something like not f.getName().matches("operator%"), but I suspect some of these results may turn out to be true positives. Does anyone have any thoughts on this?

Those ought to be converting to ptrdiff_t, which will be a 64-bit type on most systems with 64-bit pointers. Given that this isn't looking for specifically attacker-controlled values, I'd be inclined to filter it to only casts to 32-bit signed types, or perhaps only types smaller than the pointer size. If it were based on interprocedural taint-tracking, we wouldn't need that exclusion, but it also wouldn't catch the original result. Filtering to only types with less than 8 bytes gets rid of most of those operator results while leaving a fairly large number of remaining ones: https://lgtm.com/query/141104918446121519/

@JordyZomer do you think that filter makes sense? I'm happy to merge otherwise, although I think you'll also need to run the autoformatter (alt-shift-f in the VS Code extension, or codeql query format -i in the CLI).

[JordyZomer]

Hey @rdmarsh2 ! Sorry for the delay, I was on a holiday and didn't have access to a PC :) The filter seems like a reasonable thing to do, I just added it, formatted the query and updated the PR :D

[rdmarsh2]

Looks good now, thanks!