Merge pull request github#6760 from andersfugmann/relax_memberMayBeVarSize

Increase precision to high for cpp/static-buffer-overflow

Author: MathiasVP

cpp/change-notes/2021-09-27-overflow-static.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* Increase precision to high for the "Static buffer overflow" query
+  (`cpp/static-buffer-overflow`). This means the query is run and displayed by default on Code Scanning and LGTM.

cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll

@@ -10,44 +10,11 @@ import semmle.code.cpp.dataflow.DataFlow
  *   char data[1]; // v
  * };
  * ```
- * This requires that `v` is an array of size 0 or 1, and `v` is the last member of `c`.
- * In addition, if the size of the structure is taken, there must be at least one instance
- * where a `c` pointer is allocated with additional space.
- * For example, holds for `c` if it occurs as
- * ```
- * malloc(sizeof(c) + 100 * sizeof(char))
- * ```
- * but not if it only ever occurs as
- * ```
- * malloc(sizeof(c))
- * ```
+ * This requires that `v` is an array of size 0 or 1.
  */
 predicate memberMayBeVarSize(Class c, MemberVariable v) {
-  exists(int i |
-    // `v` is the last field in `c`
-    i = max(int j | c.getCanonicalMember(j) instanceof Field | j) and
-    v = c.getCanonicalMember(i) and
-    // v is an array of size at most 1
-    v.getUnspecifiedType().(ArrayType).getArraySize() <= 1 and
-    not c instanceof Union
-  ) and
-  // If the size is taken, then arithmetic is performed on the result at least once
-  (
-    // `sizeof(c)` is not taken
-    not exists(SizeofOperator so |
-      so.(SizeofTypeOperator).getTypeOperand().getUnspecifiedType() = c or
-      so.(SizeofExprOperator).getExprOperand().getUnspecifiedType() = c
-    )
-    or
-    // or `sizeof(c)` is taken
-    exists(SizeofOperator so |
-      so.(SizeofTypeOperator).getTypeOperand().getUnspecifiedType() = c or
-      so.(SizeofExprOperator).getExprOperand().getUnspecifiedType() = c
-    |
-      // and arithmetic is performed on the result
-      so.getParent*() instanceof AddExpr
-    )
-  )
+  c = v.getDeclaringType() and
+  v.getUnspecifiedType().(ArrayType).getArraySize() <= 1
 }
 
 /**
@@ -60,10 +27,6 @@ int getBufferSize(Expr bufferExpr, Element why) {
     result = bufferVar.getUnspecifiedType().(ArrayType).getSize() and
     why = bufferVar and
     not memberMayBeVarSize(_, bufferVar) and
-    not exists(Union bufferType |
-      bufferType.getAMemberVariable() = why and
-      bufferVar.getUnspecifiedType().(ArrayType).getSize() <= 1
-    ) and
     not result = 0 // zero sized arrays are likely to have special usage, for example
     or
     // behaving a bit like a 'union' overlapping other fields.
@@ -85,13 +48,6 @@ int getBufferSize(Expr bufferExpr, Element why) {
       parentPtr.getTarget().getUnspecifiedType().(PointerType).getBaseType() = parentClass and
       result = getBufferSize(parentPtr, _) + bufferVar.getType().getSize() - parentClass.getSize()
     )
-    or
-    exists(Union bufferType |
-      bufferType.getAMemberVariable() = why and
-      why = bufferVar and
-      bufferVar.getUnspecifiedType().(ArrayType).getSize() <= 1 and
-      result = bufferType.getSize()
-    )
   )
   or
   // buffer is a fixed size dynamic allocation

cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

@@ -1586,6 +1586,15 @@ private module SimpleRangeAnalysisCached {
     result = min([max(getTruncatedUpperBounds(expr)), getGuardedUpperBound(expr)])
   }
 
+  /** Holds if the upper bound of `expr` may have been widened. This means the the upper bound is in practice likely to be overly wide. */
+  cached
+  predicate upperBoundMayBeWidened(Expr e) {
+    isRecursiveExpr(e) and
+    // Widening is not a problem if the post-analysis in `getGuardedUpperBound` has overridden the widening.
+    // Note that the RHS of `<` may be multi-valued.
+    not getGuardedUpperBound(e) < getTruncatedUpperBounds(e)
+  }
+
   /**
    * Holds if `expr` has a provably empty range. For example:
    *

cpp/ql/src/Critical/OverflowStatic.ql

@@ -5,7 +5,7 @@
  * @kind problem
  * @problem.severity warning
  * @security-severity 9.3
- * @precision medium
+ * @precision high
  * @id cpp/static-buffer-overflow
  * @tags reliability
  *       security
@@ -55,6 +55,8 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) {
     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
+    // The upper bounds analysis must not have been widended
+    not upperBoundMayBeWidened(bufaccess.getArrayOffset().getFullyConverted()) and
     msg =
       "Potential buffer-overflow: counter '" + loop.counter().toString() + "' <= " +
         loop.limit().toString() + " but '" + bufaccess.buffer().getName() + "' has " +
@@ -130,11 +132,13 @@ predicate outOfBounds(BufferAccess bufaccess, string msg) {
     (
       access > size
       or
-      access = size and not exists(AddressOfExpr addof | bufaccess = addof.getOperand())
+      access = size and
+      not exists(AddressOfExpr addof | bufaccess = addof.getOperand()) and
+      not exists(BuiltInOperationBuiltInOffsetOf offsetof | offsetof.getAChild() = bufaccess)
     ) and
     msg =
       "Potential buffer-overflow: '" + buf + "' has size " + size.toString() + " but '" + buf + "[" +
-        access.toString() + "]' is accessed here."
+        access.toString() + "]' may be accessed here."
   )
 }
 

cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected

@@ -5,14 +5,10 @@
 | test2.c:33:26:33:27 | 46 | Potential buffer-overflow: 'buffer' has size 40 not 46. |
 | test2.c:34:22:34:23 | 47 | Potential buffer-overflow: 'buffer' has size 40 not 47. |
 | test2.c:35:23:35:24 | 48 | Potential buffer-overflow: 'buffer' has size 40 not 48. |
-| test.c:14:9:14:13 | access to array | Potential buffer-overflow: 'xs' has size 5 but 'xs[5]' is accessed here. |
-| test.c:15:9:15:13 | access to array | Potential buffer-overflow: 'xs' has size 5 but 'xs[6]' is accessed here. |
-| test.c:20:9:20:18 | access to array | Potential buffer-overflow: 'ys' has size 5 but 'ys[5]' is accessed here. |
-| test.c:21:9:21:18 | access to array | Potential buffer-overflow: 'ys' has size 5 but 'ys[6]' is accessed here. |
-| test.c:47:3:47:18 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
-| test.c:54:3:54:26 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
-| test.c:61:3:61:18 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
-| test.c:72:3:72:11 | access to array | Potential buffer-overflow: 'buf' has size 1 but 'buf[1]' is accessed here. |
+| test.c:14:9:14:13 | access to array | Potential buffer-overflow: 'xs' has size 5 but 'xs[5]' may be accessed here. |
+| test.c:15:9:15:13 | access to array | Potential buffer-overflow: 'xs' has size 5 but 'xs[6]' may be accessed here. |
+| test.c:20:9:20:18 | access to array | Potential buffer-overflow: 'ys' has size 5 but 'ys[5]' may be accessed here. |
+| test.c:21:9:21:18 | access to array | Potential buffer-overflow: 'ys' has size 5 but 'ys[6]' may be accessed here. |
 | test.cpp:19:3:19:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer1' has 3 elements. |
 | test.cpp:20:3:20:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer2' has 3 elements. |
 | test.cpp:24:27:24:27 | 4 | Potential buffer-overflow: 'buffer1' has size 3 not 4. |

cpp/ql/test/query-tests/Critical/OverflowStatic/test.c

@@ -44,21 +44,21 @@ void union_test() {
   union u u;
   u.ptr[0] = 0; // GOOD
   u.ptr[sizeof(u)-1] = 0; // GOOD
-  u.ptr[sizeof(u)] = 0; // BAD
+  u.ptr[sizeof(u)] = 0; // BAD [NOT DETECTED]
 }
 
 void test_struct_union() {
   struct { union u u; } v;
   v.u.ptr[0] = 0; // GOOD
   v.u.ptr[sizeof(union u)-1] = 0; // GOOD
-  v.u.ptr[sizeof(union u)] = 0; // BAD
+  v.u.ptr[sizeof(union u)] = 0; // BAD [NOT DETECTED]
 }
 
 void union_test2() {
   union { char ptr[1]; unsigned long value; } u;
   u.ptr[0] = 0; // GOOD
   u.ptr[sizeof(u)-1] = 0; // GOOD
-  u.ptr[sizeof(u)] = 0; // BAD
+  u.ptr[sizeof(u)] = 0; // BAD [NOT DETECTED]
 }
 
 typedef struct {
@@ -69,5 +69,5 @@ typedef struct {
 void test_alloc() {
   // Special case of taking sizeof without any addition or multiplications
   var_buf *b = malloc(sizeof(var_buf));
-  b->buf[1] = 0; // BAD
+  b->buf[1] = 0; // BAD [NOT DETECTED]
 }

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected

@@ -72,12 +72,9 @@
 | 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 |
 | unions.cpp:34:2:34:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:34:14:34:18 | large | destination buffer |
-| var_size_struct.cpp:54:5:54:14 | access to array | This array indexing operation accesses byte offset 1 but the $@ is only 1 byte. | var_size_struct.cpp:32:8:32:10 | str | array |
-| var_size_struct.cpp:55:5:55:14 | access to array | This array indexing operation accesses byte offset 1 but the $@ is only 1 byte. | var_size_struct.cpp:38:8:38:10 | str | 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 |
-| var_size_struct.cpp:169:3:169:8 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 1 byte. | var_size_struct.cpp:125:17:125:19 | arr | destination buffer |

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected

@@ -3,8 +3,6 @@
 | tests.cpp:163:3:163:11 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |
 | tests.cpp:164:8:164:16 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |
 | 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. |
-| 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. |
+| 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. |
 | var_size_struct.cpp:103:39:103:41 | 129 | Potential buffer-overflow: 'str' has size 128 not 129. |

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/var_size_struct.cpp

@@ -51,8 +51,8 @@ void testVarString(int n) {
     s1->str[1] = '?'; // GOOD
     s2->str[1] = '?'; // GOOD
     s3->str[1] = '?'; // GOOD
-    s4->str[1] = '?'; // BAD
-    s5->str[1] = '?'; // BAD
+    s4->str[1] = '?'; // BAD [NOT DETECTED]
+    s5->str[1] = '?'; // BAD [NOT DETECTED]
   }
 }
 
@@ -166,7 +166,7 @@ void useVarStruct34(varStruct5 *vs5) {
 
 void testVarStruct34(varStruct3 *vs3, varStruct4 *vs4, varStruct5 *vs5, varStruct6 *vs6, varStruct7 *vs7, varStruct8 *vs8, varStruct9 *vs9) {
   memset(vs3->arr, 'x', 100); // GOOD: it's variable size, we don't know how big so shouldn't flag
-  memset(vs4->arr, 'x', 100); // BAD: it's not variable size, so this is a buffer overflow
+  memset(vs4->arr, 'x', 100); // BAD: [NOT DETECTED] it's not variable size, so this is a buffer overflow
   memset(vs5->arr, 'x', 100); // GOOD: it's variable size, we don't know how big so shouldn't flag
   memset(vs6->arr, 'x', 100); // GOOD: it's variable size, we don't know how big so shouldn't flag
   memset(vs7->arr, 'x', 100); // GOOD: it's variable size, we don't know how big so shouldn't flag

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/varsize.expected

@@ -1,12 +1,17 @@
 | var_size_struct.cpp:13:8:13:17 | VarString1 | var_size_struct.cpp:15:8:15:10 | str |
 | var_size_struct.cpp:18:8:18:17 | VarString2 | var_size_struct.cpp:20:8:20:10 | str |
 | var_size_struct.cpp:24:8:24:17 | VarString3 | var_size_struct.cpp:26:8:26:10 | str |
+| var_size_struct.cpp:30:8:30:17 | VarString4 | var_size_struct.cpp:32:8:32:10 | str |
+| var_size_struct.cpp:36:8:36:17 | VarString5 | var_size_struct.cpp:38:8:38:10 | str |
 | var_size_struct.cpp:36:8:36:17 | VarString5 | var_size_struct.cpp:39:8:39:11 | str2 |
 | var_size_struct.cpp:61:8:61:17 | varStruct1 | var_size_struct.cpp:63:8:63:11 | data |
 | var_size_struct.cpp:76:8:76:17 | varStruct2 | var_size_struct.cpp:78:7:78:14 | elements |
+| var_size_struct.cpp:106:8:106:20 | notVarStruct2 | var_size_struct.cpp:107:8:107:10 | str |
 | var_size_struct.cpp:119:8:119:17 | varStruct3 | var_size_struct.cpp:121:17:121:19 | arr |
+| var_size_struct.cpp:123:8:123:17 | varStruct4 | var_size_struct.cpp:125:17:125:19 | arr |
 | var_size_struct.cpp:127:8:127:17 | varStruct5 | var_size_struct.cpp:129:17:129:19 | arr |
 | var_size_struct.cpp:131:8:131:17 | varStruct6 | var_size_struct.cpp:133:17:133:19 | arr |
 | var_size_struct.cpp:135:8:135:17 | varStruct7 | var_size_struct.cpp:137:17:137:19 | arr |
 | var_size_struct.cpp:139:8:139:17 | varStruct8 | var_size_struct.cpp:141:9:141:11 | arr |
 | var_size_struct.cpp:143:8:143:17 | varStruct9 | var_size_struct.cpp:145:17:145:19 | arr |
+| var_size_struct.cpp:181:8:181:18 | PseudoUnion | var_size_struct.cpp:183:7:183:10 | data |