From 9105375097e1bc8e6c8b1e51a8bb3b25a55b9e58 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Thu, 8 Aug 2024 15:07:42 -0400 Subject: [PATCH 01/10] Addreessing false positive due to incorrect use of getType --- cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll index 6bf7764d7e98..95fb7e475933 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll @@ -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 From abd0a076a8179e7662c4366ddfdc29cb9b459602 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Thu, 8 Aug 2024 15:08:53 -0400 Subject: [PATCH 02/10] Addressing false positive with strncpy. --- .../semmle/code/cpp/security/BufferAccess.qll | 9 ++++--- .../semmle/tests/UnboundedWrite.expected | 26 +++++++++---------- .../CWE/CWE-119/semmle/tests/tests.cpp | 7 +++++ 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll index ad7a72262f59..cb6ebd561515 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll @@ -125,10 +125,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) } diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected index cde0ba8bc754..e58b3df4c658 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected @@ -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:664:32:664:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:664:32:664:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:664:32:664: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:664:32:664:35 | **argv | tests.cpp:689:9:689:15 | *access to array | provenance | | +| tests.cpp:664:32:664:35 | **argv | tests.cpp:690:9:690:15 | *access to array | provenance | | +| tests.cpp:664:32:664:35 | *argv | tests.cpp:689:9:689:15 | *access to array | provenance | | +| tests.cpp:664:32:664:35 | *argv | tests.cpp:690:9:690:15 | *access to array | provenance | | +| tests.cpp:689:9:689:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | | +| tests.cpp:690:9:690: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:664:32:664:35 | **argv | semmle.label | **argv | +| tests.cpp:664:32:664:35 | *argv | semmle.label | *argv | +| tests.cpp:689:9:689:15 | *access to array | semmle.label | *access to array | +| tests.cpp:690:9:690: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 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index 9120377edd7e..ebd0a6710c9c 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -654,6 +654,13 @@ void test26(bool cond) if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1] } +void test27(){ + char *src = ""; + char *dest = "abcdefgh"; + + strncpy(dest, src, strlen(dest)); // GOOD, strncpy will not read past null terminator of source +} + int tests_main(int argc, char *argv[]) { long long arr17[19]; From f0eeaaf44e2583b6f5b478fa3d1646681a9b05e6 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Mon, 12 Aug 2024 10:51:04 -0400 Subject: [PATCH 03/10] BufferAccess must be reachable. False positives observed where accesses occur in dead code. --- cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll index cb6ebd561515..e619d73dadf2 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll @@ -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(); From e4d29905a90cd5593e27f389074842f9594c9e30 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Mon, 12 Aug 2024 11:21:55 -0400 Subject: [PATCH 04/10] Formatting and updating tests. --- .../semmle/code/cpp/security/BufferAccess.qll | 4 +-- .../semmle/tests/UnboundedWrite.expected | 26 +++++++++---------- .../CWE/CWE-119/semmle/tests/tests.cpp | 8 ++++++ 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll index e619d73dadf2..89c814e49187 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll @@ -14,8 +14,8 @@ int getPointedSize(Type t) { * BufferWrite differ. */ abstract class BufferAccess extends Expr { - BufferAccess() { - not this.isUnevaluated() and + BufferAccess() { + not this.isUnevaluated() and //A buffer access must be reachable (not in dead code) reachable(this) } diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected index e58b3df4c658..93090e18f569 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected @@ -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:664:32:664:35 | **argv | provenance | | -| main.cpp:10:20:10:23 | *argv | tests.cpp:664:32:664:35 | **argv | provenance | | -| main.cpp:10:20:10:23 | *argv | tests.cpp:664:32:664: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:664:32:664:35 | **argv | tests.cpp:689:9:689:15 | *access to array | provenance | | -| tests.cpp:664:32:664:35 | **argv | tests.cpp:690:9:690:15 | *access to array | provenance | | -| tests.cpp:664:32:664:35 | *argv | tests.cpp:689:9:689:15 | *access to array | provenance | | -| tests.cpp:664:32:664:35 | *argv | tests.cpp:690:9:690:15 | *access to array | provenance | | -| tests.cpp:689:9:689:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | | -| tests.cpp:690:9:690: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:664:32:664:35 | **argv | semmle.label | **argv | -| tests.cpp:664:32:664:35 | *argv | semmle.label | *argv | -| tests.cpp:689:9:689:15 | *access to array | semmle.label | *access to array | -| tests.cpp:690:9:690: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 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index ebd0a6710c9c..846c87dc78ba 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -657,10 +657,18 @@ void test26(bool cond) 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]; From cc953c87d998940fd544301388f2816602f6a982 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Wed, 4 Sep 2024 12:41:05 -0400 Subject: [PATCH 05/10] Added test case to demonstrate type error corrected in this branch. --- .../semmle/tests/UnboundedWrite.expected | 26 +++++++++---------- .../CWE/CWE-119/semmle/tests/tests.cpp | 16 ++++++++++++ 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected index 93090e18f569..5f2bd966ba23 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected @@ -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: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 | | +| main.cpp:10:20:10:23 | **argv | tests.cpp:688:32:688:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:688:32:688:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:688:32:688: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: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.cpp:688:32:688:35 | **argv | tests.cpp:713:9:713:15 | *access to array | provenance | | +| tests.cpp:688:32:688:35 | **argv | tests.cpp:714:9:714:15 | *access to array | provenance | | +| tests.cpp:688:32:688:35 | *argv | tests.cpp:713:9:713:15 | *access to array | provenance | | +| tests.cpp:688:32:688:35 | *argv | tests.cpp:714:9:714:15 | *access to array | provenance | | +| tests.cpp:713:9:713:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | | +| tests.cpp:714:9:714: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: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.cpp:688:32:688:35 | **argv | semmle.label | **argv | +| tests.cpp:688:32:688:35 | *argv | semmle.label | *argv | +| tests.cpp:713:9:713:15 | *access to array | semmle.label | *access to array | +| tests.cpp:714:9:714: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 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index 846c87dc78ba..b4f1ada844c3 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -668,6 +668,22 @@ void test27(){ } } +typedef struct _MYSTRUCT { + unsigned long a; + unsigned short b; + unsigned char z[ 100 ]; +} MYSTRUCT; + + +const MYSTRUCT _myStruct = { 0 }; +typedef const MYSTRUCT& MYSTRUCTREF; + +// False positive case due to use of typedefs +int test27(MYSTRUCTREF g) +{ + return memcmp(&g, &_myStruct, sizeof(MYSTRUCT)); // GOOD +} + int tests_main(int argc, char *argv[]) { From 77b88afa9e45f1c42b5e41a46a952150d06a5bca Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Wed, 4 Sep 2024 12:44:36 -0400 Subject: [PATCH 06/10] Removing commented out code and altering comments to explain why the code was removed. --- cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll index 89c814e49187..8615db5ddc89 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll @@ -129,11 +129,7 @@ class StrncpyBA extends BufferAccess { result = this.(FunctionCall).getArgument(0) and bufferDesc = "destination 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 + // NOTE, ignoring getting the source buffer (arg 1) since reading past the the source null terminator is not the behavior of strncpy } override Expr getSizeExpr() { result = this.(FunctionCall).getArgument(2) } From 6f1aade8e05433f7734b0f04f07260eadf898a44 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Wed, 4 Sep 2024 12:51:37 -0400 Subject: [PATCH 07/10] Added change log --- .../2024-09-04-overflow-buffer-false-positives.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md diff --git a/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md b/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md new file mode 100644 index 000000000000..1f59e51cf360 --- /dev/null +++ b/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Altered Buffer.qll and BuferAccess.qll to account for observed false positives. \ No newline at end of file From 78898955b6eae12ab852ef0b195cd980a3ae2807 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Wed, 4 Sep 2024 12:54:27 -0400 Subject: [PATCH 08/10] Updating change log --- .../change-notes/2024-09-04-overflow-buffer-false-positives.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md b/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md index 1f59e51cf360..a80f3b684a08 100644 --- a/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md +++ b/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md @@ -1,4 +1,5 @@ --- category: minorAnalysis --- -* Altered Buffer.qll and BuferAccess.qll to account for observed false positives. \ No newline at end of file +* Removed false positives caused by buffer accesses in unreachable code +* Removed false positives caused by inconsistent type checking \ No newline at end of file From 8ab22feae17c60f8b6930b866ed918ac95b1f5c4 Mon Sep 17 00:00:00 2001 From: Ben Rodes Date: Thu, 5 Sep 2024 10:07:19 -0400 Subject: [PATCH 09/10] Update cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com> --- .../query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index b4f1ada844c3..83bc3e081941 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -679,7 +679,7 @@ const MYSTRUCT _myStruct = { 0 }; typedef const MYSTRUCT& MYSTRUCTREF; // False positive case due to use of typedefs -int test27(MYSTRUCTREF g) +int test28(MYSTRUCTREF g) { return memcmp(&g, &_myStruct, sizeof(MYSTRUCT)); // GOOD } From 1005a89007a71fbe5cd51462a1e24b05ac162675 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Thu, 5 Sep 2024 15:25:23 -0400 Subject: [PATCH 10/10] Updating test cases. --- .../semmle/code/cpp/security/BufferAccess.qll | 7 +++++- cpp/ql/src/Critical/OverflowStatic.ql | 4 +++- .../Security/CWE/CWE-119/OverflowBuffer.ql | 1 + .../semmle/tests/UnboundedWrite.expected | 24 +++++++++---------- .../CWE/CWE-119/semmle/tests/tests.cpp | 11 +++++---- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll index 8615db5ddc89..247f654a310c 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll @@ -30,6 +30,8 @@ abstract class BufferAccess extends Expr { * - 1 = buffer range [0, getSize) is accessed entirely. * - 2 = buffer range [0, getSize) may be accessed partially or entirely. * - 3 = buffer is accessed at offset getSize - 1. + * - 4 = buffer is accessed with null terminator read protections + * (does not read past null terminator, regardless of access size) */ abstract Expr getBuffer(string bufferDesc, int accessType); @@ -129,7 +131,10 @@ class StrncpyBA extends BufferAccess { result = this.(FunctionCall).getArgument(0) and bufferDesc = "destination buffer" and accessType = 2 - // NOTE, ignoring getting the source buffer (arg 1) since reading past the the source null terminator is not the behavior of strncpy + or + result = this.(FunctionCall).getArgument(1) and + bufferDesc = "source buffer" and + accessType = 4 } override Expr getSizeExpr() { result = this.(FunctionCall).getArgument(2) } diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index 13a4fb6bcb76..519c3f9b4015 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -42,7 +42,9 @@ class BufferAccess extends ArrayExpr { not exists(Macro m | m.getName() = "strcmp" and m.getAnInvocation().getAnExpandedElement() = this - ) + ) and + //A buffer access must be reachable (not in dead code) + reachable(this) } int bufferSize() { staticBuffer(this.getArrayBase(), _, result) } diff --git a/cpp/ql/src/Security/CWE/CWE-119/OverflowBuffer.ql b/cpp/ql/src/Security/CWE/CWE-119/OverflowBuffer.ql index 1c903081baf2..378926828146 100644 --- a/cpp/ql/src/Security/CWE/CWE-119/OverflowBuffer.ql +++ b/cpp/ql/src/Security/CWE/CWE-119/OverflowBuffer.ql @@ -26,6 +26,7 @@ from BufferAccess ba, string bufferDesc, int accessSize, int accessType, Element bufferAlloc, int bufferSize, string message where + accessType != 4 and accessSize = ba.getSize() and bufferSize = getBufferSize(ba.getBuffer(bufferDesc, accessType), bufferAlloc) and ( diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected index 5f2bd966ba23..c71c9c7926e0 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected @@ -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:688:32:688:35 | **argv | provenance | | -| main.cpp:10:20:10:23 | *argv | tests.cpp:688:32:688:35 | **argv | provenance | | -| main.cpp:10:20:10:23 | *argv | tests.cpp:688:32:688:35 | *argv | provenance | | +| main.cpp:10:20:10:23 | **argv | tests.cpp:689:32:689:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:689:32:689:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:689:32:689: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:688:32:688:35 | **argv | tests.cpp:713:9:713:15 | *access to array | provenance | | -| tests.cpp:688:32:688:35 | **argv | tests.cpp:714:9:714:15 | *access to array | provenance | | -| tests.cpp:688:32:688:35 | *argv | tests.cpp:713:9:713:15 | *access to array | provenance | | -| tests.cpp:688:32:688:35 | *argv | tests.cpp:714:9:714:15 | *access to array | provenance | | -| tests.cpp:713:9:713:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | | -| tests.cpp:714:9:714:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance | | +| tests.cpp:689:32:689:35 | **argv | tests.cpp:714:9:714:15 | *access to array | provenance | | +| tests.cpp:689:32:689:35 | **argv | tests.cpp:715:9:715:15 | *access to array | provenance | | +| tests.cpp:689:32:689:35 | *argv | tests.cpp:714:9:714:15 | *access to array | provenance | | +| tests.cpp:689:32:689:35 | *argv | tests.cpp:715:9:715:15 | *access to array | provenance | | +| tests.cpp:714:9:714:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | | +| tests.cpp:715:9:715: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:688:32:688:35 | **argv | semmle.label | **argv | -| tests.cpp:688:32:688:35 | *argv | semmle.label | *argv | -| tests.cpp:713:9:713:15 | *access to array | semmle.label | *access to array | +| tests.cpp:689:32:689:35 | **argv | semmle.label | **argv | +| tests.cpp:689:32:689:35 | *argv | semmle.label | *argv | | tests.cpp:714:9:714:15 | *access to array | semmle.label | *access to array | +| tests.cpp:715:9:715: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 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index 83bc3e081941..50ee477085e0 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp @@ -654,17 +654,18 @@ void test26(bool cond) if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1] } +#define IND 100 +#define MAX_SIZE 100 void test27(){ char *src = ""; char *dest = "abcdefgh"; - const int size = 100; int ind = 100; - char buffer[size]; + char buffer[MAX_SIZE]; - strncpy(dest, src, strlen(dest)); // GOOD, strncpy will not read past null terminator of source + strncpy(dest, src, 8); // GOOD, strncpy will not read past null terminator of source - if(ind < size){ - buffer[ind] = 0; // GOOD: out of bounds, but inaccessible code + if(IND < MAX_SIZE){ + buffer[IND] = 0; // GOOD: out of bounds, but inaccessible code } }