From c06f7259cf25b2162dba0917bfd2da75c49371f8 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 16 Dec 2022 12:58:45 +0000 Subject: [PATCH 1/2] C++: Make the 'getBufferSize' a lot more like the pre-use-use flow implementation. --- cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll | 80 +++++-------------- 1 file changed, 21 insertions(+), 59 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll index f82b470a8d6..45ebafd203e 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll @@ -73,67 +73,29 @@ private int isSource(Expr bufferExpr, Element why) { ) } -/** - * Holds if `e2` is an expression that is derived from `e1` such that if `e1[n]` is a - * well-defined expression for some number `n`, then `e2[n + delta]` is also a well-defined - * expression. - */ -private predicate step(Expr e1, Expr e2, int delta) { - getBufferSizeCand0(e1) and - ( - exists(Variable bufferVar, Class parentClass, VariableAccess parentPtr, int bufferSize | - e1 = parentPtr - | - bufferVar = e2.(VariableAccess).getTarget() and - // buffer is the parentPtr->bufferVar of a 'variable size struct' - memberMayBeVarSize(parentClass, bufferVar) and - parentPtr = e2.(VariableAccess).getQualifier() and - parentPtr.getTarget().getUnspecifiedType().(PointerType).getBaseType() = parentClass and - ( - if exists(bufferVar.getType().getSize()) - then bufferSize = bufferVar.getType().getSize() - else bufferSize = 0 - ) and - delta = bufferSize - parentClass.getSize() - ) - or - DataFlow::localExprFlowStep(e1, e2) and - delta = 0 - ) -} - -pragma[nomagic] -private predicate getBufferSizeCand0(Expr e) { - exists(isSource(e, _)) - or - exists(Expr e0 | - getBufferSizeCand0(e0) and - step(e0, e, _) - ) -} - -/** - * Get the size in bytes of the buffer pointed to by an expression (if this can be determined). - * - * NOTE: There can be multiple `(result, why)` for a given `bufferExpr`. - */ -private int getBufferSizeCand(Expr bufferExpr, Element why) { - getBufferSizeCand0(bufferExpr) and - ( - result = isSource(bufferExpr, why) - or - exists(Expr e0, int delta, int size | - size = getBufferSizeCand(e0, why) and - step(e0, bufferExpr, delta) and - result = size + delta - ) - ) -} - /** * Get the size in bytes of the buffer pointed to by an expression (if this can be determined). */ +language[monotonicAggregates] int getBufferSize(Expr bufferExpr, Element why) { - result = max( | | getBufferSizeCand(bufferExpr, _)) and - result = getBufferSizeCand(bufferExpr, why) + result = isSource(bufferExpr, why) + or + exists(Class parentClass, VariableAccess parentPtr, int bufferSize, Variable bufferVar | + bufferVar = bufferExpr.(VariableAccess).getTarget() and + // buffer is the parentPtr->bufferVar of a 'variable size struct' + memberMayBeVarSize(parentClass, bufferVar) and + why = bufferVar and + parentPtr = bufferExpr.(VariableAccess).getQualifier() and + parentPtr.getTarget().getUnspecifiedType().(PointerType).getBaseType() = parentClass and + result = getBufferSize(parentPtr, _) + bufferSize - parentClass.getSize() + | + if exists(bufferVar.getType().getSize()) + then bufferSize = bufferVar.getType().getSize() + else bufferSize = 0 + ) + or + // dataflow (all sources must be the same size) + result = unique(Expr def | DataFlow::localExprFlowStep(def, bufferExpr) | getBufferSize(def, _)) and + // find reason + exists(Expr def | DataFlow::localExprFlowStep(def, bufferExpr) | exists(getBufferSize(def, why))) } From 81de93da2dc95e38c2223f683c9a7b37858ae2eb Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 16 Dec 2022 12:58:53 +0000 Subject: [PATCH 2/2] C++: Accept test changes --- .../CWE/CWE-119/semmle/tests/OverflowBuffer.expected | 12 ++++++------ .../Security/CWE/CWE-119/semmle/tests/tests.cpp | 6 +++--- .../semmle/tests/VeryLikelyOverrunWrite.expected | 1 + 3 files changed, 10 insertions(+), 9 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 252e581d2d5..b4e73c5df09 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 @@ -42,25 +42,25 @@ | tests.cpp:476:2:476:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array | | tests.cpp:477:2:477:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array | | tests.cpp:481:2:481:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array | -| tests.cpp:485:2:485:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array | | tests.cpp:487:2:487:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array | | tests.cpp:491:2:491:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array | -| tests.cpp:495:2:495:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array | | tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:502:15:502:20 | call to malloc | destination buffer | | tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:510:16:510:21 | call to malloc | destination buffer | | tests.cpp:541:6:541:10 | call to fread | This 'fread' operation may access 101 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer | | 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:579:6:579:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array | | tests.cpp:586:6:586:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565: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 | +| unions.cpp:29:2:29: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 | +| unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | 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 | | unions.cpp:34:2:34:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:16:7:16:11 | large | destination buffer | -| var_size_struct.cpp:71:3:71:8 | call to memset | This 'memset' operation accesses 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:67:35:67:40 | call to malloc | destination buffer | -| var_size_struct.cpp:73:3:73:9 | call to strncpy | This 'strncpy' operation may access 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:67:35:67:40 | call to malloc | destination buffer | -| var_size_struct.cpp:87:3:87:19 | access to array | This array indexing operation accesses byte offset 67 but the $@ is only 64 bytes. | var_size_struct.cpp:82:35:82:40 | call to malloc | array | +| var_size_struct.cpp:71:3:71:8 | call to memset | This 'memset' operation accesses 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:63:8:63:11 | data | destination buffer | +| var_size_struct.cpp:73:3:73:9 | call to strncpy | This 'strncpy' operation may access 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:63:8:63:11 | data | destination buffer | +| var_size_struct.cpp:87:3:87:19 | access to array | This array indexing operation accesses byte offset 67 but the $@ is only 64 bytes. | var_size_struct.cpp:78:7:78:14 | elements | array | | var_size_struct.cpp:99:3:99:8 | call to memset | This 'memset' operation accesses 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer | | var_size_struct.cpp:101:3:101:8 | call to memset | This 'memset' operation accesses 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer | | var_size_struct.cpp:103:3:103:9 | call to strncpy | This 'strncpy' operation may access 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | 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 49c02b9f199..570430d771c 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 @@ -482,7 +482,7 @@ void test18() while (*p3 != 0) { p3 = update(p3); } - p3[-1] = 0; // GOOD [FALSE POSITIVE] + p3[-1] = 0; // GOOD p4[-1] = 0; // BAD: underrun write p4++; @@ -492,7 +492,7 @@ void test18() while (*p5 != 0) { p5 = update(p5); } - p5[-1] = 0; // GOOD [FALSE POSITIVE] + p5[-1] = 0; // GOOD } void test19(bool b) @@ -576,7 +576,7 @@ void test21(bool cond) } else { if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1] } - if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1] or buffer[0] + if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1] or buffer[0] [NOT DETECTED] ptr = buffer; for (i = 0; i < 2; i++) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected index 51554c09406..c20cf040504 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected @@ -17,5 +17,6 @@ | tests.c:186:3:186:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 2 bytes. | | tests.c:189:3:189:9 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. | | unions.c:26:2:26:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | +| unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 15 bytes. | | unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | | var_size_struct.cpp:22:3:22:8 | call to strcpy | This 'call to strcpy' operation requires 10 bytes but the destination is only 9 bytes. |