hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-24 04:36:35 +01:00
Code Issues Packages Projects Releases Wiki Activity

Brodes/overflow buffer fixes (#79)

Browse Source 
* Addreessing false positive due to incorrect use of getType

* Addressing false positive with strncpy.

* BufferAccess must be reachable. False positives observed where accesses occur in dead code.

* Formatting and updating tests.
This commit is contained in:
Ben Rodes
2024-08-12 16:00:11 -04:00
committed by GitHub
parent fd512d2a2d
commit d4c0c4059d
4 changed files with 39 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll
View File

@@ -57,7 +57,7 @@ private int isSource(Expr bufferExpr, Element why) {
exists(Type bufferType |
// buffer is the address of a variable
why = bufferExpr.(AddressOfExpr).getAddressable() and
bufferType = why.(Variable).getType() and
bufferType = why.(Variable).getUnspecifiedType() and
result = bufferType.getSize() and
not bufferType instanceof ReferenceType and
not any(Union u).getAMemberVariable() = why

15
cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll
View File

@@ -14,7 +14,11 @@ int getPointedSize(Type t) {
* BufferWrite differ.
*/
abstract class BufferAccess extends Expr {
BufferAccess() { not this.isUnevaluated() }
BufferAccess() {
not this.isUnevaluated() and
//A buffer access must be reachable (not in dead code)
reachable(this)
}
abstract string getName();
@@ -125,10 +129,11 @@ class StrncpyBA extends BufferAccess {
result = this.(FunctionCall).getArgument(0) and
bufferDesc = "destination buffer" and
accessType = 2
or
result = this.(FunctionCall).getArgument(1) and
bufferDesc = "source buffer" and
accessType = 2
// Ignore this case as reading past the source null terminator is not the behavior of strncpy
// or
// result = this.(FunctionCall).getArgument(1) and
// bufferDesc = "source buffer" and
// accessType = 2
}
override Expr getSizeExpr() { result = this.(FunctionCall).getArgument(2) }

26
cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected
View File

@@ -31,9 +31,9 @@ edges
| main.cpp:9:29:9:32 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance | |
| main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | **argv | provenance | |
| main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | *argv | provenance | |
| main.cpp:10:20:10:23 | **argv | tests.cpp:657:32:657:35 | **argv | provenance | |
| main.cpp:10:20:10:23 | *argv | tests.cpp:657:32:657:35 | **argv | provenance | |
| main.cpp:10:20:10:23 | *argv | tests.cpp:657:32:657:35 | *argv | provenance | |
| main.cpp:10:20:10:23 | **argv | tests.cpp:672:32:672:35 | **argv | provenance | |
| main.cpp:10:20:10:23 | *argv | tests.cpp:672:32:672:35 | **argv | provenance | |
| main.cpp:10:20:10:23 | *argv | tests.cpp:672:32:672:35 | *argv | provenance | |
| overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | **argv | provenance | |
| overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | *argv | provenance | |
| test_buffer_overrun.cpp:32:46:32:49 | **argv | test_buffer_overrun.cpp:32:46:32:49 | **argv | provenance | |
@@ -46,12 +46,12 @@ edges
| tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:14:628:19 | *home | provenance | |
| tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:16:628:19 | *home | provenance | |
| tests.cpp:628:16:628:19 | *home | tests.cpp:628:14:628:19 | *home | provenance | |
| tests.cpp:657:32:657:35 | **argv | tests.cpp:682:9:682:15 | *access to array | provenance | |
| tests.cpp:657:32:657:35 | **argv | tests.cpp:683:9:683:15 | *access to array | provenance | |
| tests.cpp:657:32:657:35 | *argv | tests.cpp:682:9:682:15 | *access to array | provenance | |
| tests.cpp:657:32:657:35 | *argv | tests.cpp:683:9:683:15 | *access to array | provenance | |
| tests.cpp:682:9:682:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | |
| tests.cpp:683:9:683:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance | |
| tests.cpp:672:32:672:35 | **argv | tests.cpp:697:9:697:15 | *access to array | provenance | |
| tests.cpp:672:32:672:35 | **argv | tests.cpp:698:9:698:15 | *access to array | provenance | |
| tests.cpp:672:32:672:35 | *argv | tests.cpp:697:9:697:15 | *access to array | provenance | |
| tests.cpp:672:32:672:35 | *argv | tests.cpp:698:9:698:15 | *access to array | provenance | |
| tests.cpp:697:9:697:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | |
| tests.cpp:698:9:698:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance | |
| tests_restrict.c:15:41:15:44 | **argv | tests_restrict.c:15:41:15:44 | **argv | provenance | |
| tests_restrict.c:15:41:15:44 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance | |
nodes
@@ -85,10 +85,10 @@ nodes
| tests.cpp:628:14:628:14 | *s [*home] | semmle.label | *s [*home] |
| tests.cpp:628:14:628:19 | *home | semmle.label | *home |
| tests.cpp:628:16:628:19 | *home | semmle.label | *home |
| tests.cpp:657:32:657:35 | **argv | semmle.label | **argv |
| tests.cpp:657:32:657:35 | *argv | semmle.label | *argv |
| tests.cpp:682:9:682:15 | *access to array | semmle.label | *access to array |
| tests.cpp:683:9:683:15 | *access to array | semmle.label | *access to array |
| tests.cpp:672:32:672:35 | **argv | semmle.label | **argv |
| tests.cpp:672:32:672:35 | *argv | semmle.label | *argv |
| tests.cpp:697:9:697:15 | *access to array | semmle.label | *access to array |
| tests.cpp:698:9:698:15 | *access to array | semmle.label | *access to array |
| tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
| tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
| tests_restrict.c:15:41:15:44 | *argv | semmle.label | *argv |

15
cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp
View File

@@ -654,6 +654,21 @@ void test26(bool cond)
if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1]
}
void test27(){
char *src = "";
char *dest = "abcdefgh";
const int size = 100;
int ind = 100;
char buffer[size];
strncpy(dest, src, strlen(dest)); // GOOD, strncpy will not read past null terminator of source
if(ind < size){
buffer[ind] = 0; // GOOD: out of bounds, but inaccessible code
}
}
int tests_main(int argc, char *argv[])
{
long long arr17[19];