Merge pull request #18837 from geoffw0/overflowbuffer

C++: Improve and promote cpp/overflow-buffer

Author: geoffw0

cpp/ql/lib/change-notes/2025-02-20-getbuffersize.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed an issue where the `getBufferSize` predicate in `commons/Buffer.qll` was returning results for references inside `offsetof` expressions, which are not accesses to a buffer.

cpp/ql/lib/change-notes/2025-02-25-getbuffersize.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Modified the `getBufferSize` predicate in `commons/Buffer.qll` to be more tolerant in some cases involving member variables in a larger struct or class.

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

@@ -71,7 +71,7 @@ private int getSize(VariableAccess va) {
       result = t.getSize()
     )
     or
-    exists(Class c |
+    exists(Class c, int trueSize |
       // Otherwise, we find the "outermost" object and compute the size
       // as the difference between the size of the type of the "outermost
       // object" and the offset of the field relative to that type.
@@ -91,7 +91,9 @@ private int getSize(VariableAccess va) {
       // of `y` relative to the type `S2` (i.e., `4`). So the size of the
       // buffer is `12 - 4 = 8`.
       c = getRootType(va) and
-      result = c.getSize() - v.(Field).getOffsetInClass(c)
+      // we calculate the size based on the last field, to avoid including any padding after it
+      trueSize = max(Field f | | f.getOffsetInClass(c) + f.getUnspecifiedType().getSize()) and
+      result = trueSize - v.(Field).getOffsetInClass(c)
     )
   )
 }
@@ -105,9 +107,16 @@ private int getSize(VariableAccess va) {
 private int isSource(Expr bufferExpr, Element why) {
   exists(Variable bufferVar | bufferVar = bufferExpr.(VariableAccess).getTarget() |
     // buffer is a fixed size array
-    result = bufferVar.getUnspecifiedType().(ArrayType).getSize() and
+    exists(bufferVar.getUnspecifiedType().(ArrayType).getSize()) and
+    result =
+      unique(int size | // more generous than .getSize() itself, when the array is a class field or similar.
+        size = getSize(bufferExpr)
+      |
+        size
+      ) and
     why = bufferVar and
     not memberMayBeVarSize(_, bufferVar) and
+    not exists(BuiltInOperationBuiltInOffsetOf offsetof | offsetof.getAChild*() = bufferExpr) and
     // zero sized arrays are likely to have special usage, for example
     // behaving a bit like a 'union' overlapping other fields.
     not result = 0

cpp/ql/src/Security/CWE/CWE-119/OverflowBuffer.ql

@@ -5,8 +5,9 @@
  *              buffer.
  * @kind problem
  * @id cpp/overflow-buffer
- * @problem.severity recommendation
+ * @problem.severity warning
  * @security-severity 9.3
+ * @precision medium
  * @tags security
  *       external/cwe/cwe-119
  *       external/cwe/cwe-121

cpp/ql/src/change-notes/2025-02-20-overflow-buffer.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query "Call to memory access function may overflow buffer" (`cpp/overflow-buffer`) has been added to the security-extended query suite. The query detects a range of buffer overflow and underflow issues.

cpp/ql/src/change-notes/2025-02-27-static-buffer-overflow.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Due to changes in libraries the query "Static array access may cause overflow" (`cpp/static-buffer-overflow`) will no longer report cases where multiple fields of a struct or class are written with a single `memset` or similar operation.

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

@@ -1,5 +1,3 @@
-| tests.cpp:45:9:45:14 | call to memcpy | This 'memcpy' operation accesses 32 bytes but the $@ is only 16 bytes. | tests.cpp:32:10:32:18 | charFirst | destination buffer |
-| tests.cpp:60:9:60:14 | call to memcpy | This 'memcpy' operation accesses 32 bytes but the $@ is only 16 bytes. | tests.cpp:32:10:32:18 | charFirst | destination buffer |
 | tests.cpp:171:9:171:14 | call to memcpy | This 'memcpy' operation accesses 100 bytes but the $@ is only 50 bytes. | tests.cpp:164:20:164:25 | call to malloc | destination buffer |
 | tests.cpp:172:9:172:19 | access to array | This array indexing operation accesses byte offset 99 but the $@ is only 50 bytes. | tests.cpp:164:20:164:25 | call to malloc | array |
 | tests.cpp:192:9:192:14 | call to memcpy | This 'memcpy' operation accesses 100 bytes but the $@ is only 50 bytes. | tests.cpp:181:10:181:22 | dataBadBuffer | destination buffer |

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

@@ -1,2 +0,0 @@
-| tests.cpp:45:51:45:72 | sizeof(<expr>) | Potential buffer-overflow: 'charFirst' has size 16 not 32. |
-| tests.cpp:60:52:60:74 | sizeof(<expr>) | Potential buffer-overflow: 'charFirst' has size 16 not 32. |

cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/tests.cpp

@@ -42,7 +42,7 @@ void CWE121_Stack_Based_Buffer_Overflow__char_type_overrun_memcpy_01_bad()
         /* Print the initial block pointed to by structCharVoid.voidSecond */
         printLine((char *)structCharVoid.voidSecond);
         /* FLAW: Use the sizeof(structCharVoid) which will overwrite the pointer voidSecond */
-        memcpy(structCharVoid.charFirst, SRC_STR, sizeof(structCharVoid));
+        memcpy(structCharVoid.charFirst, SRC_STR, sizeof(structCharVoid)); // [NOT DETECTED]
         structCharVoid.charFirst[(sizeof(structCharVoid.charFirst)/sizeof(char))-1] = '\0'; /* null terminate the string */
         printLine((char *)structCharVoid.charFirst);
         printLine((char *)structCharVoid.voidSecond);
@@ -57,7 +57,7 @@ void CWE122_Heap_Based_Buffer_Overflow__char_type_overrun_memcpy_01_bad()
         /* Print the initial block pointed to by structCharVoid->voidSecond */
         printLine((char *)structCharVoid->voidSecond);
         /* FLAW: Use the sizeof(*structCharVoid) which will overwrite the pointer y */
-        memcpy(structCharVoid->charFirst, SRC_STR, sizeof(*structCharVoid));
+        memcpy(structCharVoid->charFirst, SRC_STR, sizeof(*structCharVoid)); // [NOT DETECTED]
         structCharVoid->charFirst[(sizeof(structCharVoid->charFirst)/sizeof(char))-1] = '\0'; /* null terminate the string */
         printLine((char *)structCharVoid->charFirst);
         printLine((char *)structCharVoid->voidSecond);
@@ -292,7 +292,7 @@ namespace CWE122_Heap_Based_Buffer_Overflow__cpp_CWE193_wchar_t_ncpy_01
             delete [] data;
         }
     }
-    
+
     static void goodG2B()
 	{
 		wchar_t * data;
@@ -459,7 +459,7 @@ void CWE122_Heap_Based_Buffer_Overflow__cpp_CWE805_wchar_t_ncpy_01_bad()
 #ifdef _WIN32
 int _snwprintf(wchar_t *buffer, size_t count, const wchar_t *format, ...);
 #define SNPRINTF _snwprintf
-#else 
+#else
 int snprintf(char *s, size_t n, const char *format, ...);
 int swprintf(wchar_t *wcs, size_t maxlen, const wchar_t *format, ...);
 //#define SNPRINTF snprintf --- original code; using snprintf appears to be a mistake in samate?
@@ -485,14 +485,14 @@ void CWE122_Heap_Based_Buffer_Overflow__cpp_CWE805_wchar_t_snprintf_01_bad()
 }
 
 /* classes used in some test cases as a custom type */
-class TwoIntsClass 
+class TwoIntsClass
 {
     public: // Needed to access variables from label files
         int intOne;
         int intTwo;
 };
 
-class OneIntClass 
+class OneIntClass
 {
     public: // Needed to access variables from label files
         int intOne;
@@ -636,7 +636,7 @@ void CWE122_Heap_Based_Buffer_Overflow__cpp_CWE805_wchar_t_snprintf_31_bad()
 
 int rand(void);
 
-int globalReturnsTrueOrFalse() 
+int globalReturnsTrueOrFalse()
 {
     return (rand() % 2);
 }

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

@@ -1,2 +1,4 @@
+| tests.cpp:1055:2:1055:8 | call to strncpy | This 'call to strncpy' operation is limited to 131 bytes but the destination is only 128 bytes. |
+| tests.cpp:1057:2:1057:8 | call to strncpy | This 'call to strncpy' operation is limited to 131 bytes but the destination is only 64 bytes. |
 | var_size_struct.cpp:73:3:73:9 | call to strncpy | This 'call to strncpy' operation is limited to 1025 bytes but the destination is only 1024 bytes. |
 | var_size_struct.cpp:103:3:103:9 | call to strncpy | This 'call to strncpy' operation is limited to 129 bytes but the destination is only 128 bytes. |