Merge branch 'main' into py/add-ssrf-sinks

Author: haby0

cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql

@@ -2,7 +2,7 @@
  * @name Use of expired stack-address
  * @description Accessing the stack-allocated memory of a function
  *              after it has returned can lead to memory corruption.
- * @kind problem
+ * @kind path-problem
  * @problem.severity error
  * @security-severity 9.3
  * @precision high
@@ -178,13 +178,10 @@ predicate blockStoresToAddress(
   globalAddress = globalAddress(store.getDestinationAddress())
 }
 
-predicate blockLoadsFromAddress(
-  IRBlock block, int index, LoadInstruction load, TGlobalAddress globalAddress
-) {
-  block.getInstruction(index) = load and
-  globalAddress = globalAddress(load.getSourceAddress())
-}
-
+/**
+ * Holds if `globalAddress` evaluates to the address of `var` (which escaped through `store` before
+ * returning through `call`) when control reaches `block`.
+ */
 predicate globalAddressPointsToStack(
   StoreInstruction store, StackVariable var, CallInstruction call, IRBlock block,
   TGlobalAddress globalAddress, boolean isCallBlock, boolean isStoreBlock
@@ -203,21 +200,127 @@ predicate globalAddressPointsToStack(
     )
     or
     isCallBlock = false and
-    exists(IRBlock mid |
-      mid.immediatelyDominates(block) and
-      // Only recurse if there is no store to `globalAddress` in `mid`.
-      globalAddressPointsToStack(store, var, call, mid, globalAddress, _, false)
+    step(store, var, call, globalAddress, _, block)
+  )
+}
+
+pragma[inline]
+int getInstructionIndex(Instruction instr, IRBlock block) { block.getInstruction(result) = instr }
+
+predicate step(
+  StoreInstruction store, StackVariable var, CallInstruction call, TGlobalAddress globalAddress,
+  IRBlock pred, IRBlock succ
+) {
+  exists(boolean isCallBlock, boolean isStoreBlock |
+    // Only recurse if there is no store to `globalAddress` in `mid`.
+    globalAddressPointsToStack(store, var, call, pred, globalAddress, isCallBlock, isStoreBlock)
+  |
+    // Post domination ensures that `block` is always executed after `mid`
+    // Domination ensures that `mid` is always executed before `block`
+    isStoreBlock = false and
+    succ.immediatelyPostDominates(pred) and
+    pred.immediatelyDominates(succ)
+    or
+    exists(CallInstruction anotherCall, int anotherCallIndex |
+      anotherCall = pred.getInstruction(anotherCallIndex) and
+      succ.getFirstInstruction() instanceof EnterFunctionInstruction and
+      succ.getEnclosingFunction() = anotherCall.getStaticCallTarget() and
+      (if isCallBlock = true then getInstructionIndex(call, _) < anotherCallIndex else any()) and
+      (
+        if isStoreBlock = true
+        then
+          forex(int storeIndex | blockStoresToAddress(pred, storeIndex, _, globalAddress) |
+            anotherCallIndex < storeIndex
+          )
+        else any()
+      )
     )
   )
 }
 
+newtype TPathElement =
+  TStore(StoreInstruction store) { globalAddressPointsToStack(store, _, _, _, _, _, _) } or
+  TCall(CallInstruction call, IRBlock block) {
+    globalAddressPointsToStack(_, _, call, block, _, _, _)
+  } or
+  TMid(IRBlock block) { step(_, _, _, _, _, block) } or
+  TSink(LoadInstruction load, IRBlock block) {
+    exists(TGlobalAddress address |
+      globalAddressPointsToStack(_, _, _, block, address, _, _) and
+      block.getAnInstruction() = load and
+      globalAddress(load.getSourceAddress()) = address
+    )
+  }
+
+class PathElement extends TPathElement {
+  StoreInstruction asStore() { this = TStore(result) }
+
+  CallInstruction asCall(IRBlock block) { this = TCall(result, block) }
+
+  predicate isCall(IRBlock block) { exists(this.asCall(block)) }
+
+  IRBlock asMid() { this = TMid(result) }
+
+  LoadInstruction asSink(IRBlock block) { this = TSink(result, block) }
+
+  predicate isSink(IRBlock block) { exists(this.asSink(block)) }
+
+  string toString() {
+    result = [asStore().toString(), asCall(_).toString(), asMid().toString(), asSink(_).toString()]
+  }
+
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    this.asStore()
+        .getLocation()
+        .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    or
+    this.asCall(_)
+        .getLocation()
+        .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    or
+    this.asMid().getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    or
+    this.asSink(_)
+        .getLocation()
+        .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+  }
+}
+
+predicate isSink(LoadInstruction load, IRBlock block, int index, TGlobalAddress globalAddress) {
+  block.getInstruction(index) = load and
+  globalAddress(load.getSourceAddress()) = globalAddress
+}
+
+query predicate edges(PathElement pred, PathElement succ) {
+  // Store -> caller
+  globalAddressPointsToStack(pred.asStore(), _, succ.asCall(_), _, _, _, _)
+  or
+  // Call -> basic block
+  pred.isCall(succ.asMid())
+  or
+  // Special case for when the caller goes directly to the load with no steps
+  // across basic blocks (i.e., caller -> sink)
+  exists(IRBlock block |
+    pred.isCall(block) and
+    succ.isSink(block)
+  )
+  or
+  // Basic block -> basic block
+  step(_, _, _, _, pred.asMid(), succ.asMid())
+  or
+  // Basic block -> load
+  succ.isSink(pred.asMid())
+}
+
 from
   StoreInstruction store, StackVariable var, LoadInstruction load, CallInstruction call,
-  IRBlock block, boolean isCallBlock, TGlobalAddress address, boolean isStoreBlock
+  IRBlock block, boolean isCallBlock, TGlobalAddress address, boolean isStoreBlock,
+  PathElement source, PathElement sink, int loadIndex
 where
   globalAddressPointsToStack(store, var, call, block, address, isCallBlock, isStoreBlock) and
-  block.getAnInstruction() = load and
-  globalAddress(load.getSourceAddress()) = address and
+  isSink(load, block, loadIndex, address) and
   (
     // We know that we have a sequence:
     // (1) store to `address` -> (2) return from `f` -> (3) load from `address`.
@@ -226,22 +329,20 @@ where
     if isCallBlock = true
     then
       // If so, the load must happen after the call.
-      exists(int callIndex, int loadIndex |
-        blockLoadsFromAddress(_, loadIndex, load, _) and
-        block.getInstruction(callIndex) = call and
-        callIndex < loadIndex
+      getInstructionIndex(call, _) < loadIndex
+    else any()
+  ) and
+  (
+    // If there is a store to the address we need to make sure that the load we found was
+    // before that store (So that the load doesn't read an overwritten value).
+    if isStoreBlock = true
+    then
+      forex(int storeIndex | blockStoresToAddress(block, storeIndex, _, address) |
+        loadIndex < storeIndex
       )
     else any()
   ) and
-  // If there is a store to the address we need to make sure that the load we found was
-  // before that store (So that the load doesn't read an overwritten value).
-  if isStoreBlock = true
-  then
-    exists(int storeIndex, int loadIndex |
-      blockStoresToAddress(block, storeIndex, _, address) and
-      block.getInstruction(loadIndex) = load and
-      loadIndex < storeIndex
-    )
-  else any()
-select load, "Stack variable $@ escapes $@ and is used after it has expired.", var, var.toString(),
-  store, "here"
+  source.asStore() = store and
+  sink.asSink(_) = load
+select sink, source, sink, "Stack variable $@ escapes $@ and is used after it has expired.", var,
+  var.toString(), store, "here"

cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.c

@@ -0,0 +1,8 @@
+void encrypt_with_openssl(EVP_PKEY_CTX *ctx) {
+
+  // BAD: only 1024 bits for an RSA key
+  EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, 1024);
+
+  // GOOD: 2048 bits for an RSA key
+  EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, 2048);
+}

cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.qhelp

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using cryptographic algorithms with a small key size can leave data vulnerable to being decrypted.</p>
+
+<p>Many cryptographic algorithms provided by cryptography libraries can be configured with key sizes that are
+vulnerable to brute force attacks. Using such a key size means that an attacker may be able to easily decrypt the
+encrypted data.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that you use a strong, modern cryptographic algorithm. Use at least AES-128 or RSA-2048.</p>
+
+</recommendation>
+<example>
+
+<p>The following code shows an example of using the <code>openssl</code> library to generate an RSA key.
+When creating a key, you must specify which key size to use. The first example uses 1024 bits, which is not
+considered sufficient. The second example uses 2048 bits, which is currently considered sufficient.</p>
+
+<sample src="InsufficientKeySize.c" />
+
+</example>
+<references>
+
+<li>NIST, FIPS 140 Annex a: <a href="http://csrc.nist.gov/publications/fips/fips140-2/fips1402annexa.pdf">
+Approved Security Functions</a>.</li>
+<li>NIST, SP 800-131A: <a href="https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar2.pdf">
+Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths</a>.</li>
+
+<!--  LocalWords:  CWE
+ -->
+
+</references>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql

@@ -0,0 +1,63 @@
+/**
+ * @name Use of a cryptographic algorithm with insufficient key size
+ * @description Using cryptographic algorithms with too small a key size can
+ *              allow an attacker to compromise security.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/insufficient-key-size
+ * @tags security
+ *       external/cwe/cwe-326
+ */
+
+import cpp
+import semmle.code.cpp.ir.dataflow.DataFlow
+import semmle.code.cpp.ir.IR
+import DataFlow::PathGraph
+
+// Gets the recommended minimum key size (in bits) of `func`, the name of an encryption function that accepts a key size as parameter `paramIndex`
+int getMinimumKeyStrength(string func, int paramIndex) {
+  func =
+    [
+      "EVP_PKEY_CTX_set_dsa_paramgen_bits", "DSA_generate_parameters_ex",
+      "EVP_PKEY_CTX_set_rsa_keygen_bits", "RSA_generate_key_ex", "RSA_generate_key_fips",
+      "EVP_PKEY_CTX_set_dh_paramgen_prime_len", "DH_generate_parameters_ex"
+    ] and
+  paramIndex = 1 and
+  result = 2048
+}
+
+class KeyStrengthFlow extends DataFlow::Configuration {
+  KeyStrengthFlow() { this = "KeyStrengthFlow" }
+
+  override predicate isSource(DataFlow::Node node) {
+    exists(int bits |
+      node.asInstruction().(IntegerConstantInstruction).getValue().toInt() = bits and
+      bits < getMinimumKeyStrength(_, _) and
+      bits > 0 // exclude sentinel values
+    )
+  }
+
+  override predicate isSink(DataFlow::Node node) {
+    exists(FunctionCall fc, string name, int param |
+      node.asExpr() = fc.getArgument(param) and
+      fc.getTarget().hasGlobalName(name) and
+      exists(getMinimumKeyStrength(name, param))
+    )
+  }
+}
+
+from
+  DataFlow::PathNode source, DataFlow::PathNode sink, KeyStrengthFlow conf, FunctionCall fc,
+  int param, string name, int minimumBits, int bits
+where
+  conf.hasFlowPath(source, sink) and
+  sink.getNode().asExpr() = fc.getArgument(param) and
+  fc.getTarget().hasGlobalName(name) and
+  minimumBits = getMinimumKeyStrength(name, param) and
+  bits = source.getNode().asInstruction().(ConstantValueInstruction).getValue().toInt() and
+  bits < minimumBits and
+  bits != 0
+select fc, source, sink,
+  "The key size $@ is less than the recommended key size of " + minimumBits.toString() + " bits.",
+  source, bits.toString()

cpp/ql/src/change-notes/2022-02-16-insufficient-key-size.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* A new `cpp/insufficient-key-size` query has been added to the default query suite for C/C++. The query finds uses of certain cryptographic algorithms where the key size is too small to provide adequate encryption strength.