Merge branch 'main' into add-cwe-208

Author: boveus

cpp/ql/src/Security/CWE/CWE-120/BadlyBoundedWrite.ql

@@ -24,7 +24,7 @@ import semmle.code.cpp.security.BufferWrite
 from BufferWrite bw, int destSize
 where
   bw.hasExplicitLimit() and // has an explicit size limit
-  destSize = getBufferSize(bw.getDest(), _) and
+  destSize = max(getBufferSize(bw.getDest(), _)) and
   bw.getExplicitLimit() > destSize // but it's larger than the destination
 select bw,
   "This '" + bw.getBWDesc() + "' operation is limited to " + bw.getExplicitLimit() +

cpp/ql/src/change-notes/2023-08-09-badly-bounded-write.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/badly-bounded-write` query could report false positives when a pointer was first initialized with a literal and later assigned a dynamically allocated array. These false positives now no longer occur.

cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected

@@ -1,10 +1,10 @@
-| tests2.cpp:17:3:17:8 | call to wcscpy | This 'call to wcscpy' operation requires 12 bytes but the destination is only 8 bytes. |
-| tests2.cpp:22:3:22:8 | call to wcscpy | This 'call to wcscpy' operation requires 16 bytes but the destination is only 12 bytes. |
-| tests2.cpp:27:3:27:8 | call to wcscpy | This 'call to wcscpy' operation requires 20 bytes but the destination is only 16 bytes. |
-| tests2.cpp:31:3:31:8 | call to wcscpy | This 'call to wcscpy' operation requires 24 bytes but the destination is only 20 bytes. |
-| tests2.cpp:36:3:36:8 | call to wcscpy | This 'call to wcscpy' operation requires 28 bytes but the destination is only 24 bytes. |
-| tests2.cpp:41:3:41:8 | call to wcscpy | This 'call to wcscpy' operation requires 32 bytes but the destination is only 28 bytes. |
-| tests2.cpp:46:3:46:8 | call to wcscpy | This 'call to wcscpy' operation requires 36 bytes but the destination is only 32 bytes. |
+| tests2.cpp:18:3:18:8 | call to wcscpy | This 'call to wcscpy' operation requires 12 bytes but the destination is only 8 bytes. |
+| tests2.cpp:23:3:23:8 | call to wcscpy | This 'call to wcscpy' operation requires 16 bytes but the destination is only 12 bytes. |
+| tests2.cpp:28:3:28:8 | call to wcscpy | This 'call to wcscpy' operation requires 20 bytes but the destination is only 16 bytes. |
+| tests2.cpp:32:3:32:8 | call to wcscpy | This 'call to wcscpy' operation requires 24 bytes but the destination is only 20 bytes. |
+| tests2.cpp:37:3:37:8 | call to wcscpy | This 'call to wcscpy' operation requires 28 bytes but the destination is only 24 bytes. |
+| tests2.cpp:42:3:42:8 | call to wcscpy | This 'call to wcscpy' operation requires 32 bytes but the destination is only 28 bytes. |
+| tests2.cpp:47:3:47:8 | call to wcscpy | This 'call to wcscpy' operation requires 36 bytes but the destination is only 32 bytes. |
 | tests.c:54:3:54:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. |
 | tests.c:58:3:58:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. |
 | tests.c:62:17:62:24 | buffer10 | This 'scanf string argument' operation requires 11 bytes but the destination is only 10 bytes. |

cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests2.cpp

@@ -6,6 +6,7 @@ void *realloc(void *ptr, size_t size);
 void *calloc(size_t nmemb, size_t size); 
 void free(void *ptr);
 wchar_t *wcscpy(wchar_t *s1, const wchar_t *s2); 
+int snprintf(char *s, size_t n, const char *format, ...);
 
 // --- Semmle tests ---
 
@@ -46,3 +47,18 @@ void tests2() {
   wcscpy(buffer, L"12345678"); // BAD: buffer overflow
   delete [] buffer;
 }
+
+char* dest1 = "a";
+char* dest2 = "abcdefghijklmnopqrstuvwxyz";
+
+void test3() {
+  const char src[] = "abcdefghijkl";
+  dest1 = (char*)malloc(sizeof(src));
+  if (!dest1)
+    return;
+  snprintf(dest1, sizeof(src), "%s", src); // GOOD
+  dest2 = (char*)malloc(3);
+  if (!dest2)
+    return;
+  snprintf(dest2, sizeof(src), "%s", src); // BAD [NOT DETECTED]: buffer overflow
+}

csharp/ql/lib/Linq/Helpers.qll

@@ -3,7 +3,9 @@
  * Provides helper classes and methods related to LINQ.
  */
 
-import csharp
+private import csharp
+private import semmle.code.csharp.frameworks.system.collections.Generic as GenericCollections
+private import semmle.code.csharp.frameworks.system.Collections as Collections
 
 //#################### PREDICATES ####################
 private Stmt firstStmt(ForeachStmt fes) {
@@ -29,13 +31,40 @@ predicate isIEnumerableType(ValueOrRefType t) {
   )
 }
 
+/**
+ * A class of foreach statements where the iterable expression
+ * supports the use of the LINQ extension methods on `IEnumerable<T>`.
+ */
+class ForeachStmtGenericEnumerable extends ForeachStmt {
+  ForeachStmtGenericEnumerable() {
+    exists(ValueOrRefType t | t = this.getIterableExpr().getType() |
+      t.getABaseType*().getUnboundDeclaration() instanceof
+        GenericCollections::SystemCollectionsGenericIEnumerableTInterface or
+      t.(ArrayType).getRank() = 1
+    )
+  }
+}
+
+/**
+ * A class of foreach statements where the iterable expression
+ * supports the use of the LINQ extension methods on `IEnumerable`.
+ */
+class ForeachStmtEnumerable extends ForeachStmt {
+  ForeachStmtEnumerable() {
+    exists(ValueOrRefType t | t = this.getIterableExpr().getType() |
+      t.getABaseType*() instanceof Collections::SystemCollectionsIEnumerableInterface or
+      t.(ArrayType).getRank() = 1
+    )
+  }
+}
+
 /**
  * Holds if `foreach` statement `fes` could be converted to a `.All()` call.
  * That is, the `ForeachStmt` contains a single `if` with a condition that
  * accesses the loop variable and with a body that assigns `false` to a variable
  * and `break`s out of the `foreach`.
  */
-predicate missedAllOpportunity(ForeachStmt fes) {
+predicate missedAllOpportunity(ForeachStmtGenericEnumerable fes) {
   exists(IfStmt is |
     // The loop contains an if statement with no else case, and nothing else.
     is = firstStmt(fes) and
@@ -54,12 +83,12 @@ predicate missedAllOpportunity(ForeachStmt fes) {
 }
 
 /**
- * Holds if `foreach` statement `fes` could be converted to a `.Cast()` call.
+ * Holds if the `foreach` statement `fes` can be converted to a `.Cast()` call.
  * That is, the loop variable is accessed only in the first statement of the
- * block, and the access is a cast. The first statement needs to be a
- * `LocalVariableDeclStmt`.
+ * block, the access is a cast, and the first statement is a
+ * local variable declaration statement `s`.
  */
-predicate missedCastOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
+predicate missedCastOpportunity(ForeachStmtEnumerable fes, LocalVariableDeclStmt s) {
   s = firstStmt(fes) and
   forex(VariableAccess va | va = fes.getVariable().getAnAccess() |
     va = s.getAVariableDeclExpr().getAChildExpr*()
@@ -71,12 +100,12 @@ predicate missedCastOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
 }
 
 /**
- * Holds if `foreach` statement `fes` could be converted to an `.OfType()` call.
+ * Holds if `foreach` statement `fes` can be converted to an `.OfType()` call.
  * That is, the loop variable is accessed only in the first statement of the
- * block, and the access is a cast with the `as` operator. The first statement
- * needs to be a `LocalVariableDeclStmt`.
+ * block, the access is a cast with the `as` operator, and the first statement
+ * is a local variable declaration statement `s`.
  */
-predicate missedOfTypeOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
+predicate missedOfTypeOpportunity(ForeachStmtEnumerable fes, LocalVariableDeclStmt s) {
   s = firstStmt(fes) and
   forex(VariableAccess va | va = fes.getVariable().getAnAccess() |
     va = s.getAVariableDeclExpr().getAChildExpr*()
@@ -88,12 +117,12 @@ predicate missedOfTypeOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
 }
 
 /**
- * Holds if `foreach` statement `fes` could be converted to a `.Select()` call.
+ * Holds if `foreach` statement `fes` can be converted to a `.Select()` call.
  * That is, the loop variable is accessed only in the first statement of the
- * block, and the access is not a cast. The first statement needs to be a
- * `LocalVariableDeclStmt`.
+ * block, the access is not a cast, and the first statement is a
+ * local variable declaration statement `s`.
  */
-predicate missedSelectOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
+predicate missedSelectOpportunity(ForeachStmtGenericEnumerable fes, LocalVariableDeclStmt s) {
   s = firstStmt(fes) and
   forex(VariableAccess va | va = fes.getVariable().getAnAccess() |
     va = s.getAVariableDeclExpr().getAChildExpr*()
@@ -107,7 +136,7 @@ predicate missedSelectOpportunity(ForeachStmt fes, LocalVariableDeclStmt s) {
  * variable, and the body of the `if` is either a `continue` or there's nothing
  * else in the loop than the `if`.
  */
-predicate missedWhereOpportunity(ForeachStmt fes, IfStmt is) {
+predicate missedWhereOpportunity(ForeachStmtGenericEnumerable fes, IfStmt is) {
   // The very first thing the foreach loop does is test its iteration variable.
   is = firstStmt(fes) and
   exists(VariableAccess va |

csharp/ql/lib/semmle/code/csharp/Stmt.qll

@@ -861,6 +861,12 @@ class YieldReturnStmt extends YieldStmt {
   override string getAPrimaryQlClass() { result = "YieldReturnStmt" }
 }
 
+bindingset[cfe1, cfe2]
+pragma[inline_late]
+private predicate sameCallable(ControlFlowElement cfe1, ControlFlowElement cfe2) {
+  cfe1.getEnclosingCallable() = cfe2.getEnclosingCallable()
+}
+
 /**
  * A `try` statement, for example
  *
@@ -947,8 +953,7 @@ class TryStmt extends Stmt, @try_stmt {
       mid = this.getATriedElement() and
       not mid instanceof TryStmt and
       result = mid.getAChild() and
-      pragma[only_bind_into](mid.getEnclosingCallable()) =
-        pragma[only_bind_into](result.getEnclosingCallable())
+      sameCallable(mid, result)
     )
   }
 }

csharp/ql/src/Linq/MissedAllOpportunity.ql

@@ -10,7 +10,6 @@
  *       language-features
  */
 
-import csharp
 import Linq.Helpers
 
 /*
@@ -31,7 +30,7 @@ import Linq.Helpers
  * bool allEven = lst.All(i => i % 2 == 0);
  */
 
-from ForeachStmt fes
+from ForeachStmtGenericEnumerable fes
 where missedAllOpportunity(fes)
 select fes,
   "This foreach loop looks as if it might be testing whether every sequence element satisfies a predicate - consider using '.All(...)'."

csharp/ql/src/Linq/MissedCastOpportunity.ql

@@ -13,7 +13,7 @@
 import csharp
 import Linq.Helpers
 
-from ForeachStmt fes, LocalVariableDeclStmt s
+from ForeachStmtEnumerable fes, LocalVariableDeclStmt s
 where missedCastOpportunity(fes, s)
 select fes,
   "This foreach loop immediately $@ - consider casting the sequence explicitly using '.Cast(...)'.",

csharp/ql/src/Linq/MissedOfTypeOpportunity.ql

@@ -13,7 +13,7 @@
 import csharp
 import Linq.Helpers
 
-from ForeachStmt fes, LocalVariableDeclStmt s
+from ForeachStmtEnumerable fes, LocalVariableDeclStmt s
 where missedOfTypeOpportunity(fes, s)
 select fes,
   "This foreach loop immediately uses 'as' to $@ - consider using '.OfType(...)' instead.", s,

csharp/ql/src/Linq/MissedSelectOpportunity.ql

@@ -20,7 +20,7 @@ predicate oversized(LocalVariableDeclStmt s) {
   )
 }
 
-from ForeachStmt fes, LocalVariableDeclStmt s
+from ForeachStmtGenericEnumerable fes, LocalVariableDeclStmt s
 where
   missedSelectOpportunity(fes, s) and
   not oversized(s)