Skip to content

Commit 94ac2fd

Browse files
committed
Fix Allocation Size Overflow for use-use flow
We have an operator expression like `x * 5`. We want to follow where the value of the operator expression goes. We used to follow local flow from an operand, but now there is flow from that operand to the next use of the variable. The fix is to explicitly start local flow from the operator expression. There are also some expected edge changes due to use-use flow.
1 parent ca98c4a commit 94ac2fd

File tree

2 files changed

+28
-11
lines changed

2 files changed

+28
-11
lines changed

go/ql/lib/semmle/go/security/AllocationSizeOverflowCustomizations.qll

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,10 @@ module AllocationSizeOverflow {
3232
/**
3333
* A data-flow node that is an operand to an operation that may overflow.
3434
*/
35-
abstract class OverflowProneOperand extends DataFlow::Node { }
35+
abstract class OverflowProneOperand extends DataFlow::Node {
36+
/** Gets the operation that may overflow that `this` is an operand of. */
37+
abstract DataFlow::Node getOverflowProneOperation();
38+
}
3639

3740
/**
3841
* A data-flow node that represents the size argument of an allocation, such as the `n` in
@@ -91,14 +94,19 @@ module AllocationSizeOverflow {
9194
AllocationSize allocsz;
9295

9396
DefaultSink() {
94-
this instanceof OverflowProneOperand and
95-
localStep*(this, allocsz) and
97+
localStep*(this.(OverflowProneOperand).getOverflowProneOperation(), allocsz) and
9698
not allocsz instanceof AllocationSizeCheckBarrier
9799
}
98100

99101
override DataFlow::Node getAllocationSize() { result = allocsz }
100102
}
101103

104+
predicate foo(DataFlow::Node t, DataFlow::Node p) {
105+
t.hasLocationInfo(_, 34, _, _, _) and
106+
t instanceof OverflowProneOperand and
107+
localStep*(t, p)
108+
}
109+
102110
private predicate lengthCheck(DataFlow::Node g, Expr e, boolean branch) {
103111
exists(DataFlow::CallNode lesser |
104112
g.(DataFlow::RelationalComparisonNode).leq(branch, lesser, _, _)
@@ -134,15 +142,18 @@ module AllocationSizeOverflow {
134142

135143
/** An operand of an arithmetic expression that could cause overflow. */
136144
private class DefaultOverflowProneOperand extends OverflowProneOperand {
145+
OperatorExpr parent;
146+
137147
DefaultOverflowProneOperand() {
138-
exists(OperatorExpr parent | isOverflowProne(parent) |
139-
this.asExpr() = parent.getAnOperand() and
140-
// only consider outermost operands to avoid double reporting
141-
not exists(OperatorExpr grandparent | parent = grandparent.getAnOperand().stripParens() |
142-
isOverflowProne(grandparent)
143-
)
148+
isOverflowProne(parent) and
149+
this.asExpr() = parent.getAnOperand() and
150+
// only consider outermost operands to avoid double reporting
151+
not exists(OperatorExpr grandparent | parent = grandparent.getAnOperand().stripParens() |
152+
isOverflowProne(grandparent)
144153
)
145154
}
155+
156+
override DataFlow::Node getOverflowProneOperation() { result.asExpr() = parent }
146157
}
147158

148159
/**

go/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,13 @@ edges
66
| tst2.go:14:2:14:29 | ... := ...[0] | tst2.go:15:26:15:29 | data |
77
| tst2.go:15:26:15:29 | data | tst2.go:15:22:15:30 | call to len |
88
| tst3.go:6:2:6:31 | ... := ...[0] | tst3.go:7:26:7:33 | jsonData |
9-
| tst3.go:6:2:6:31 | ... := ...[0] | tst3.go:24:20:24:27 | jsonData |
10-
| tst3.go:6:2:6:31 | ... := ...[0] | tst3.go:32:20:32:27 | jsonData |
119
| tst3.go:7:26:7:33 | jsonData | tst3.go:7:22:7:34 | call to len |
10+
| tst3.go:7:26:7:33 | jsonData | tst3.go:9:32:9:39 | jsonData |
11+
| tst3.go:9:32:9:39 | jsonData | tst3.go:11:9:11:16 | jsonData |
12+
| tst3.go:11:9:11:16 | jsonData | tst3.go:16:20:16:27 | jsonData |
13+
| tst3.go:16:20:16:27 | jsonData | tst3.go:24:20:24:27 | jsonData |
1214
| tst3.go:24:20:24:27 | jsonData | tst3.go:24:16:24:28 | call to len |
15+
| tst3.go:24:20:24:27 | jsonData | tst3.go:32:20:32:27 | jsonData |
1316
| tst3.go:32:20:32:27 | jsonData | tst3.go:32:16:32:28 | call to len |
1417
| tst.go:14:2:14:30 | ... = ...[0] | tst.go:15:26:15:33 | jsonData |
1518
| tst.go:15:26:15:33 | jsonData | tst.go:15:22:15:34 | call to len |
@@ -32,6 +35,9 @@ nodes
3235
| tst3.go:6:2:6:31 | ... := ...[0] | semmle.label | ... := ...[0] |
3336
| tst3.go:7:22:7:34 | call to len | semmle.label | call to len |
3437
| tst3.go:7:26:7:33 | jsonData | semmle.label | jsonData |
38+
| tst3.go:9:32:9:39 | jsonData | semmle.label | jsonData |
39+
| tst3.go:11:9:11:16 | jsonData | semmle.label | jsonData |
40+
| tst3.go:16:20:16:27 | jsonData | semmle.label | jsonData |
3541
| tst3.go:24:16:24:28 | call to len | semmle.label | call to len |
3642
| tst3.go:24:20:24:27 | jsonData | semmle.label | jsonData |
3743
| tst3.go:32:16:32:28 | call to len | semmle.label | call to len |

0 commit comments

Comments
 (0)