Skip to content

Commit d919310

Browse files
ex0dus-0xex0dus-0x
committed
Fix up queries and add tests
1 parent b463056 commit d919310

File tree

12 files changed

+223
-90
lines changed

12 files changed

+223
-90
lines changed

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

Lines changed: 15 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -66,70 +66,41 @@ class BIGNUM extends FunctionCall {
6666
// BN_CTX *BN_CTX_new(void);
6767
class BN_CTX_new extends CustomAllocator {
6868
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
69+
this.getQualifiedName().matches("BN\\_CTX_new%")
70+
or
71+
this.getQualifiedName().matches("BN\\_CTX\\_secure\\_new%") and
7872
dealloc instanceof BN_CTX_free
7973
}
8074
}
8175

8276
// void BN_CTX_free(BN_CTX *c);
8377
class BN_CTX_free extends CustomDeallocator {
84-
BN_CTX_free() { this.getName() = "BN_CTX_free" }
78+
BN_CTX_free() { this.getQualifiedName() = "BN_CTX_free" }
8579

8680
override int getPointer() { result = 0 }
8781
}
8882

8983
// 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-
}
84+
class BN_CTX_start extends FunctionCall {
85+
BN_CTX_start() { this.getTarget().getName() = "BN_CTX_start" }
9786

98-
Expr getContext() { result = this.(FunctionCall).getArgument(0) }
87+
Expr getContext() { result = this.getArgument(0) }
9988
}
10089

10190
// 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-
}
91+
class BN_CTX_end extends FunctionCall {
92+
BN_CTX_end() { this.getTarget().getName() = "BN_CTX_end" }
10993

110-
Expr getContext() { result = this.(FunctionCall).getArgument(0) }
94+
Expr getContext() { result = this.getArgument(0) }
11195
}
11296

11397
// 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-
}
98+
class BN_CTX_get extends FunctionCall {
99+
BN_CTX_get() { this.getTarget().getName() = "BN_CTX_get" }
121100

122-
Expr getContext() { result = this.(FunctionCall).getArgument(0) }
101+
Expr getContext() { result = this.getArgument(0) }
123102
}
124103

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-
}
104+
class BN_CTX extends FunctionCall {
105+
BN_CTX() { this.getTarget() instanceof BN_CTX_new }
135106
}

‎cpp/src/crypto/BnCtxFreeBeforeEnd.ql‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,21 @@
55
* @kind path-problem
66
* @tags correctness crypto
77
* @problem.severity error
8-
* @precision high
8+
* @precision medium
99
* @group cryptography
1010
*/
1111

1212
import cpp
1313
import trailofbits.crypto.libraries
1414
import semmle.code.cpp.dataflow.new.DataFlow
1515
import semmle.code.cpp.controlflow.ControlFlowGraph
16-
import DataFlow::PathGraph
1716

1817
/**
1918
* Tracks BN_CTX from BN_CTX_start to BN_CTX_free without going through BN_CTX_end
2019
*/
2120
module BnCtxFreeBeforeEndConfig implements DataFlow::ConfigSig {
2221
predicate isSource(DataFlow::Node source) {
23-
exists(BN_CTX_start start |
24-
source.asExpr() = start.getContext()
25-
)
22+
exists(BN_CTX_start start | source.asExpr() = start.getContext())
2623
}
2724

2825
predicate isSink(DataFlow::Node sink) {
@@ -34,22 +31,22 @@ module BnCtxFreeBeforeEndConfig implements DataFlow::ConfigSig {
3431

3532
predicate isBarrier(DataFlow::Node node) {
3633
// If the context flows through BN_CTX_end, it's properly handled
37-
exists(BN_CTX_end end |
38-
node.asExpr() = end.getContext()
39-
)
34+
exists(BN_CTX_end end | node.asExpr() = end.getContext())
4035
}
4136
}
4237

4338
module BnCtxFreeBeforeEndFlow = DataFlow::Global<BnCtxFreeBeforeEndConfig>;
4439

45-
from BnCtxFreeBeforeEndFlow::PathNode source, BnCtxFreeBeforeEndFlow::PathNode sink,
46-
BN_CTX_start start, CustomDeallocatorCall free
40+
import BnCtxFreeBeforeEndFlow::PathGraph
41+
42+
from
43+
BnCtxFreeBeforeEndFlow::PathNode source, BnCtxFreeBeforeEndFlow::PathNode sink,
44+
BN_CTX_start start, CustomDeallocatorCall free
4745
where
4846
BnCtxFreeBeforeEndFlow::flowPath(source, sink) and
4947
source.getNode().asExpr() = start.getContext() and
5048
sink.getNode().asExpr() = free.getPointer() and
5149
free.getTarget() instanceof BN_CTX_free
5250
select free, source, sink,
5351
"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()
52+
" before BN_CTX_end after BN_CTX_start at line " + start.getLocation().getStartLine().toString()

‎cpp/src/crypto/MissingBnCtxEnd.ql‎

Lines changed: 0 additions & 29 deletions
This file was deleted.

‎cpp/src/crypto/MissingZeroization.ql‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,18 @@ import semmle.code.cpp.dataflow.new.DataFlow
1515

1616
predicate isCleared(Expr bignum) {
1717
exists(BN_clear clear |
18-
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)))
18+
DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(clear.getBignum()))
19+
)
20+
or
21+
exists(CustomDeallocatorCall clear_free |
22+
clear_free.getTarget() instanceof BN_clear_free and
23+
DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(clear_free.getPointer()))
2224
)
2325
}
2426

2527
predicate isRandom(Expr bignum) {
2628
exists(BN_rand rand |
27-
DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(rand.getArgument(0)))
29+
DataFlow::localFlow(DataFlow::exprNode(bignum), DataFlow::exprNode(rand.getBignum()))
2830
)
2931
}
3032

‎cpp/src/crypto/UnbalancedBnCtx.ql‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/**
2+
* @name Unbalanced BN_CTX_start and BN_CTX_end pair
3+
* @id tob/cpp/missing-bn-ctx-end
4+
* @description Detects if one call in the BN_CTX_start/BN_CTX_end pair is missing
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()), DataFlow::exprNode(end.getContext()))
22+
)
23+
}
24+
25+
predicate hasMatchingStart(BN_CTX_end end) {
26+
exists(BN_CTX_start start, Function f |
27+
end.getEnclosingFunction() = f and
28+
start.getEnclosingFunction() = f and
29+
DataFlow::localFlow(DataFlow::exprNode(start.getContext()), DataFlow::exprNode(end.getContext()))
30+
)
31+
}
32+
33+
from FunctionCall call, string message
34+
where
35+
(
36+
call instanceof BN_CTX_start and
37+
not hasMatchingEnd(call) and
38+
message = "BN_CTX_start called without corresponding BN_CTX_end"
39+
) or
40+
(
41+
call instanceof BN_CTX_end and
42+
not hasMatchingStart(call) and
43+
message = "BN_CTX_end called without corresponding BN_CTX_start"
44+
)
45+
select call.getLocation(), message

‎cpp/test/include/openssl/bn.h‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,32 @@ int BN_rand(BIGNUM *rnd, int bits, int top, int bottom) {
3232
return 1;
3333
}
3434

35+
// BN_CTX functions
36+
#define BN_CTX int
37+
38+
BN_CTX *BN_CTX_new(void) {
39+
static BN_CTX ctx = 1;
40+
return &ctx;
41+
}
42+
43+
BN_CTX *BN_CTX_secure_new(void) {
44+
static BN_CTX ctx = 1;
45+
return &ctx;
46+
}
47+
48+
void BN_CTX_free(BN_CTX *c) {
49+
}
50+
51+
void BN_CTX_start(BN_CTX *ctx) {
52+
}
53+
54+
void BN_CTX_end(BN_CTX *ctx) {
55+
}
56+
57+
BIGNUM *BN_CTX_get(BN_CTX *ctx) {
58+
return &BN;
59+
}
60+
3561
# ifdef __cplusplus
3662
}
3763
#endif

‎cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/BnCtxFreeBeforeEnd.expected‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
edges
2+
| test.c:13:16:13:18 | ctx | test.c:15:15:15:17 | ctx | provenance | |
3+
| test.c:34:16:34:18 | ctx | test.c:40:15:40:17 | ctx | provenance | |
4+
nodes
5+
| test.c:13:16:13:18 | ctx | semmle.label | ctx |
6+
| test.c:15:15:15:17 | ctx | semmle.label | ctx |
7+
| test.c:34:16:34:18 | ctx | semmle.label | ctx |
8+
| test.c:40:15:40:17 | ctx | semmle.label | ctx |
9+
subpaths
10+
#select
11+
| 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 |
12+
| 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 |

‎cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/BnCtxFreeBeforeEnd.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
crypto/BnCtxFreeBeforeEnd.ql

‎cpp/test/query-tests/crypto/BnCtxFreeBeforeEnd/test.c‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
#include "../../../include/openssl/bn.h"
2+
3+
void good_proper_lifecycle() {
4+
BN_CTX *ctx = BN_CTX_new();
5+
BN_CTX_start(ctx);
6+
BIGNUM *a = BN_CTX_get(ctx);
7+
BN_CTX_end(ctx); // Properly call end before free
8+
BN_CTX_free(ctx);
9+
}
10+
11+
void bad_free_before_end() {
12+
BN_CTX *ctx = BN_CTX_new();
13+
BN_CTX_start(ctx);
14+
BIGNUM *a = BN_CTX_get(ctx);
15+
BN_CTX_free(ctx); // BUG: free before end
16+
}
17+
18+
void good_mult_ctx() {
19+
BN_CTX *ctx = BN_CTX_new();
20+
21+
BN_CTX_start(ctx);
22+
BIGNUM *a = BN_CTX_get(ctx);
23+
BN_CTX_end(ctx);
24+
25+
BN_CTX_start(ctx);
26+
BIGNUM *b = BN_CTX_get(ctx);
27+
BN_CTX_end(ctx);
28+
29+
BN_CTX_free(ctx);
30+
}
31+
32+
void bad_conditional_end(int condition) {
33+
BN_CTX *ctx = BN_CTX_new();
34+
BN_CTX_start(ctx);
35+
BIGNUM *a = BN_CTX_get(ctx);
36+
37+
if (condition) {
38+
BN_CTX_end(ctx);
39+
}
40+
BN_CTX_free(ctx); // BUG: May free before end if condition is false
41+
}
42+
43+
int main(void) {}

‎cpp/test/query-tests/crypto/UnbalancedBnCtx/UnbalancedBnCtx.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff 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 |

0 commit comments

Comments
 (0)