github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.53k stars 1.5k forks source link

False positive #13464

Open ImpossibleShape opened 1 year ago

ImpossibleShape commented 1 year ago

Description of the false positive

The error

This 'call to strcpy_s' operation is limited to 512 bytes but the destination is only 256 bytes.

Somehow it is getting a different size for sizeof and the actual size of the array. Not sure why it thinks they are different sizes when it's the same physical array. Code is below.

Code samples or links to source code

// Get the full device path char devPath[ 256 ]; strcpy_s( devPath, sizeof( devPath ), pDetData->DevicePath );

jketema commented 1 year ago

Hi @ImpossibleShape,

Thanks for your report. I'm missing some details unfortunately, what is the size of pDetData->DevicePath? 512 bytes, or something else?

Note the second argument of strcpy_s is ignored, as that indicates how much more data many be written to devPath beyond the \0 character encountered in pDetData->DevicePath, and because a runtime error may occur when pDetData->DevicePath is too large (depending on the installed constraint handler). If the intention was not to overflow devPath, then strncpy_s should be used (note the extra n in the function name).

ImpossibleShape commented 1 year ago

Your description of the 2nd parameter is incorrect or worded where I don't understand it. It's defined as destsz - maximum number of characters to write, typically the size of the destination buffer

if destsz is smaller than the string length of the source then as you mention the currently installed constraint handler function is called. This is the intent. It's the equivalent of protecting against a developer bug you might use an assert to catch, however, in the code being analyzed the size of pDetData->DevicePath is 256 byes so it could never be bigger than the destination (I have no idea where the 512 is coming from in the error message)

ether way it's not a "cpp/badly-bounded-write" with the description

"The program performs a buffer copy or write operation with an incorrect upper limit on the size of the copy. A sufficiently long input will overflow the target buffer. In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code."

jketema commented 1 year ago

My apologies. I had not read the query code correctly. The second argument should indeed taken into account, which means you've indeed found a false positive. Unfortunately, just copying an pasting the code you provided does not trigger the alert. Hence, I will need more context to do anything with this report.

ImpossibleShape commented 1 year ago

I'll try and see if I can make a demo app that reproduces it. The actual code is in a private enterprise repo I can't share unfortunately.

jketema commented 1 year ago

If it's private enterprise code, then you likely have a GHAS license, which means we can handle this confidentially via the GitHub support channels.