From df9981de4fb2b01383dc1fc06d197f1b3b422e5b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 18 May 2021 17:44:08 +0200 Subject: [PATCH 1/4] C++: Add testcases with false positives. --- .../semmle/tests/OverflowStatic.expected | 2 ++ .../CWE/CWE-119/semmle/tests/tests.cpp | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) 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 ef334a73a2c..0e137bb29d7 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,6 +5,8 @@ | 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]' is accessed here. | | tests.cpp:350:17:350:29 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' is accessed here. | +| tests.cpp:594:4:594:12 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 16 elements. | +| tests.cpp:603:24:603:24 | n | Potential buffer-overflow: 'dest' has size 128 not 132. | | var_size_struct.cpp:54:5:54:14 | access to array | Potential buffer-overflow: 'str' has size 1 but 'str[1]' is accessed here. | | var_size_struct.cpp:55:5:55:14 | access to array | Potential buffer-overflow: 'str' has size 1 but 'str[1]' is 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 c4ccdfad8b3..d18d60a2ed6 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 @@ -586,6 +586,23 @@ void test21(bool cond) if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1] } +void test22(bool b, const char* source) { + char buffer[16]; + int k; + for (k = 0; k <= 100; k++) { + if(k < 16) { + buffer[k] = 'x'; // GOOD [FALSE POSITIVE] + } + } + + char dest[128]; + int n = b ? 1024 : 132; + if (n >= 128) { + return; + } + memcpy(dest, source, n); // GOOD [FALSE POSITIVE] +} + int main(int argc, char *argv[]) { long long arr17[19]; @@ -609,6 +626,7 @@ int main(int argc, char *argv[]) test19(argc == 0); test20(); test21(argc == 0); + test22(argc == 0, argv[0]); return 0; } From 26c4a66dc407bd55b12d3afd83b80083708c85fc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 18 May 2021 17:54:30 +0200 Subject: [PATCH 2/4] C++: Add range analysis to fix FPs. --- cpp/ql/src/Critical/OverflowStatic.ql | 16 ++++++++++++---- .../CWE-119/semmle/tests/OverflowStatic.expected | 2 -- .../Security/CWE/CWE-119/semmle/tests/tests.cpp | 4 ++-- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index 833ee45499e..a7c1cb41cda 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -14,6 +14,7 @@ import cpp import semmle.code.cpp.commons.Buffer +import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis import LoopBounds private predicate staticBufferBase(VariableAccess access, Variable v) { @@ -51,6 +52,8 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) { loop.getStmt().getAChild*() = bufaccess.getEnclosingStmt() and loop.limit() >= bufaccess.bufferSize() and loop.counter().getAnAccess() = bufaccess.getArrayOffset() and + // Ensure that we don't have an upper bound on the array index that's less than the buffer size. + not upperBound(bufaccess.getArrayOffset().getFullyConverted()) < bufaccess.bufferSize() and msg = "Potential buffer-overflow: counter '" + loop.counter().toString() + "' <= " + loop.limit().toString() + " but '" + bufaccess.buffer().getName() + "' has " + @@ -94,10 +97,15 @@ class CallWithBufferSize extends FunctionCall { } int statedSizeValue() { - exists(Expr statedSizeSrc | - DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) and - result = statedSizeSrc.getValue().toInt() - ) + // `upperBound(e)` defaults to `exprMaxVal(e)` when `e` isn't analyzable. So to get a meaningful + // result in this case we pick the minimum value obtainable from dataflow and range analysis. + result = + upperBound(statedSizeExpr()) + .minimum(any(Expr statedSizeSrc | + DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) + | + statedSizeSrc.getValue().toInt() + )) } } 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 0e137bb29d7..ef334a73a2c 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,8 +5,6 @@ | 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]' is accessed here. | | tests.cpp:350:17:350:29 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' is accessed here. | -| tests.cpp:594:4:594:12 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 16 elements. | -| tests.cpp:603:24:603:24 | n | Potential buffer-overflow: 'dest' has size 128 not 132. | | var_size_struct.cpp:54:5:54:14 | access to array | Potential buffer-overflow: 'str' has size 1 but 'str[1]' is accessed here. | | var_size_struct.cpp:55:5:55:14 | access to array | Potential buffer-overflow: 'str' has size 1 but 'str[1]' is 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 d18d60a2ed6..4f8bcc0fa59 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 @@ -591,7 +591,7 @@ void test22(bool b, const char* source) { int k; for (k = 0; k <= 100; k++) { if(k < 16) { - buffer[k] = 'x'; // GOOD [FALSE POSITIVE] + buffer[k] = 'x'; // GOOD } } @@ -600,7 +600,7 @@ void test22(bool b, const char* source) { if (n >= 128) { return; } - memcpy(dest, source, n); // GOOD [FALSE POSITIVE] + memcpy(dest, source, n); // GOOD } int main(int argc, char *argv[]) From 6103aabdce00d9c2d7683b47c8a71af86647cb32 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 18 May 2021 19:17:11 +0200 Subject: [PATCH 3/4] C++: Add change-note. --- cpp/change-notes/2021-05-18-static-buffer-overflow.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 cpp/change-notes/2021-05-18-static-buffer-overflow.md diff --git a/cpp/change-notes/2021-05-18-static-buffer-overflow.md b/cpp/change-notes/2021-05-18-static-buffer-overflow.md new file mode 100644 index 00000000000..f56040fe8aa --- /dev/null +++ b/cpp/change-notes/2021-05-18-static-buffer-overflow.md @@ -0,0 +1,2 @@ +lgtm +* The "Static buffer overflow" query (cpp/static-buffer-overflow) has been improved to produce fewer false positives. From 741eed93b2ddde49d69413cdac4d4511018f40d8 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 19 May 2021 09:03:05 +0200 Subject: [PATCH 4/4] C++: Replace minimum(any(...)) with a min aggregate. Also removed the min aggregate further down since it's no longer needed. --- cpp/ql/src/Critical/OverflowStatic.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index a7c1cb41cda..011c38ff734 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -101,7 +101,7 @@ class CallWithBufferSize extends FunctionCall { // result in this case we pick the minimum value obtainable from dataflow and range analysis. result = upperBound(statedSizeExpr()) - .minimum(any(Expr statedSizeSrc | + .minimum(min(Expr statedSizeSrc | DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) | statedSizeSrc.getValue().toInt() @@ -112,7 +112,7 @@ class CallWithBufferSize extends FunctionCall { predicate wrongBufferSize(Expr error, string msg) { exists(CallWithBufferSize call, int bufsize, Variable buf, int statedSize | staticBuffer(call.buffer(), buf, bufsize) and - statedSize = min(call.statedSizeValue()) and + statedSize = call.statedSizeValue() and statedSize > bufsize and error = call.statedSizeExpr() and msg =