diff --git a/cpp/lib/trailofbits/crypto/openssl/bn.qll b/cpp/lib/trailofbits/crypto/openssl/bn.qll index 4fafca5..9eaaa3a 100644 --- 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 new file mode 100644 index 0000000..a1ad6ef --- /dev/null +++ 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; + +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 index a2b0513..dcd7b16 100644 --- 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 new file mode 100644 index 0000000..e13f2b4 --- /dev/null +++ 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 index 3e6e3f9..f05102a 100644 --- 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 new file mode 100644 index 0000000..a5527d7 --- /dev/null +++ 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 new file mode 100644 index 0000000..1f356fa --- /dev/null +++ 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 new file mode 100644 index 0000000..d80831c --- /dev/null +++ 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) {} \ No newline at end of file diff --git a/cpp/test/query-tests/crypto/UnbalancedBnCtx/UnbalancedBnCtx.expected b/cpp/test/query-tests/crypto/UnbalancedBnCtx/UnbalancedBnCtx.expected new file mode 100644 index 0000000..98ba061 --- /dev/null +++ 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 new file mode 100644 index 0000000..9aae5a7 --- /dev/null +++ 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 new file mode 100644 index 0000000..606cf04 --- /dev/null +++ b/cpp/test/query-tests/crypto/UnbalancedBnCtx/test.c @@ -0,0 +1,62 @@ +#include "../../../include/openssl/bn.h" + +void good_balanced() { + BN_CTX *ctx = BN_CTX_new(); + BN_CTX_start(ctx); + BIGNUM *a = BN_CTX_get(ctx); + BN_CTX_end(ctx); + BN_CTX_free(ctx); +} + +void bad_start_without_end() { + BN_CTX *ctx = BN_CTX_new(); + BN_CTX_start(ctx); // BUG: Missing corresponding end + BIGNUM *a = BN_CTX_get(ctx); + BN_CTX_free(ctx); +} + +void bad_end_without_start() { + BN_CTX *ctx = BN_CTX_new(); + BIGNUM *a = BN_CTX_get(ctx); + BN_CTX_end(ctx); // BUG: No corresponding start + BN_CTX_free(ctx); +} + +void good_conditional_balanced(int condition) { + BN_CTX *ctx = BN_CTX_new(); + if (condition) { + BN_CTX_start(ctx); + BIGNUM *a = BN_CTX_get(ctx); + BN_CTX_end(ctx); + } else { + BN_CTX_start(ctx); + BIGNUM *b = BN_CTX_get(ctx); + BN_CTX_end(ctx); + } + BN_CTX_free(ctx); +} + +void bad_conditional_start(int condition) { + BN_CTX *ctx = BN_CTX_new(); + + if (condition) { + BN_CTX_start(ctx); + BIGNUM *a = BN_CTX_get(ctx); + } + + BN_CTX_end(ctx); // BUG: End without start if condition is false + 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); + } + // BUG: Start without end if condition is false + BN_CTX_free(ctx); +} \ No newline at end of file