From 1643a66183bd5d3fb343f887c47569d6fbdd7a74 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2025 15:38:14 +0000 Subject: [PATCH 1/6] C++: Add 'cpp/overflow-buffer' FP tests. --- .../CWE-119/SAMATE/UnboundedWrite.expected | 2 +- .../semmle/tests/OverflowBuffer.expected | 7 ++++ .../semmle/tests/UnboundedWrite.expected | 24 +++++------ .../CWE/CWE-119/semmle/tests/tests.cpp | 41 +++++++++++++++++++ 4 files changed, 61 insertions(+), 13 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/UnboundedWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/UnboundedWrite.expected index 7cefb7cfafce..e217064d1dfc 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/UnboundedWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/UnboundedWrite.expected @@ -1,4 +1,4 @@ edges -subpaths nodes +subpaths #select diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected index 3235b1abe7fa..134537fa2964 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected @@ -49,6 +49,13 @@ | tests.cpp:577:7:577:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array | | tests.cpp:637:6:637:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:633:7:633:12 | buffer | array | | tests.cpp:645:7:645:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:633:7:633:12 | buffer | array | +| tests.cpp:696:3:696:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:691:16:691:16 | a | destination buffer | +| tests.cpp:700:3:700:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:692:16:692:16 | b | destination buffer | +| tests.cpp:708:3:708:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | +| tests.cpp:712:3:712:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | +| tests.cpp:716:3:716:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | +| tests.cpp:726:2:726:7 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:691:16:691:16 | a | destination buffer | +| tests.cpp:727:2:727:7 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | | tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer | | unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer | | unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:15:7:15:11 | small | destination buffer | 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 98e6fb3a0144..82b5412dd4cf 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 @@ -27,8 +27,8 @@ 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: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:730:32:730:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:730:32:730: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 | | @@ -41,12 +41,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: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.cpp:730:32:730:35 | **argv | tests.cpp:755:9:755:15 | *access to array | provenance | | +| tests.cpp:730:32:730:35 | **argv | tests.cpp:756:9:756:15 | *access to array | provenance | | +| tests.cpp:730:32:730:35 | *argv | tests.cpp:755:9:755:15 | *access to array | provenance | | +| tests.cpp:730:32:730:35 | *argv | tests.cpp:756:9:756:15 | *access to array | provenance | | +| tests.cpp:755:9:755:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | | +| tests.cpp:756:9:756: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 @@ -80,10 +80,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: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.cpp:730:32:730:35 | **argv | semmle.label | **argv | +| tests.cpp:730:32:730:35 | *argv | semmle.label | *argv | +| tests.cpp:755:9:755:15 | *access to array | semmle.label | *access to array | +| tests.cpp:756:9:756: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 50ee477085e0..81d76dc72f63 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 @@ -685,6 +685,47 @@ int test28(MYSTRUCTREF g) return memcmp(&g, &_myStruct, sizeof(MYSTRUCT)); // GOOD } +#define offsetof(s, m) __builtin_offsetof(s, m) + +struct HasSomeFields { + unsigned long a; + unsigned long b; + unsigned long c; + + void test29() { + memset(&a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD [FALSE POSITIVE] + }; + + void test30() { + memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // GOOD [FALSE POSITIVE] + }; + + void test31() { + memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, c)); // GOOD + }; + + void test32() { + memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // BAD + }; + + void test33() { + memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // BAD + }; + + void test34() { + memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // BAD + }; + + void test35() { + memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b) - sizeof(unsigned long)); // GOOD + }; +}; + +void test36() { + HasSomeFields hsf; + memset(&hsf.a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD [FALSE POSITIVE] + memset(&hsf.c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // BAD +} int tests_main(int argc, char *argv[]) { From d6054c9a5104fe486e15f54d94528ea8d01c9ec1 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2025 15:48:04 +0000 Subject: [PATCH 2/6] C++: Infer larger buffer sizes for non-static member variables. --- cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll index 95fb7e475933..567760019095 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll @@ -54,13 +54,28 @@ private int isSource(Expr bufferExpr, Element why) { result = bufferExpr.(AllocationExpr).getSizeBytes() and why = bufferExpr or - exists(Type bufferType | + exists(Type bufferType, Variable v | + v = why and // buffer is the address of a variable why = bufferExpr.(AddressOfExpr).getAddressable() and - bufferType = why.(Variable).getUnspecifiedType() and - result = bufferType.getSize() and + bufferType = v.getUnspecifiedType() and not bufferType instanceof ReferenceType and not any(Union u).getAMemberVariable() = why + | + not v instanceof Field and + result = bufferType.getSize() + or + // If it's an address of a field (i.e., a non-static member variable) + // then it's okay to use that address to access the other member variables. + // For example, this is okay: + // ``` + // struct S { uint8_t a, b, c; }; + // S s; + // memset(&s.a, 0, sizeof(S) - offsetof(S, a)); + exists(Field f | + v = f and + result = f.getDeclaringType().getSize() - f.getByteOffset() + ) ) or exists(Union bufferType | From c9a3cf4bd0257688414540ce3583dc9204b06dbc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2025 15:48:11 +0000 Subject: [PATCH 3/6] C++: Accept test changes. --- .../CWE/CWE-119/semmle/tests/OverflowBuffer.expected | 3 --- .../query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected index 134537fa2964..8f78aea1533c 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected @@ -49,12 +49,9 @@ | tests.cpp:577:7:577:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array | | tests.cpp:637:6:637:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:633:7:633:12 | buffer | array | | tests.cpp:645:7:645:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:633:7:633:12 | buffer | array | -| tests.cpp:696:3:696:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:691:16:691:16 | a | destination buffer | -| tests.cpp:700:3:700:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:692:16:692:16 | b | destination buffer | | tests.cpp:708:3:708:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | | tests.cpp:712:3:712:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | | tests.cpp:716:3:716:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | -| tests.cpp:726:2:726:7 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:691:16:691:16 | a | destination buffer | | tests.cpp:727:2:727:7 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | | tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer | | unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer | 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 81d76dc72f63..807ccc32c1ee 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 @@ -693,11 +693,11 @@ struct HasSomeFields { unsigned long c; void test29() { - memset(&a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD [FALSE POSITIVE] + memset(&a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD }; void test30() { - memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // GOOD [FALSE POSITIVE] + memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // GOOD }; void test31() { @@ -723,7 +723,7 @@ struct HasSomeFields { void test36() { HasSomeFields hsf; - memset(&hsf.a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD [FALSE POSITIVE] + memset(&hsf.a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD memset(&hsf.c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // BAD } From 202a5e86da5a8e51ef71e93a34ef1b8420e85d39 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2025 16:07:09 +0000 Subject: [PATCH 4/6] C++: Add change note. --- cpp/ql/src/change-notes/2025-01-28-overflow-buffer.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2025-01-28-overflow-buffer.md diff --git a/cpp/ql/src/change-notes/2025-01-28-overflow-buffer.md b/cpp/ql/src/change-notes/2025-01-28-overflow-buffer.md new file mode 100644 index 000000000000..e06ad16a71ed --- /dev/null +++ b/cpp/ql/src/change-notes/2025-01-28-overflow-buffer.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Call to memory access function may overflow buffer" query (`cpp/overflow-buffer`) now produces fewer FPs involving non-static member variables. From 373b38e881c58d5e2f87d3c91e61e27b5af53a10 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 29 Jan 2025 11:03:52 +0000 Subject: [PATCH 5/6] 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 807ccc32c1ee..bcaf2a38d867 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 @@ -713,7 +713,7 @@ struct HasSomeFields { }; void test34() { - memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // BAD + memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // BAD }; void test35() { From 48cae7e7ed25dd50588a8d13c76c4f5de3a5b6dc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 29 Jan 2025 11:04:55 +0000 Subject: [PATCH 6/6] C++: Accept test changes after previous commit. --- .../Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected index 8f78aea1533c..78adaebd0527 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected @@ -51,7 +51,7 @@ | tests.cpp:645:7:645:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:633:7:633:12 | buffer | array | | tests.cpp:708:3:708:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | | tests.cpp:712:3:712:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | -| tests.cpp:716:3:716:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | +| tests.cpp:716:3:716:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 16 bytes. | tests.cpp:692:16:692:16 | b | destination buffer | | tests.cpp:727:2:727:7 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer | | tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer | | unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer |