Merge pull request #31 from trailofbits/alan/enhance-crypto-queries

mschwager · web-flow · commit d9b80589870d · 2025-12-18T10:48:09.000-07:00
Improve and extend C++ openssl crypto queries
diff --git a/cpp/lib/trailofbits/crypto/openssl/bn.qll b/cpp/lib/trailofbits/crypto/openssl/bn.qll
@@ -4,7 +4,8 @@ import trailofbits.crypto.common
 // BIGNUM *BN_new(void);
 class BN_new extends CustomAllocator {
   BN_new() {
-    this.getQualifiedName() = "BN_new" and (
+    this.getQualifiedName() = "BN_new" and
+    (
       dealloc instanceof BN_free or
       dealloc instanceof BN_clear_free
     )
@@ -14,7 +15,8 @@ class BN_new extends CustomAllocator {
 // BIGNUM *BN_secure_new(void);
 class BN_secure_new extends CustomAllocator {
   BN_secure_new() {
-    this.getQualifiedName() = "BN_secure_new" and (
+    this.getQualifiedName() = "BN_secure_new" and
+    (
       dealloc instanceof BN_free or
       dealloc instanceof BN_clear_free
     )
@@ -23,24 +25,16 @@ class BN_secure_new extends CustomAllocator {
 
 // void BN_free(BIGNUM *a);
 class BN_free extends CustomDeallocator {
-  BN_free() {
-    this.getQualifiedName() = "BN_free"
-  }
+  BN_free() { this.getQualifiedName() = "BN_free" }
 
-  override int getPointer() {
-    result = 0
-  }
+  override int getPointer() { result = 0 }
 }
 
 // void BN_clear_free(BIGNUM *a);
 class BN_clear_free extends CustomDeallocator {
-  BN_clear_free() {
-    this.getQualifiedName() = "BN_clear_free"
-  }
+  BN_clear_free() { this.getQualifiedName() = "BN_clear_free" }
 
-  override int getPointer() {
-    result = 0
-  }
+  override int getPointer() { result = 0 }
 }
 
 // void BN_clear(BIGNUM *a);
@@ -50,18 +44,63 @@ class BN_clear extends FunctionCall {
   Expr getBignum() { result = this.getArgument(0) }
 }
 
-//  int BN_rand(BIGNUM *rnd, int bits, int top, int bottom);
+//  int BN_rand(BIGNUM *rnd, int bits, int top, int bottom); (and variants)
+/// Reference: https://docs.openssl.org/master/man3/BN_rand/#synopsis
 class BN_rand extends FunctionCall {
-  BN_rand() { this.getTarget().getName() = "BN_rand" }
-
-  Expr getBignum() {
-    result = this.getArgument(0)
+  BN_rand() {
+    this.getTarget().getName().matches("BN\\_rand%") or
+    this.getTarget().getName().matches("BN\\_priv\\_rand%") or
+    this.getTarget().getName().matches("BN\\_pseudo\\_rand%")
   }
+
+  Expr getBignum() { result = this.getArgument(0) }
 }
 
 class BIGNUM extends FunctionCall {
-  BIGNUM () {
+  BIGNUM() {
     this.getTarget() instanceof BN_new or
     this.getTarget() instanceof BN_secure_new
   }
 }
+
+// BN_CTX *BN_CTX_new(void);
+class BN_CTX_new extends CustomAllocator {
+  BN_CTX_new() {
+    this.getQualifiedName().matches("BN\\_CTX_new%")
+    or
+    this.getQualifiedName().matches("BN\\_CTX\\_secure\\_new%") and
+    dealloc instanceof BN_CTX_free
+  }
+}
+
+// void BN_CTX_free(BN_CTX *c);
+class BN_CTX_free extends CustomDeallocator {
+  BN_CTX_free() { this.getQualifiedName() = "BN_CTX_free" }
+
+  override int getPointer() { result = 0 }
+}
+
+// void BN_CTX_start(BN_CTX *ctx);
+class BN_CTX_start extends FunctionCall {
+  BN_CTX_start() { this.getTarget().getName() = "BN_CTX_start" }
+
+  Expr getContext() { result = this.getArgument(0) }
+}
+
+// void BN_CTX_end(BN_CTX *ctx);
+class BN_CTX_end extends FunctionCall {
+  BN_CTX_end() { this.getTarget().getName() = "BN_CTX_end" }
+
+  Expr getContext() { result = this.getArgument(0) }
+}
+
+// BIGNUM *BN_CTX_get(BN_CTX *ctx);
+class BN_CTX_get extends FunctionCall {
+  BN_CTX_get() { this.getTarget().getName() = "BN_CTX_get" }
+
+  Expr getContext() { result = this.getArgument(0) }
+}
+
+class BN_CTX extends FunctionCall {
+  BN_CTX() { this.getTarget() instanceof BN_CTX_new }
+}
diff --git a/cpp/src/crypto/BnCtxFreeBeforeEnd.ql b/cpp/src/crypto/BnCtxFreeBeforeEnd.ql
@@ -0,0 +1,52 @@
+/**
+ * @name BN_CTX_free called before BN_CTX_end
+ * @id tob/cpp/bn-ctx-free-before-end
+ * @description Detects BN_CTX_free called before BN_CTX_end, which violates the required lifecycle
+ * @kind path-problem
+ * @tags correctness crypto
+ * @problem.severity error
+ * @precision medium
+ * @group cryptography
+ */
+
+import cpp
+import trailofbits.crypto.libraries
+import semmle.code.cpp.dataflow.new.DataFlow
+import semmle.code.cpp.controlflow.ControlFlowGraph
+
+/**
+ * Tracks BN_CTX from BN_CTX_start to BN_CTX_free without going through BN_CTX_end
+ */
+module BnCtxFreeBeforeEndConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(BN_CTX_start start | source.asExpr() = start.getContext())
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(CustomDeallocatorCall free |
+      free.getTarget() instanceof BN_CTX_free and
+      sink.asExpr() = free.getPointer()
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    // If the context flows through BN_CTX_end, it's properly handled
+    exists(BN_CTX_end end | node.asExpr() = end.getContext())
+  }
+}
+
+module BnCtxFreeBeforeEndFlow = DataFlow::Global<BnCtxFreeBeforeEndConfig>;
+
+import BnCtxFreeBeforeEndFlow::PathGraph
+
+from
+  BnCtxFreeBeforeEndFlow::PathNode source, BnCtxFreeBeforeEndFlow::PathNode sink,
+  BN_CTX_start start, CustomDeallocatorCall free
+where
+  BnCtxFreeBeforeEndFlow::flowPath(source, sink) and
+  source.getNode().asExpr() = start.getContext() and
+  sink.getNode().asExpr() = free.getPointer() and
+  free.getTarget() instanceof BN_CTX_free
+select free, source, sink,
+  "BN_CTX_free called at line " + free.getLocation().getStartLine().toString() +
+    " before BN_CTX_end after BN_CTX_start at line " + start.getLocation().getStartLine().toString()
diff --git a/cpp/src/crypto/MissingZeroization.ql b/cpp/src/crypto/MissingZeroization.ql
@@ -13,20 +13,24 @@ import cpp
 import trailofbits.crypto.libraries
 import semmle.code.cpp.dataflow.new.DataFlow
 
-// TODO: Handle `BN_clear_free` as well.
 predicate isCleared(Expr bignum) {
   exists(BN_clear clear |
-    DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(clear.getArgument(0)))
+    DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(clear.getBignum()))
+  )
+  or
+  exists(CustomDeallocatorCall clear_free |
+    clear_free.getTarget() instanceof BN_clear_free and
+    DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(clear_free.getPointer()))
   )
 }
 
-// TODO: Add support for remaining OpenSSL PRNG functions.
 predicate isRandom(Expr bignum) {
   exists(BN_rand rand |
-    DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(rand.getArgument(0)))
+    DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(rand.getBignum()))
   )
 }
 
 from BIGNUM bignum
 where isRandom(bignum) and not isCleared(bignum)
-select bignum.getLocation(), "Bignum is initialized with random data but is not zeroized before it goes out of scope"
+select bignum.getLocation(),
+  "Bignum is initialized with random data but is not zeroized before it goes out of scope"
diff --git a/cpp/src/crypto/UnbalancedBnCtx.ql b/cpp/src/crypto/UnbalancedBnCtx.ql
@@ -0,0 +1,45 @@
+/**
+ * @name Unbalanced BN_CTX_start and BN_CTX_end pair
+ * @id tob/cpp/missing-bn-ctx-end
+ * @description Detects if one call in the BN_CTX_start/BN_CTX_end pair is missing
+ * @kind problem
+ * @tags correctness crypto
+ * @problem.severity warning
+ * @precision medium
+ * @group cryptography
+ */
+
+import cpp
+import trailofbits.crypto.libraries
+import semmle.code.cpp.dataflow.new.DataFlow
+import semmle.code.cpp.controlflow.Guards
+
+predicate hasMatchingEnd(BN_CTX_start start) {
+  exists(BN_CTX_end end, Function f |
+    start.getEnclosingFunction() = f and
+    end.getEnclosingFunction() = f and
+    DataFlow::localFlow(DataFlow::exprNode(start.getContext()), DataFlow::exprNode(end.getContext()))
+  )
+}
+
+predicate hasMatchingStart(BN_CTX_end end) {
+  exists(BN_CTX_start start, Function f |
+    end.getEnclosingFunction() = f and
+    start.getEnclosingFunction() = f and
+    DataFlow::localFlow(DataFlow::exprNode(start.getContext()), DataFlow::exprNode(end.getContext()))
+  )
+}
+
+from FunctionCall call, string message
+where
+  (
+    call instanceof BN_CTX_start and
+    not hasMatchingEnd(call) and
+    message = "BN_CTX_start called without corresponding BN_CTX_end"
+  ) or
+  (
+    call instanceof BN_CTX_end and
+    not hasMatchingStart(call) and
+    message = "BN_CTX_end called without corresponding BN_CTX_start"
+  )
+select call.getLocation(), message
diff --git a/cpp/test/include/openssl/bn.h b/cpp/test/include/openssl/bn.h
@@ -32,6 +32,32 @@ int BN_rand(BIGNUM *rnd, int bits, int top, int bottom) {
   return 1;
 }
 
+// BN_CTX functions
+#define BN_CTX int
+
+BN_CTX *BN_CTX_new(void) {
+  static BN_CTX ctx = 1;
+  return &ctx;
+}
+
+BN_CTX *BN_CTX_secure_new(void) {
+  static BN_CTX ctx = 1;
+  return &ctx;
+}
+
+void BN_CTX_free(BN_CTX *c) {
+}
+
+void BN_CTX_start(BN_CTX *ctx) {
+}
+
+void BN_CTX_end(BN_CTX *ctx) {
+}
+
+BIGNUM *BN_CTX_get(BN_CTX *ctx) {
+  return &BN;
+}
+
 # ifdef  __cplusplus
 }
 #endif
diff --git a/cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/BnCtxFreeBeforeEnd.expected b/cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/BnCtxFreeBeforeEnd.expected
@@ -0,0 +1,12 @@
+edges
+| test.c:13:16:13:18 | ctx | test.c:15:15:15:17 | ctx | provenance |  |
+| test.c:34:16:34:18 | ctx | test.c:40:15:40:17 | ctx | provenance |  |
+nodes
+| test.c:13:16:13:18 | ctx | semmle.label | ctx |
+| test.c:15:15:15:17 | ctx | semmle.label | ctx |
+| test.c:34:16:34:18 | ctx | semmle.label | ctx |
+| test.c:40:15:40:17 | ctx | semmle.label | ctx |
+subpaths
+#select
+| test.c:15:3:15:13 | call to BN_CTX_free | test.c:13:16:13:18 | ctx | test.c:15:15:15:17 | ctx | BN_CTX_free called at line 15 before BN_CTX_end after BN_CTX_start at line 13 |
+| test.c:40:3:40:13 | call to BN_CTX_free | test.c:34:16:34:18 | ctx | test.c:40:15:40:17 | ctx | BN_CTX_free called at line 40 before BN_CTX_end after BN_CTX_start at line 34 |
diff --git a/cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/BnCtxFreeBeforeEnd.qlref b/cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/BnCtxFreeBeforeEnd.qlref
@@ -0,0 +1 @@
+crypto/BnCtxFreeBeforeEnd.ql
diff --git a/cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/test.c b/cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/test.c
@@ -0,0 +1,43 @@
+#include "../../../include/openssl/bn.h"
+
+void good_proper_lifecycle() {
+  BN_CTX *ctx = BN_CTX_new();
+  BN_CTX_start(ctx);
+  BIGNUM *a = BN_CTX_get(ctx);
+  BN_CTX_end(ctx); // Properly call end before free
+  BN_CTX_free(ctx);
+}
+
+void bad_free_before_end() {
+  BN_CTX *ctx = BN_CTX_new();
+  BN_CTX_start(ctx);
+  BIGNUM *a = BN_CTX_get(ctx);
+  BN_CTX_free(ctx); // BUG: free before end
+}
+
+void good_mult_ctx() {
+  BN_CTX *ctx = BN_CTX_new();
+
+  BN_CTX_start(ctx);
+  BIGNUM *a = BN_CTX_get(ctx);
+  BN_CTX_end(ctx);
+
+  BN_CTX_start(ctx);
+  BIGNUM *b = BN_CTX_get(ctx);
+  BN_CTX_end(ctx);
+
+  BN_CTX_free(ctx);
+}
+
+void bad_conditional_end(int condition) {
+  BN_CTX *ctx = BN_CTX_new();
+  BN_CTX_start(ctx);
+  BIGNUM *a = BN_CTX_get(ctx);
+
+  if (condition) {
+    BN_CTX_end(ctx);
+  }
+  BN_CTX_free(ctx); // BUG: May free before end if condition is false
+}
+
+int main(void) {}
diff --git a/cpp/test/query-tests/crypto/UnbalancedBnCtx/UnbalancedBnCtx.expected b/cpp/test/query-tests/crypto/UnbalancedBnCtx/UnbalancedBnCtx.expected
@@ -0,0 +1,2 @@
+| test.c:13:3:13:14 | test.c:13:3:13:14 | BN_CTX_start called without corresponding BN_CTX_end |
+| test.c:21:3:21:12 | test.c:21:3:21:12 | BN_CTX_end called without corresponding BN_CTX_start |
diff --git a/cpp/test/query-tests/crypto/UnbalancedBnCtx/UnbalancedBnCtx.qlref b/cpp/test/query-tests/crypto/UnbalancedBnCtx/UnbalancedBnCtx.qlref
@@ -0,0 +1 @@
+crypto/UnbalancedBnCtx.ql
diff --git a/cpp/test/query-tests/crypto/UnbalancedBnCtx/test.c b/cpp/test/query-tests/crypto/UnbalancedBnCtx/test.c

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+\| test.c:13:3:13:14 \| test.c:13:3:13:14 \| BN_CTX_start called without corresponding BN_CTX_end \|`
	`2`	`+\| test.c:21:3:21:12 \| test.c:21:3:21:12 \| BN_CTX_end called without corresponding BN_CTX_start \|`