Merge pull request #17384 from microsoft/brodes/overflow-buffer-fixes…

…-upstream

Brodes/overflow buffer fixes upstream

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

@@ -57,7 +57,7 @@ private int isSource(Expr bufferExpr, Element why) {
   exists(Type bufferType |
     // buffer is the address of a variable
     why = bufferExpr.(AddressOfExpr).getAddressable() and
-    bufferType = why.(Variable).getType() and
+    bufferType = why.(Variable).getUnspecifiedType() and
     result = bufferType.getSize() and
     not bufferType instanceof ReferenceType and
     not any(Union u).getAMemberVariable() = why

cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll

@@ -14,7 +14,11 @@ int getPointedSize(Type t) {
  * BufferWrite differ.
  */
 abstract class BufferAccess extends Expr {
-  BufferAccess() { not this.isUnevaluated() }
+  BufferAccess() {
+    not this.isUnevaluated() and
+    //A buffer access must be reachable (not in dead code)
+    reachable(this)
+  }
 
   abstract string getName();
 
@@ -26,6 +30,8 @@ abstract class BufferAccess extends Expr {
    * - 1 = buffer range [0, getSize) is accessed entirely.
    * - 2 = buffer range [0, getSize) may be accessed partially or entirely.
    * - 3 = buffer is accessed at offset getSize - 1.
+   * - 4 = buffer is accessed with null terminator read protections
+   *       (does not read past null terminator, regardless of access size)
    */
   abstract Expr getBuffer(string bufferDesc, int accessType);
 
@@ -128,7 +134,7 @@ class StrncpyBA extends BufferAccess {
     or
     result = this.(FunctionCall).getArgument(1) and
     bufferDesc = "source buffer" and
-    accessType = 2
+    accessType = 4
   }
 
   override Expr getSizeExpr() { result = this.(FunctionCall).getArgument(2) }

cpp/ql/src/Critical/OverflowStatic.ql

@@ -42,7 +42,9 @@ class BufferAccess extends ArrayExpr {
     not exists(Macro m |
       m.getName() = "strcmp" and
       m.getAnInvocation().getAnExpandedElement() = this
-    )
+    ) and
+    //A buffer access must be reachable (not in dead code)
+    reachable(this)
   }
 
   int bufferSize() { staticBuffer(this.getArrayBase(), _, result) }

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

@@ -26,6 +26,7 @@ from
   BufferAccess ba, string bufferDesc, int accessSize, int accessType, Element bufferAlloc,
   int bufferSize, string message
 where
+  accessType != 4 and
   accessSize = ba.getSize() and
   bufferSize = getBufferSize(ba.getBuffer(bufferDesc, accessType), bufferAlloc) and
   (

cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Removed false positives caused by buffer accesses in unreachable code
+* Removed false positives caused by inconsistent type checking

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

@@ -31,9 +31,9 @@ edges
 | main.cpp:9:29:9:32 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance |  |
 | main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | **argv | provenance |  |
 | main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | *argv | provenance |  |
-| main.cpp:10:20:10:23 | **argv | tests.cpp:657:32:657:35 | **argv | provenance |  |
-| main.cpp:10:20:10:23 | *argv | tests.cpp:657:32:657:35 | **argv | provenance |  |
-| main.cpp:10:20:10:23 | *argv | tests.cpp:657:32:657:35 | *argv | provenance |  |
+| main.cpp:10:20:10:23 | **argv | tests.cpp:689:32:689:35 | **argv | provenance |  |
+| main.cpp:10:20:10:23 | *argv | tests.cpp:689:32:689:35 | **argv | provenance |  |
+| main.cpp:10:20:10:23 | *argv | tests.cpp:689:32:689:35 | *argv | provenance |  |
 | overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | **argv | provenance |  |
 | overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | *argv | provenance |  |
 | test_buffer_overrun.cpp:32:46:32:49 | **argv | test_buffer_overrun.cpp:32:46:32:49 | **argv | provenance |  |
@@ -46,12 +46,12 @@ edges
 | tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:14:628:19 | *home | provenance |  |
 | tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:16:628:19 | *home | provenance |  |
 | tests.cpp:628:16:628:19 | *home | tests.cpp:628:14:628:19 | *home | provenance |  |
-| tests.cpp:657:32:657:35 | **argv | tests.cpp:682:9:682:15 | *access to array | provenance |  |
-| tests.cpp:657:32:657:35 | **argv | tests.cpp:683:9:683:15 | *access to array | provenance |  |
-| tests.cpp:657:32:657:35 | *argv | tests.cpp:682:9:682:15 | *access to array | provenance |  |
-| tests.cpp:657:32:657:35 | *argv | tests.cpp:683:9:683:15 | *access to array | provenance |  |
-| tests.cpp:682:9:682:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance |  |
-| tests.cpp:683:9:683:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance |  |
+| tests.cpp:689:32:689:35 | **argv | tests.cpp:714:9:714:15 | *access to array | provenance |  |
+| tests.cpp:689:32:689:35 | **argv | tests.cpp:715:9:715:15 | *access to array | provenance |  |
+| tests.cpp:689:32:689:35 | *argv | tests.cpp:714:9:714:15 | *access to array | provenance |  |
+| tests.cpp:689:32:689:35 | *argv | tests.cpp:715:9:715:15 | *access to array | provenance |  |
+| tests.cpp:714:9:714:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance |  |
+| tests.cpp:715:9:715:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance |  |
 | tests_restrict.c:15:41:15:44 | **argv | tests_restrict.c:15:41:15:44 | **argv | provenance |  |
 | tests_restrict.c:15:41:15:44 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance |  |
 nodes
@@ -85,10 +85,10 @@ nodes
 | tests.cpp:628:14:628:14 | *s [*home] | semmle.label | *s [*home] |
 | tests.cpp:628:14:628:19 | *home | semmle.label | *home |
 | tests.cpp:628:16:628:19 | *home | semmle.label | *home |
-| tests.cpp:657:32:657:35 | **argv | semmle.label | **argv |
-| tests.cpp:657:32:657:35 | *argv | semmle.label | *argv |
-| tests.cpp:682:9:682:15 | *access to array | semmle.label | *access to array |
-| tests.cpp:683:9:683:15 | *access to array | semmle.label | *access to array |
+| tests.cpp:689:32:689:35 | **argv | semmle.label | **argv |
+| tests.cpp:689:32:689:35 | *argv | semmle.label | *argv |
+| tests.cpp:714:9:714:15 | *access to array | semmle.label | *access to array |
+| tests.cpp:715:9:715:15 | *access to array | semmle.label | *access to array |
 | tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
 | tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
 | tests_restrict.c:15:41:15:44 | *argv | semmle.label | *argv |

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

@@ -654,6 +654,38 @@ void test26(bool cond)
 	if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1]
 }
 
+#define IND 100
+#define MAX_SIZE 100
+void test27(){
+	char *src = "";
+	char *dest = "abcdefgh";
+	int ind = 100;
+	char buffer[MAX_SIZE];
+
+	strncpy(dest, src, 8); // GOOD, strncpy will not read past null terminator of source
+
+	if(IND < MAX_SIZE){
+		buffer[IND] = 0; // GOOD: out of bounds, but inaccessible code
+	}
+}
+
+typedef struct _MYSTRUCT {
+    unsigned long  a;
+    unsigned short b;
+    unsigned char  z[ 100 ];
+} MYSTRUCT;
+
+
+const MYSTRUCT _myStruct = { 0 };
+typedef const MYSTRUCT& MYSTRUCTREF;
+
+// False positive case due to use of typedefs
+int test28(MYSTRUCTREF g)
+{
+	return memcmp(&g, &_myStruct, sizeof(MYSTRUCT)); // GOOD
+}
+
+
 int tests_main(int argc, char *argv[])
 {
 	long long arr17[19];