Merge branch 'main' into go/mad/convert-sinks

Author: owen-mc

cpp/ql/lib/change-notes/2024-07-25-alias-analysis-perf.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Improved performance of alias analysis of large function bodies. In rare cases, alerts that depend on alias analysis of large function bodies may be affected.

cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowPrivate.qll

@@ -215,19 +215,16 @@ predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { none() }
 predicate localMustFlowStep(Node node1, Node node2) { none() }
 
 /** Gets the type of `n` used for type pruning. */
-Type getNodeType(Node n) {
+DataFlowType getNodeType(Node n) {
   exists(n) and
   result instanceof VoidType // stub implementation
 }
 
-/** Gets a string representation of a type returned by `getNodeType`. */
-string ppReprType(Type t) { none() } // stub implementation
-
 /**
  * Holds if `t1` and `t2` are compatible, that is, whether data can flow from
  * a node of type `t1` to a node of type `t2`.
  */
-predicate compatibleTypes(Type t1, Type t2) {
+predicate compatibleTypes(DataFlowType t1, DataFlowType t2) {
   t1 instanceof VoidType and t2 instanceof VoidType // stub implementation
 }
 
@@ -243,7 +240,11 @@ class DataFlowCallable extends Function { }
 
 class DataFlowExpr = Expr;
 
-class DataFlowType = Type;
+final private class TypeFinal = Type;
+
+class DataFlowType extends TypeFinal {
+  string toString() { result = "" }
+}
 
 /** A function call relevant for data flow. */
 class DataFlowCall extends Expr instanceof Call {

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

@@ -994,9 +994,6 @@ DataFlowType getNodeType(Node n) {
   result instanceof VoidType // stub implementation
 }
 
-/** Gets a string representation of a type returned by `getNodeType`. */
-string ppReprType(DataFlowType t) { none() } // stub implementation
-
 /**
  * Holds if `t1` and `t2` are compatible, that is, whether data can flow from
  * a node of type `t1` to a node of type `t2`.
@@ -1097,7 +1094,11 @@ class SummarizedCallable extends DataFlowCallable, TSummarizedCallable {
 
 class DataFlowExpr = Expr;
 
-class DataFlowType = Type;
+final private class TypeFinal = Type;
+
+class DataFlowType extends TypeFinal {
+  string toString() { result = "" }
+}
 
 cached
 private newtype TDataFlowCall =

cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll

@@ -227,13 +227,15 @@ private newtype TMemoryLocation =
   TAllAliasedMemory(IRFunction irFunc, Boolean isMayAccess)
 
 /**
- * Represents the memory location accessed by a memory operand or memory result. In this implementation, the location is
+ * A memory location accessed by a memory operand or memory result. In this implementation, the location is
  * one of the following:
  * - `VariableMemoryLocation` - A location within a known `IRVariable`, at an offset that is either a constant or is
  * unknown.
  * - `UnknownMemoryLocation` - A location not known to be within a specific `IRVariable`.
+ *
+ * Some of these memory locations will be filtered out for performance reasons before being passed to SSA construction.
  */
-abstract class MemoryLocation extends TMemoryLocation {
+abstract private class MemoryLocation0 extends TMemoryLocation {
   final string toString() {
     if this.isMayAccess()
     then result = "?" + this.toStringInternal()
@@ -294,9 +296,9 @@ abstract class MemoryLocation extends TMemoryLocation {
  * represented by a `MemoryLocation` that totally overlaps all other
  * `MemoryLocations` in the set.
  */
-abstract class VirtualVariable extends MemoryLocation { }
+abstract class VirtualVariable extends MemoryLocation0 { }
 
-abstract class AllocationMemoryLocation extends MemoryLocation {
+abstract class AllocationMemoryLocation extends MemoryLocation0 {
   Allocation var;
   boolean isMayAccess;
 
@@ -424,7 +426,7 @@ class VariableMemoryLocation extends TVariableMemoryLocation, AllocationMemoryLo
  * `{a, b}` into a memory location that represents _all_ of the allocations
  * in the set.
  */
-class GroupedMemoryLocation extends TGroupedMemoryLocation, MemoryLocation {
+class GroupedMemoryLocation extends TGroupedMemoryLocation, MemoryLocation0 {
   VariableGroup vg;
   boolean isMayAccess;
   boolean isAll;
@@ -528,7 +530,7 @@ class GroupedVirtualVariable extends GroupedMemoryLocation, VirtualVariable {
 /**
  * An access to memory that is not known to be confined to a specific `IRVariable`.
  */
-class UnknownMemoryLocation extends TUnknownMemoryLocation, MemoryLocation {
+class UnknownMemoryLocation extends TUnknownMemoryLocation, MemoryLocation0 {
   IRFunction irFunc;
   boolean isMayAccess;
 
@@ -555,7 +557,7 @@ class UnknownMemoryLocation extends TUnknownMemoryLocation, MemoryLocation {
  * An access to memory that is not known to be confined to a specific `IRVariable`, but is known to
  * not access memory on the current function's stack frame.
  */
-class AllNonLocalMemory extends TAllNonLocalMemory, MemoryLocation {
+class AllNonLocalMemory extends TAllNonLocalMemory, MemoryLocation0 {
   IRFunction irFunc;
   boolean isMayAccess;
 
@@ -589,7 +591,7 @@ class AllNonLocalMemory extends TAllNonLocalMemory, MemoryLocation {
 /**
  * An access to all aliased memory.
  */
-class AllAliasedMemory extends TAllAliasedMemory, MemoryLocation {
+class AllAliasedMemory extends TAllAliasedMemory, MemoryLocation0 {
   IRFunction irFunc;
   boolean isMayAccess;
 
@@ -620,7 +622,7 @@ class AliasedVirtualVariable extends AllAliasedMemory, VirtualVariable {
 /**
  * Gets the overlap relationship between the definition location `def` and the use location `use`.
  */
-Overlap getOverlap(MemoryLocation def, MemoryLocation use) {
+Overlap getOverlap(MemoryLocation0 def, MemoryLocation0 use) {
   exists(Overlap overlap |
     // Compute the overlap based only on the extent.
     overlap = getExtentOverlap(def, use) and
@@ -648,7 +650,7 @@ Overlap getOverlap(MemoryLocation def, MemoryLocation use) {
  * based only on the set of memory locations accessed. Handling of "may" accesses and read-only
  * locations occurs in `getOverlap()`.
  */
-private Overlap getExtentOverlap(MemoryLocation def, MemoryLocation use) {
+private Overlap getExtentOverlap(MemoryLocation0 def, MemoryLocation0 use) {
   // The def and the use must have the same virtual variable, or no overlap is possible.
   (
     // AllAliasedMemory must totally overlap any location within the same virtual variable.
@@ -861,6 +863,40 @@ predicate canReuseSsaForOldResult(Instruction instr) { OldSsa::canReuseSsaForMem
 bindingset[result, b]
 private boolean unbindBool(boolean b) { result != b.booleanNot() }
 
+/** Gets the number of overlapping uses of `def`. */
+private int numberOfOverlappingUses(MemoryLocation0 def) {
+  result = strictcount(MemoryLocation0 use | exists(getOverlap(def, use)))
+}
+
+/**
+ * Holds if `def` is a busy definition. That is, it has a large number of
+ * overlapping uses.
+ */
+private predicate isBusyDef(MemoryLocation0 def) { numberOfOverlappingUses(def) > 1024 }
+
+/** Holds if `use` is a use that overlaps with a busy definition. */
+private predicate useOverlapWithBusyDef(MemoryLocation0 use) {
+  exists(MemoryLocation0 def |
+    exists(getOverlap(def, use)) and
+    isBusyDef(def)
+  )
+}
+
+final private class FinalMemoryLocation = MemoryLocation0;
+
+/**
+ * A memory location accessed by a memory operand or memory result. In this implementation, the location is
+ * one of the following:
+ * - `VariableMemoryLocation` - A location within a known `IRVariable`, at an offset that is either a constant or is
+ * unknown.
+ * - `UnknownMemoryLocation` - A location not known to be within a specific `IRVariable`.
+ *
+ * Compared to `MemoryLocation0`, this class does not contain memory locations that represent uses of busy definitions.
+ */
+class MemoryLocation extends FinalMemoryLocation {
+  MemoryLocation() { not useOverlapWithBusyDef(this) }
+}
+
 MemoryLocation getResultMemoryLocation(Instruction instr) {
   not canReuseSsaForOldResult(instr) and
   exists(MemoryAccessKind kind, boolean isMayAccess |
@@ -905,9 +941,9 @@ MemoryLocation getResultMemoryLocation(Instruction instr) {
   )
 }
 
-MemoryLocation getOperandMemoryLocation(MemoryOperand operand) {
+private MemoryLocation0 getOperandMemoryLocation0(MemoryOperand operand, boolean isMayAccess) {
   not canReuseSsaForOldResult(operand.getAnyDef()) and
-  exists(MemoryAccessKind kind, boolean isMayAccess |
+  exists(MemoryAccessKind kind |
     kind = operand.getMemoryAccess() and
     (if operand.hasMayReadMemoryAccess() then isMayAccess = true else isMayAccess = false) and
     (
@@ -948,6 +984,19 @@ MemoryLocation getOperandMemoryLocation(MemoryOperand operand) {
   )
 }
 
+MemoryLocation getOperandMemoryLocation(MemoryOperand operand) {
+  exists(MemoryLocation0 use0, boolean isMayAccess |
+    use0 = getOperandMemoryLocation0(operand, isMayAccess)
+  |
+    result = use0
+    or
+    // If `use0` overlaps with a busy definition we turn it into a use
+    // of `UnknownMemoryLocation`.
+    not use0 instanceof MemoryLocation and
+    result = TUnknownMemoryLocation(operand.getEnclosingIRFunction(), isMayAccess)
+  )
+}
+
 /** Gets the start bit offset of a `MemoryLocation`, if any. */
 int getStartBitOffset(VariableMemoryLocation location) {
   result = location.getStartBitOffset() and Ints::hasValue(result)

cpp/ql/lib/semmle/code/cpp/models/implementations/StdMath.qll

@@ -51,6 +51,12 @@ private class Remquo extends Function, SideEffectFunction {
   override predicate hasOnlySpecificReadSideEffects() { any() }
 
   override predicate hasOnlySpecificWriteSideEffects() { any() }
+
+  override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
+    this.getParameter(i).getUnspecifiedType() instanceof PointerType and
+    buffer = false and
+    mustWrite = true
+  }
 }
 
 private class Fma extends Function, SideEffectFunction {
@@ -95,4 +101,8 @@ private class Nan extends Function, SideEffectFunction, AliasFunction {
   override predicate parameterNeverEscapes(int index) { index = 0 }
 
   override predicate parameterEscapesOnlyViaReturn(int index) { none() }
+
+  override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
+    i = 0 and buffer = true
+  }
 }

cpp/ql/src/Security/CWE/CWE-014/MemsetMayBeDeleted.qhelp

@@ -10,11 +10,12 @@ contains sensitive data that could somehow be retrieved by an attacker.</p>
 </overview>
 <recommendation>
 
-<p>Use alternative platform-supplied functions that will not get optimized away. Examples of such
-functions include <code>memset_s</code>, <code>SecureZeroMemory</code>, and <code>bzero_explicit</code>.
-Alternatively, passing the <code>-fno-builtin-memset</code> option to the GCC/Clang compiler usually
-also prevents the optimization. Finally, you can use the public-domain <code>secure_memzero</code> function
-(see references below). This function, however, is not guaranteed to work on all platforms and compilers.</p>
+<p>Use <code>memset_s</code> (from C11) instead of <code>memset</code>, as <code>memset_s</code> will not
+get optimized away. Alternatively use platform-supplied functions such as <code>SecureZeroMemory</code> or
+<code>bzero_explicit</code> that make the same guarantee. Passing the <code>-fno-builtin-memset</code>
+option to the GCC/Clang compiler usually also prevents the optimization. Finally, you can use the
+public-domain <code>secure_memzero</code> function (see references below). This function, however, is not
+guaranteed to work on all platforms and compilers.</p>
 
 </recommendation>
 <example>

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql

@@ -5,7 +5,7 @@
  * @id cpp/unsigned-difference-expression-compared-zero
  * @problem.severity warning
  * @security-severity 9.8
- * @precision medium
+ * @precision high
  * @tags security
  *       correctness
  *       external/cwe/cwe-191

cpp/ql/src/Security/CWE/CWE-311/CleartextStorage.c

@@ -1,12 +1,31 @@
-void writeCredentials() {
-  char *password = "cleartext password";
-  FILE* file = fopen("credentials.txt", "w");
-  
+#include <sodium.h>
+#include <stdio.h>
+#include <string.h>
+
+void writeCredentialsBad(FILE *file, const char *cleartextCredentials) {
   // BAD: write password to disk in cleartext
-  fputs(password, file);
-  
-  // GOOD: encrypt password first
-  char *encrypted = encrypt(password);
-  fputs(encrypted, file);
+  fputs(cleartextCredentials, file);
 }
 
+int writeCredentialsGood(FILE *file, const char *cleartextCredentials, const unsigned char *key, const unsigned char *nonce) {
+  size_t credentialsLen = strlen(cleartextCredentials);
+  size_t ciphertext_len = crypto_secretbox_MACBYTES + credentialsLen;
+  unsigned char *ciphertext = malloc(ciphertext_len);
+  if (!ciphertext) {
+    logError();
+    return -1;
+  }
+
+  // encrypt the password first
+  if (crypto_secretbox_easy(ciphertext, (const unsigned char *)cleartextCredentials, credentialsLen, nonce, key) != 0) {
+    free(ciphertext);
+    logError();
+    return -1;
+  }
+
+  // GOOD: write encrypted password to disk
+  fwrite(ciphertext, 1, ciphertext_len, file);
+
+  free(ciphertext);
+  return 0;
+}

cpp/ql/src/Security/CWE/CWE-311/CleartextStorage.inc.qhelp

@@ -19,15 +19,20 @@ cleartext.</p>
 <example>
 
 <p>The following example shows two ways of storing user credentials in a file. In the 'BAD' case,
-the credentials are simply stored in cleartext. In the 'GOOD' case, the credentials are encrypted before 
+the credentials are simply stored in cleartext. In the 'GOOD' case, the credentials are encrypted before
 storing them.</p>
 
 <sample src="CleartextStorage.c" />
 
+<p>Note that for the 'GOOD' example to work we need to link against an encryption library (in this case libsodium),
+initialize it with a call to <code>sodium_init</code>, and create the key and nonce with
+<code>crypto_secretbox_keygen</code> and <code>randombytes_buf</code> respectively. We also need to store those
+details securely so they can be used for decryption.</p>
+
 </example>
 <references>
 
-<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li> 
+<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li>
 <li>M. Howard and D. LeBlanc, <i>Writing Secure Code</i>, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.</li>
 
 

cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.c

@@ -1,6 +1,6 @@
 
 void bad(void) {
-  char *password = "cleartext password";
+  const char *password = "cleartext password";
   sqlite3 *credentialsDB;
   sqlite3_stmt *stmt;
 
@@ -16,14 +16,15 @@ void bad(void) {
   }
 }
 
-void good(void) {
-  char *password = "cleartext password";
+void good(const char *secretKey) {
+  const char *password = "cleartext password";
   sqlite3 *credentialsDB;
   sqlite3_stmt *stmt;
 
   if (sqlite3_open("credentials.db", &credentialsDB) == SQLITE_OK) {
     // GOOD: database encryption enabled:
-    sqlite3_exec(credentialsDB, "PRAGMA key = 'secretKey!'", NULL, NULL, NULL);
+    std::string setKeyString = std::string("PRAGMA key = '") + secretKey + "'";
+    sqlite3_exec(credentialsDB, setKeyString.c_str(), NULL, NULL, NULL);
     sqlite3_exec(credentialsDB, "CREATE TABLE IF NOT EXISTS creds (password TEXT);", NULL, NULL, NULL);
     if (sqlite3_prepare_v2(credentialsDB, "INSERT INTO creds(password) VALUES(?)", -1, &stmt, NULL) == SQLITE_OK) {
       sqlite3_bind_text(stmt, 1, password, -1, SQLITE_TRANSIENT);
@@ -33,4 +34,3 @@ void good(void) {
     }
   }
 }
-