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 diff --git a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll index ad7a72262f59..247f654a310c 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(); @@ -26,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); @@ -128,7 +134,7 @@ class StrncpyBA extends BufferAccess { or result = this.(FunctionCall).getArgument(1) and bufferDesc = "source buffer" and - accessType = 2 + 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/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..a80f3b684a08 --- /dev/null +++ b/cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* Removed false positives caused by buffer accesses in unreachable code +* Removed false positives caused by inconsistent type checking \ No newline at end of file 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..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: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: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: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: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: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: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 9120377edd7e..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,6 +654,38 @@ 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"; + int ind = 100; + char buffer[MAX_SIZE]; + + strncpy(dest, src, 8); // GOOD, strncpy will not read past null terminator of source + + if(IND < MAX_SIZE){ + buffer[IND] = 0; // GOOD: out of bounds, but inaccessible code + } +} + +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 test28(MYSTRUCTREF g) +{ + return memcmp(&g, &_myStruct, sizeof(MYSTRUCT)); // GOOD +} + + int tests_main(int argc, char *argv[]) { long long arr17[19];