Merge pull request #6679 from andersfugmann/relax_memberMayBeVarSize

Improve precision on OverflowStatic query.

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

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* The `memberMayBeVarSize` predicate considers more fields to be variable size.
+  As a result, the "Static buffer overflow" query (cpp/static-buffer-overflow)
+  produces fewer false positives.

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

@@ -2,17 +2,18 @@ import cpp
 import semmle.code.cpp.dataflow.DataFlow
 
 /**
- * Holds if `v` is a member variable of `c` that looks like it might be variable sized in practice.  For
- * example:
+ * Holds if `v` is a member variable of `c` that looks like it might be variable sized
+ * in practice. For example:
  * ```
  * struct myStruct { // c
  *   int amount;
  *   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,
- * 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
+ * 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))
  * ```
@@ -27,27 +28,25 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
     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
+    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 |
-      // `sizeof(c)` is taken
       so.(SizeofTypeOperator).getTypeOperand().getUnspecifiedType() = c or
       so.(SizeofExprOperator).getExprOperand().getUnspecifiedType() = c
     |
-      // arithmetic is performed on the result
+      // and arithmetic is performed on the result
       so.getParent*() instanceof AddExpr
     )
-    or
-    exists(AddressOfExpr aoe |
-      // `&(c.v)` is taken
-      aoe.getAddressable() = v
-    )
-    or
-    exists(BuiltInOperationBuiltInOffsetOf oo |
-      // `offsetof(c, v)` using a builtin
-      oo.getAChild().(VariableAccess).getTarget() = v
-    )
   )
 }
 
@@ -61,6 +60,10 @@ 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.
@@ -82,6 +85,13 @@ 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/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected

@@ -9,6 +9,10 @@
 | 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.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

@@ -27,3 +27,47 @@ void f(void) {
     c = stru.zs[6]; // GOOD (zs is variable size)
 }
 
+void* malloc(long unsigned int);
+void test_buffer_sentinal() {
+  struct { char len; char buf[1]; } *b = malloc(10); // len(buf.buffer) effectively 8
+  b->buf[0] = 0; // GOOD
+  b->buf[7] = 0; // GOOD
+  b->buf[8] = 0; // BAD [NOT DETECTED]
+}
+
+union u {
+  unsigned long value;
+  char ptr[1];
+};
+
+void union_test() {
+  union u u;
+  u.ptr[0] = 0; // GOOD
+  u.ptr[sizeof(u)-1] = 0; // GOOD
+  u.ptr[sizeof(u)] = 0; // BAD
+}
+
+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
+}
+
+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
+}
+
+typedef struct {
+    char len;
+    char buf[1];
+} var_buf;
+
+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
+}

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

@@ -80,4 +80,4 @@
 | 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:171:3:171: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 |
+| 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/var_size_struct.cpp

@@ -161,8 +161,6 @@ void useVarStruct34(varStruct5 *vs5) {
   varStruct5 *vs5b = (varStruct5 *)malloc(sizeof(*vs5));
   varStruct6 *vs6 = (varStruct6 *)malloc(offsetof(varStruct6, arr) + 9); // establish varStruct6 as variable size
   varStruct7 *vs7 = (varStruct7 *)malloc(sizeForVarStruct7(9)); // establish varStruct7 as variable size
-  varStruct8 *vs8a = (varStruct8 *)malloc(sizeof(varStruct8) + 9); // establish varStruct8 as variable size
-  varStruct8 *vs8b = (varStruct8 *)malloc(sizeof(varStruct8));
   varStruct9 *vs9 = (varStruct9 *)malloc(__builtin_offsetof(varStruct9, arr) + 9); // establish varStruct9 as variable size
 }