From 26be9839df4c5d6e9b004719ea35f82be1de06e5 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 8 Aug 2025 15:10:40 +0100 Subject: [PATCH 1/4] C++: Add FP. --- .../semmle/tests/OverflowBuffer.expected | 1 + .../semmle/tests/UnboundedWrite.expected | 24 +++++++++---------- .../CWE/CWE-119/semmle/tests/tests.cpp | 14 +++++++++++ 3 files changed, 27 insertions(+), 12 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 ac0e8d3a25a8..a4a04cc74824 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 @@ -92,6 +92,7 @@ | tests.cpp:1052:2:1052:7 | call to memset | This 'memset' operation accesses 132 bytes but the $@ is only 64 bytes. | tests.cpp:1041:8:1041:14 | buffer2 | destination buffer | | tests.cpp:1055:2:1055:8 | call to strncpy | This 'strncpy' operation may access 131 bytes but the $@ is only 128 bytes. | tests.cpp:1037:8:1037:14 | buffer1 | destination buffer | | tests.cpp:1057:2:1057:8 | call to strncpy | This 'strncpy' operation may access 131 bytes but the $@ is only 64 bytes. | tests.cpp:1041:8:1041:14 | buffer2 | destination buffer | +| tests.cpp:1070:3:1070:8 | call to memcpy | This 'memcpy' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:1069:4:1069:4 | e | 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 30b78aa875ca..5c10f6e059d1 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:1060:32:1060:35 | **argv | provenance | | -| main.cpp:10:20:10:23 | *argv | tests.cpp:1060:32:1060:35 | *argv | provenance | | +| main.cpp:10:20:10:23 | **argv | tests.cpp:1074:32:1074:35 | **argv | provenance | | +| main.cpp:10:20:10:23 | *argv | tests.cpp:1074:32:1074: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:649:14:649:14 | *s [*home] | tests.cpp:649:14:649:19 | *home | provenance | | | tests.cpp:649:14:649:14 | *s [*home] | tests.cpp:649:16:649:19 | *home | provenance | | | tests.cpp:649:16:649:19 | *home | tests.cpp:649:14:649:19 | *home | provenance | | -| tests.cpp:1060:32:1060:35 | **argv | tests.cpp:1085:9:1085:15 | *access to array | provenance | | -| tests.cpp:1060:32:1060:35 | **argv | tests.cpp:1086:9:1086:15 | *access to array | provenance | | -| tests.cpp:1060:32:1060:35 | *argv | tests.cpp:1085:9:1085:15 | *access to array | provenance | | -| tests.cpp:1060:32:1060:35 | *argv | tests.cpp:1086:9:1086:15 | *access to array | provenance | | -| tests.cpp:1085:9:1085:15 | *access to array | tests.cpp:634:19:634:24 | *source | provenance | | -| tests.cpp:1086:9:1086:15 | *access to array | tests.cpp:643:19:643:24 | *source | provenance | | +| tests.cpp:1074:32:1074:35 | **argv | tests.cpp:1099:9:1099:15 | *access to array | provenance | | +| tests.cpp:1074:32:1074:35 | **argv | tests.cpp:1100:9:1100:15 | *access to array | provenance | | +| tests.cpp:1074:32:1074:35 | *argv | tests.cpp:1099:9:1099:15 | *access to array | provenance | | +| tests.cpp:1074:32:1074:35 | *argv | tests.cpp:1100:9:1100:15 | *access to array | provenance | | +| tests.cpp:1099:9:1099:15 | *access to array | tests.cpp:634:19:634:24 | *source | provenance | | +| tests.cpp:1100:9:1100:15 | *access to array | tests.cpp:643:19:643: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:649:14:649:14 | *s [*home] | semmle.label | *s [*home] | | tests.cpp:649:14:649:19 | *home | semmle.label | *home | | tests.cpp:649:16:649:19 | *home | semmle.label | *home | -| tests.cpp:1060:32:1060:35 | **argv | semmle.label | **argv | -| tests.cpp:1060:32:1060:35 | *argv | semmle.label | *argv | -| tests.cpp:1085:9:1085:15 | *access to array | semmle.label | *access to array | -| tests.cpp:1086:9:1086:15 | *access to array | semmle.label | *access to array | +| tests.cpp:1074:32:1074:35 | **argv | semmle.label | **argv | +| tests.cpp:1074:32:1074:35 | *argv | semmle.label | *argv | +| tests.cpp:1099:9:1099:15 | *access to array | semmle.label | *access to array | +| tests.cpp:1100:9:1100: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 cdef965314b0..e4d815e4cf81 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 @@ -1057,6 +1057,20 @@ void test30() { strncpy(us.buffer2, "", sizeof(us) - 1); // BAD } +struct S_Size16 { + unsigned short uint16; + unsigned char uint8; + unsigned char raw[13]; +}; + +void test31() { + S_Size16 e; + + [&e](void* data){ + memcpy(&e, data, sizeof(e)); // GOOD [FALSE POSITIVE] + }; +} + int tests_main(int argc, char *argv[]) { long long arr17[19]; From d76ce4fb6994fa11af35d757fbfd34bbe26795be Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 8 Aug 2025 15:12:45 +0100 Subject: [PATCH 2/4] C++: Also handle reference types when computing 'trueSize'. --- cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll index 565c15d0d5c9..675090ad9584 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll @@ -57,6 +57,18 @@ private Class getRootType(FieldAccess fa) { ) } +/** + * Gets the size of `v`. This predicate does not have a result when the + * unspecified type of `v` is a `ReferenceType`. + */ +private int getVariableSize(Variable v) { + exists(Type t | + t = v.getUnspecifiedType() and + not t instanceof ReferenceType and + result = t.getSize() + ) +} + /** * Gets the size of the buffer access at `va`. */ @@ -64,12 +76,8 @@ private int getSize(VariableAccess va) { exists(Variable v | va.getTarget() = v | // If `v` is not a field then the size of the buffer is just // the size of the type of `v`. - exists(Type t | - t = v.getUnspecifiedType() and - not v instanceof Field and - not t instanceof ReferenceType and - result = t.getSize() - ) + not v instanceof Field and + result = getVariableSize(v) or exists(Class c, int trueSize | // Otherwise, we find the "outermost" object and compute the size @@ -92,7 +100,7 @@ private int getSize(VariableAccess va) { // buffer is `12 - 4 = 8`. c = getRootType(va) and // we calculate the size based on the last field, to avoid including any padding after it - trueSize = max(Field f | | f.getOffsetInClass(c) + f.getUnspecifiedType().getSize()) and + trueSize = max(Field f | | f.getOffsetInClass(c) + getVariableSize(f)) and result = trueSize - v.(Field).getOffsetInClass(c) ) ) From 0c9d14f417734335b8684c338bf1833daaca93a0 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 8 Aug 2025 15:12:57 +0100 Subject: [PATCH 3/4] C++: Accept test changes. --- .../Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected | 1 - .../query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp | 2 +- 2 files changed, 1 insertion(+), 2 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 a4a04cc74824..ac0e8d3a25a8 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 @@ -92,7 +92,6 @@ | tests.cpp:1052:2:1052:7 | call to memset | This 'memset' operation accesses 132 bytes but the $@ is only 64 bytes. | tests.cpp:1041:8:1041:14 | buffer2 | destination buffer | | tests.cpp:1055:2:1055:8 | call to strncpy | This 'strncpy' operation may access 131 bytes but the $@ is only 128 bytes. | tests.cpp:1037:8:1037:14 | buffer1 | destination buffer | | tests.cpp:1057:2:1057:8 | call to strncpy | This 'strncpy' operation may access 131 bytes but the $@ is only 64 bytes. | tests.cpp:1041:8:1041:14 | buffer2 | destination buffer | -| tests.cpp:1070:3:1070:8 | call to memcpy | This 'memcpy' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:1069:4:1069:4 | e | 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/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp index e4d815e4cf81..555c8e25fb50 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 @@ -1067,7 +1067,7 @@ void test31() { S_Size16 e; [&e](void* data){ - memcpy(&e, data, sizeof(e)); // GOOD [FALSE POSITIVE] + memcpy(&e, data, sizeof(e)); // GOOD }; } From b00107f9277818c6605fcc18b6c8f3bf4c9fe86b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 8 Aug 2025 15:23:40 +0100 Subject: [PATCH 4/4] C++: Add change note. --- cpp/ql/src/change-notes/2025-08-08-overflow-buffer.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2025-08-08-overflow-buffer.md diff --git a/cpp/ql/src/change-notes/2025-08-08-overflow-buffer.md b/cpp/ql/src/change-notes/2025-08-08-overflow-buffer.md new file mode 100644 index 000000000000..02c73804c9f0 --- /dev/null +++ b/cpp/ql/src/change-notes/2025-08-08-overflow-buffer.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Fixed a false positive in `cpp/overflow-buffer` when the type of the destination buffer is a reference to a class/struct type. \ No newline at end of file