Skip to content

Commit b463056

Browse files
ex0dus-0xex0dus-0x
committed
Fix TODOs for openssl queries, add additional support for BN_CTX* models and querying for unbalanced BN_CTX usage
1 parent a906346 commit b463056

File tree

4 files changed

+177
-23
lines changed

4 files changed

+177
-23
lines changed

‎cpp/lib/trailofbits/crypto/openssl/bn.qll‎

Lines changed: 88 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ import trailofbits.crypto.common
44
// BIGNUM *BN_new(void);
55
class BN_new extends CustomAllocator {
66
BN_new() {
7-
this.getQualifiedName() = "BN_new" and (
7+
this.getQualifiedName() = "BN_new" and
8+
(
89
dealloc instanceof BN_free or
910
dealloc instanceof BN_clear_free
1011
)
@@ -14,7 +15,8 @@ class BN_new extends CustomAllocator {
1415
// BIGNUM *BN_secure_new(void);
1516
class BN_secure_new extends CustomAllocator {
1617
BN_secure_new() {
17-
this.getQualifiedName() = "BN_secure_new" and (
18+
this.getQualifiedName() = "BN_secure_new" and
19+
(
1820
dealloc instanceof BN_free or
1921
dealloc instanceof BN_clear_free
2022
)
@@ -23,24 +25,16 @@ class BN_secure_new extends CustomAllocator {
2325

2426
// void BN_free(BIGNUM *a);
2527
class BN_free extends CustomDeallocator {
26-
BN_free() {
27-
this.getQualifiedName() = "BN_free"
28-
}
28+
BN_free() { this.getQualifiedName() = "BN_free" }
2929

30-
override int getPointer() {
31-
result = 0
32-
}
30+
override int getPointer() { result = 0 }
3331
}
3432

3533
// void BN_clear_free(BIGNUM *a);
3634
class BN_clear_free extends CustomDeallocator {
37-
BN_clear_free() {
38-
this.getQualifiedName() = "BN_clear_free"
39-
}
35+
BN_clear_free() { this.getQualifiedName() = "BN_clear_free" }
4036

41-
override int getPointer() {
42-
result = 0
43-
}
37+
override int getPointer() { result = 0 }
4438
}
4539

4640
// void BN_clear(BIGNUM *a);
@@ -50,18 +44,92 @@ class BN_clear extends FunctionCall {
5044
Expr getBignum() { result = this.getArgument(0) }
5145
}
5246

53-
// int BN_rand(BIGNUM *rnd, int bits, int top, int bottom);
47+
// int BN_rand(BIGNUM *rnd, int bits, int top, int bottom); (and variants)
48+
/// Reference: https://docs.openssl.org/master/man3/BN_rand/#synopsis
5449
class BN_rand extends FunctionCall {
55-
BN_rand() { this.getTarget().getName() = "BN_rand" }
56-
57-
Expr getBignum() {
58-
result = this.getArgument(0)
50+
BN_rand() {
51+
this.getTarget().getName().matches("BN\\_rand%") or
52+
this.getTarget().getName().matches("BN\\_priv\\_rand%") or
53+
this.getTarget().getName().matches("BN\\_pseudo\\_rand%")
5954
}
55+
56+
Expr getBignum() { result = this.getArgument(0) }
6057
}
6158

6259
class BIGNUM extends FunctionCall {
63-
BIGNUM () {
60+
BIGNUM() {
6461
this.getTarget() instanceof BN_new or
6562
this.getTarget() instanceof BN_secure_new
6663
}
6764
}
65+
66+
// BN_CTX *BN_CTX_new(void);
67+
class BN_CTX_new extends CustomAllocator {
68+
BN_CTX_new() {
69+
this.getName() = "BN_CTX_new" and
70+
dealloc instanceof BN_CTX_free
71+
}
72+
}
73+
74+
// BN_CTX *BN_CTX_secure_new(void);
75+
class BN_CTX_secure_new extends CustomAllocator {
76+
BN_CTX_secure_new() {
77+
this.getName() = "BN_CTX_secure_new" and
78+
dealloc instanceof BN_CTX_free
79+
}
80+
}
81+
82+
// void BN_CTX_free(BN_CTX *c);
83+
class BN_CTX_free extends CustomDeallocator {
84+
BN_CTX_free() { this.getName() = "BN_CTX_free" }
85+
86+
override int getPointer() { result = 0 }
87+
}
88+
89+
// void BN_CTX_start(BN_CTX *ctx);
90+
class BN_CTX_start extends Expr {
91+
BN_CTX_start() {
92+
exists(FunctionCall fc |
93+
fc = this and
94+
fc.getTarget().getName() = "BN_CTX_start"
95+
)
96+
}
97+
98+
Expr getContext() { result = this.(FunctionCall).getArgument(0) }
99+
}
100+
101+
// void BN_CTX_end(BN_CTX *ctx);
102+
class BN_CTX_end extends Expr {
103+
BN_CTX_end() {
104+
exists(FunctionCall fc |
105+
fc = this and
106+
fc.getTarget().getName() = "BN_CTX_end"
107+
)
108+
}
109+
110+
Expr getContext() { result = this.(FunctionCall).getArgument(0) }
111+
}
112+
113+
// BIGNUM *BN_CTX_get(BN_CTX *ctx);
114+
class BN_CTX_get extends Expr {
115+
BN_CTX_get() {
116+
exists(FunctionCall fc |
117+
fc = this and
118+
fc.getTarget().getName() = "BN_CTX_get"
119+
)
120+
}
121+
122+
Expr getContext() { result = this.(FunctionCall).getArgument(0) }
123+
}
124+
125+
class BN_CTX extends Expr {
126+
BN_CTX() {
127+
exists(FunctionCall fc |
128+
fc = this and
129+
(
130+
fc.getTarget() instanceof BN_CTX_new or
131+
fc.getTarget() instanceof BN_CTX_secure_new
132+
)
133+
)
134+
}
135+
}

‎cpp/src/crypto/BnCtxFreeBeforeEnd.ql‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/**
2+
* @name BN_CTX_free called before BN_CTX_end
3+
* @id tob/cpp/bn-ctx-free-before-end
4+
* @description Detects BN_CTX_free called before BN_CTX_end, which violates the required lifecycle
5+
* @kind path-problem
6+
* @tags correctness crypto
7+
* @problem.severity error
8+
* @precision high
9+
* @group cryptography
10+
*/
11+
12+
import cpp
13+
import trailofbits.crypto.libraries
14+
import semmle.code.cpp.dataflow.new.DataFlow
15+
import semmle.code.cpp.controlflow.ControlFlowGraph
16+
import DataFlow::PathGraph
17+
18+
/**
19+
* Tracks BN_CTX from BN_CTX_start to BN_CTX_free without going through BN_CTX_end
20+
*/
21+
module BnCtxFreeBeforeEndConfig implements DataFlow::ConfigSig {
22+
predicate isSource(DataFlow::Node source) {
23+
exists(BN_CTX_start start |
24+
source.asExpr() = start.getContext()
25+
)
26+
}
27+
28+
predicate isSink(DataFlow::Node sink) {
29+
exists(CustomDeallocatorCall free |
30+
free.getTarget() instanceof BN_CTX_free and
31+
sink.asExpr() = free.getPointer()
32+
)
33+
}
34+
35+
predicate isBarrier(DataFlow::Node node) {
36+
// If the context flows through BN_CTX_end, it's properly handled
37+
exists(BN_CTX_end end |
38+
node.asExpr() = end.getContext()
39+
)
40+
}
41+
}
42+
43+
module BnCtxFreeBeforeEndFlow = DataFlow::Global<BnCtxFreeBeforeEndConfig>;
44+
45+
from BnCtxFreeBeforeEndFlow::PathNode source, BnCtxFreeBeforeEndFlow::PathNode sink,
46+
BN_CTX_start start, CustomDeallocatorCall free
47+
where
48+
BnCtxFreeBeforeEndFlow::flowPath(source, sink) and
49+
source.getNode().asExpr() = start.getContext() and
50+
sink.getNode().asExpr() = free.getPointer() and
51+
free.getTarget() instanceof BN_CTX_free
52+
select free, source, sink,
53+
"BN_CTX_free called at line " + free.getLocation().getStartLine().toString() +
54+
" before BN_CTX_end after BN_CTX_start at line " +
55+
start.getLocation().getStartLine().toString()

‎cpp/src/crypto/MissingBnCtxEnd.ql‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* @name Missing BN_CTX_end after BN_CTX_start
3+
* @id tob/cpp/missing-bn-ctx-end
4+
* @description Detects BN_CTX_start calls without corresponding BN_CTX_end calls
5+
* @kind problem
6+
* @tags correctness crypto
7+
* @problem.severity warning
8+
* @precision medium
9+
* @group cryptography
10+
*/
11+
12+
import cpp
13+
import trailofbits.crypto.libraries
14+
import semmle.code.cpp.dataflow.new.DataFlow
15+
import semmle.code.cpp.controlflow.Guards
16+
17+
predicate hasMatchingEnd(BN_CTX_start start) {
18+
exists(BN_CTX_end end, Function f |
19+
start.getEnclosingFunction() = f and
20+
end.getEnclosingFunction() = f and
21+
DataFlow::localFlow(DataFlow::exprNode(start.getContext()),
22+
DataFlow::exprNode(end.getContext()))
23+
)
24+
}
25+
26+
from BN_CTX_start start
27+
where not hasMatchingEnd(start)
28+
select start.getLocation(),
29+
"BN_CTX_start called without corresponding BN_CTX_end"

‎cpp/src/crypto/MissingZeroization.ql‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,15 @@ import cpp
1313
import trailofbits.crypto.libraries
1414
import semmle.code.cpp.dataflow.new.DataFlow
1515

16-
// TODO: Handle `BN_clear_free` as well.
1716
predicate isCleared(Expr bignum) {
1817
exists(BN_clear clear |
1918
DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(clear.getArgument(0)))
19+
) or
20+
exists(BN_clear_free clear_free |
21+
DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(clear_free.getArgument(0)))
2022
)
2123
}
2224

23-
// TODO: Add support for remaining OpenSSL PRNG functions.
2425
predicate isRandom(Expr bignum) {
2526
exists(BN_rand rand |
2627
DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(rand.getArgument(0)))
@@ -29,4 +30,5 @@ predicate isRandom(Expr bignum) {
2930

3031
from BIGNUM bignum
3132
where isRandom(bignum) and not isCleared(bignum)
32-
select bignum.getLocation(), "Bignum is initialized with random data but is not zeroized before it goes out of scope"
33+
select bignum.getLocation(),
34+
"Bignum is initialized with random data but is not zeroized before it goes out of scope"

0 commit comments

Comments
 (0)