From b37bb660c50e80d2a353f478d4ea7a7d90b0fbdf Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 21 Mar 2023 09:24:35 +0000 Subject: [PATCH 1/6] C++: Add FP caused by a BufferAccess inside an unevalauted context. --- .../Critical/OverflowStatic/OverflowStatic.expected | 1 + .../CWE/CWE-119/semmle/tests/OverflowBuffer.expected | 1 + .../CWE/CWE-119/semmle/tests/OverflowStatic.expected | 1 + .../query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp | 6 ++++++ 4 files changed, 9 insertions(+) diff --git a/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected b/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected index 0e5bbee7d73..e9ee2bce6fe 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected +++ b/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected @@ -14,3 +14,4 @@ | test.cpp:24:27:24:27 | 4 | Potential buffer-overflow: 'buffer1' has size 3 not 4. | | test.cpp:26:27:26:27 | 4 | Potential buffer-overflow: 'buffer2' has size 3 not 4. | | test.cpp:40:22:40:27 | amount | Potential buffer-overflow: 'buffer' has size 100 not 101. | +| test.cpp:62:33:62:43 | access to array | Potential buffer-overflow: 'buffer' has size 100 but 'buffer[101]' may be accessed here. | 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 8006b5b61a0..f21f8ab3646 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 @@ -50,6 +50,7 @@ | tests.cpp:546:6:546:10 | call to fread | This 'fread' operation may access 400 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer | | tests.cpp:569:6:569:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array | | 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:608:33:608:43 | access to array | This array indexing operation accesses byte offset 101 but the $@ is only 100 bytes. | tests.cpp:607:7:607:12 | buffer | array | | 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:27:2:27:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected index ac44bbf028d..837d6a930ed 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected @@ -5,4 +5,5 @@ | tests.cpp:245:42:245:42 | 6 | Potential buffer-overflow: 'global_array_5' has size 5 not 6. | | tests.cpp:349:2:349:14 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' may be accessed here. | | tests.cpp:350:17:350:29 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' may be accessed here. | +| tests.cpp:608:33:608:43 | access to array | Potential buffer-overflow: 'buffer' has size 100 but 'buffer[101]' may be accessed here. | | var_size_struct.cpp:103:39:103:41 | 129 | Potential buffer-overflow: 'str' has size 128 not 129. | 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 6dbbcdd4b3e..8918543847f 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 @@ -603,6 +603,11 @@ void test22(bool b, const char* source) { memcpy(dest, source, n); // GOOD } +int test23() { + char buffer[100]; + return sizeof(buffer) / sizeof(buffer[101]); // GOOD [FALSE POSITIVE] +} + int tests_main(int argc, char *argv[]) { long long arr17[19]; @@ -627,6 +632,7 @@ int tests_main(int argc, char *argv[]) test20(); test21(argc == 0); test22(argc == 0, argv[0]); + test23(); return 0; } From 8623d8eb8e4ec0e6208a72eb73491bde50e1b20e Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 21 Mar 2023 09:41:30 +0000 Subject: [PATCH 2/6] C++: Exclude unevaluated expressions from BufferAccess. --- cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll | 2 ++ .../Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected | 1 - .../query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll index faeb859506d..e52babcfe2e 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll +++ b/cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll @@ -14,6 +14,8 @@ int getPointedSize(Type t) { * BufferWrite differ. */ abstract class BufferAccess extends Expr { + BufferAccess() { not this.isUnevaluated() } + abstract string getName(); /** 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 f21f8ab3646..8006b5b61a0 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 @@ -50,7 +50,6 @@ | tests.cpp:546:6:546:10 | call to fread | This 'fread' operation may access 400 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer | | tests.cpp:569:6:569:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array | | 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:608:33:608:43 | access to array | This array indexing operation accesses byte offset 101 but the $@ is only 100 bytes. | tests.cpp:607:7:607:12 | buffer | array | | 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:27:2:27:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 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 8918543847f..90f942023c4 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 @@ -605,7 +605,7 @@ void test22(bool b, const char* source) { int test23() { char buffer[100]; - return sizeof(buffer) / sizeof(buffer[101]); // GOOD [FALSE POSITIVE] + return sizeof(buffer) / sizeof(buffer[101]); // GOOD } int tests_main(int argc, char *argv[]) From 4d2a1ea1496f25b8778c79c4b1a6a213a0763a4f Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 21 Mar 2023 09:50:47 +0000 Subject: [PATCH 3/6] C++: Also add a FP test to 'OverflowStatic'. --- cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp b/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp index 01a4a0adaef..535e3eb0ce4 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp +++ b/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp @@ -56,3 +56,8 @@ void f3() { } } } + +int unevaluated_test() { + char buffer[100]; + return sizeof(buffer) / sizeof(buffer[101]); // GOOD [FALSE POSITIVE] +} \ No newline at end of file From 40cc2e78914bc79f1c0d5e58c69de2ba44629cb6 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 21 Mar 2023 09:53:08 +0000 Subject: [PATCH 4/6] C++: Also exclude unevaluated buffers in 'OverflowStatic'. --- cpp/ql/src/Critical/OverflowStatic.ql | 3 ++- .../Critical/OverflowStatic/OverflowStatic.expected | 1 - cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index 962d2ee89b0..13a4fb6bcb7 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -22,7 +22,8 @@ import LoopBounds private predicate staticBufferBase(VariableAccess access, Variable v) { v.getType().(ArrayType).getBaseType() instanceof CharType and access = v.getAnAccess() and - not memberMayBeVarSize(_, v) + not memberMayBeVarSize(_, v) and + not access.isUnevaluated() } predicate staticBuffer(VariableAccess access, Variable v, int size) { diff --git a/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected b/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected index e9ee2bce6fe..0e5bbee7d73 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected +++ b/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected @@ -14,4 +14,3 @@ | test.cpp:24:27:24:27 | 4 | Potential buffer-overflow: 'buffer1' has size 3 not 4. | | test.cpp:26:27:26:27 | 4 | Potential buffer-overflow: 'buffer2' has size 3 not 4. | | test.cpp:40:22:40:27 | amount | Potential buffer-overflow: 'buffer' has size 100 not 101. | -| test.cpp:62:33:62:43 | access to array | Potential buffer-overflow: 'buffer' has size 100 but 'buffer[101]' may be accessed here. | diff --git a/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp b/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp index 535e3eb0ce4..deeb70ffd57 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp +++ b/cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp @@ -59,5 +59,5 @@ void f3() { int unevaluated_test() { char buffer[100]; - return sizeof(buffer) / sizeof(buffer[101]); // GOOD [FALSE POSITIVE] + return sizeof(buffer) / sizeof(buffer[101]); // GOOD } \ No newline at end of file From eab43973b7546ee222689c108b808aff9458ee9b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 21 Mar 2023 10:00:11 +0000 Subject: [PATCH 5/6] C++: Add change note. --- cpp/ql/lib/change-notes/2023-03-21-buffer-access.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/lib/change-notes/2023-03-21-buffer-access.md diff --git a/cpp/ql/lib/change-notes/2023-03-21-buffer-access.md b/cpp/ql/lib/change-notes/2023-03-21-buffer-access.md new file mode 100644 index 00000000000..7b2b6bad0dc --- /dev/null +++ b/cpp/ql/lib/change-notes/2023-03-21-buffer-access.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `BufferAccess` library (`semmle.code.cpp.security.BufferAccess`) no longer matches buffer accesses inside unevaluated contexts (such as inside `sizeof` or `decltype` expressions). As a result, queries using this library may see fewer false positives. \ No newline at end of file From 2ce0d2b7ee6482209dbd07d9f1bf8f9da1170e26 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 21 Mar 2023 10:07:23 +0000 Subject: [PATCH 6/6] C++: Accept more test changes. --- .../Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected index 837d6a930ed..ac44bbf028d 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected @@ -5,5 +5,4 @@ | tests.cpp:245:42:245:42 | 6 | Potential buffer-overflow: 'global_array_5' has size 5 not 6. | | tests.cpp:349:2:349:14 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' may be accessed here. | | tests.cpp:350:17:350:29 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' may be accessed here. | -| tests.cpp:608:33:608:43 | access to array | Potential buffer-overflow: 'buffer' has size 100 but 'buffer[101]' may be accessed here. | | var_size_struct.cpp:103:39:103:41 | 129 | Potential buffer-overflow: 'str' has size 128 not 129. |