Skip to content

Commit 006d08f

Browse files
GrosQuilduGrosQuildu
committed
support globals
1 parent e71c518 commit 006d08f

3 files changed

Lines changed: 105 additions & 7 deletions

File tree

‎cpp/src/security/DecOverflowWhenComparing/DecOverflowWhenComparing.ql‎

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import semmle.code.cpp.controlflow.StackVariableReachability
1717
/**
1818
* Holds if `node` overwrites `var` (assignment or declaration with initializer).
1919
*/
20-
predicate isDefOf(ControlFlowNode node, StackVariable var) {
20+
predicate isDefOf(ControlFlowNode node, Variable var) {
2121
node = var.getAnAccess() and node.(VariableAccess).isLValue()
2222
or
2323
node.(DeclStmt).getADeclaration() = var and exists(var.getInitializer())
@@ -31,7 +31,7 @@ predicate isDefOf(ControlFlowNode node, StackVariable var) {
3131
pragma[nomagic]
3232
predicate isDecInComparison(
3333
PostfixDecrExpr dec, VariableAccess varAcc,
34-
ComparisonOperation cmp, StackVariable var
34+
ComparisonOperation cmp, Variable var
3535
) {
3636
varAcc = var.getAnAccess() and
3737
dec.getOperand() = varAcc.getExplicitlyConverted() and
@@ -46,14 +46,14 @@ predicate isDecInComparison(
4646
* (i.e., a use that could observe an overflowed value).
4747
*/
4848
pragma[nomagic]
49-
predicate isReadOf(VariableAccess va, StackVariable var) {
49+
predicate isReadOf(VariableAccess va, Variable var) {
5050
va = var.getAnAccess() and
5151
not exists(AssignExpr ae | ae.getLValue() = va)
5252
}
5353

5454
/**
5555
* Basic-block-level reachability from a decrement comparison to a use
56-
* of the same variable, blocked by any definition of the variable.
56+
* of the same stack variable, blocked by any definition of the variable.
5757
*/
5858
class DecOverflowReach extends StackVariableReachability {
5959
DecOverflowReach() { this = "DecOverflowReach" }
@@ -71,15 +71,62 @@ class DecOverflowReach extends StackVariableReachability {
7171
}
7272
}
7373

74+
/**
75+
* BB-level reachability for non-stack variables (globals, static locals).
76+
* Holds if `sink` is reachable from the entry of `bb` without crossing
77+
* a definition of `var`.
78+
*/
79+
private predicate nonStackBBEntryReaches(
80+
BasicBlock bb, Variable var, ControlFlowNode sink
81+
) {
82+
exists(int n |
83+
sink = bb.getNode(n) and
84+
isReadOf(sink, var) and
85+
not exists(int m | m < n | isDefOf(bb.getNode(m), var))
86+
)
87+
or
88+
not isDefOf(bb.getNode(_), var) and
89+
nonStackBBEntryReaches(bb.getASuccessor(), var, sink)
90+
}
91+
92+
/**
93+
* BB-level reachability from `source` to `sink` for non-stack variables,
94+
* without crossing a definition of `var`.
95+
*/
96+
pragma[nomagic]
97+
predicate nonStackReaches(
98+
ComparisonOperation source, Variable var, ControlFlowNode sink
99+
) {
100+
not var instanceof StackVariable and
101+
exists(BasicBlock bb, int i |
102+
bb.getNode(i) = source and
103+
not bb.isUnreachable()
104+
|
105+
exists(int j |
106+
j > i and
107+
sink = bb.getNode(j) and
108+
isReadOf(sink, var) and
109+
not exists(int k | k in [i + 1 .. j - 1] | isDefOf(bb.getNode(k), var))
110+
)
111+
or
112+
not exists(int k | k > i | isDefOf(bb.getNode(k), var)) and
113+
nonStackBBEntryReaches(bb.getASuccessor(), var, sink)
114+
)
115+
}
116+
74117
from
75-
SemanticStackVariable var, VariableAccess varAcc, PostfixDecrExpr dec,
118+
Variable var, VariableAccess varAcc, PostfixDecrExpr dec,
76119
VariableAccess varAccAfterOverflow, ComparisonOperation cmp
77120
where
78121
isDecInComparison(dec, varAcc, cmp, var) and
79122
isReadOf(varAccAfterOverflow, var) and
80123
varAcc != varAccAfterOverflow and
81-
// reachable without intervening overwrite (basic-block-level analysis)
82-
any(DecOverflowReach r).reaches(cmp, var, varAccAfterOverflow) and
124+
// reachable without intervening overwrite
125+
(
126+
any(DecOverflowReach r).reaches(cmp, var, varAccAfterOverflow)
127+
or
128+
nonStackReaches(cmp, var, varAccAfterOverflow)
129+
) and
83130
// exclude accesses that are part of the comparison expression itself
84131
not cmp.getAnOperand().getAChild*() = varAccAfterOverflow and
85132
// var-- > 0 (0 < var--) then only accesses in false branch matter

‎cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.c‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,54 @@ void overwrite_all_branches(uint32_t n, int *buf, int cond) {
148148
buf[n] = 0;
149149
}
150150

151+
// --- Non-stack variable tests ---
152+
153+
uint32_t g_count;
154+
155+
// BAD: global variable used after loop without overwrite
156+
void global_use_after_loop(int *buf) {
157+
while (g_count-- > 0) {
158+
buf[0] = 1;
159+
}
160+
buf[g_count] = 0; // BAD
161+
}
162+
163+
// BAD: global variable in if-neq pattern
164+
void global_use_after_neq(int *buf) {
165+
if (g_count-- != 0) {
166+
buf[0] = 1;
167+
}
168+
buf[g_count] = 0; // BAD
169+
}
170+
171+
// GOOD: global variable overwritten before use
172+
void global_overwrite(int *buf) {
173+
while (g_count-- > 0) {
174+
buf[0] = 1;
175+
}
176+
g_count = 42;
177+
buf[g_count] = 0;
178+
}
179+
180+
// BAD: static local variable used after loop
181+
void static_local_use(int *buf) {
182+
static uint32_t s_count;
183+
while (s_count-- > 0) {
184+
buf[0] = 1;
185+
}
186+
buf[s_count] = 0; // BAD
187+
}
188+
189+
// GOOD: static local variable overwritten before use
190+
void static_local_overwrite(int *buf) {
191+
static uint32_t s_count;
192+
while (s_count-- > 0) {
193+
buf[0] = 1;
194+
}
195+
s_count = 42;
196+
buf[s_count] = 0;
197+
}
198+
151199
int main() {
152200
const uint16_t datalen = 128;
153201
uint8_t data[datalen] = {};

‎cpp/test/query-tests/security/DecOverflowWhenComparing/DecOverflowWhenComparing.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,6 @@
55
| DecOverflowWhenComparing.c:100:9:100:11 | ... -- | Unsigned decrementation in comparison ($@) - $@ | DecOverflowWhenComparing.c:100:9:100:16 | ... == ... | ... == ... | DecOverflowWhenComparing.c:101:13:101:13 | n | n |
66
| DecOverflowWhenComparing.c:109:14:109:16 | ... -- | Unsigned decrementation in comparison ($@) - $@ | DecOverflowWhenComparing.c:109:14:109:20 | ... > ... | ... > ... | DecOverflowWhenComparing.c:110:9:110:9 | n | n |
77
| DecOverflowWhenComparing.c:115:9:115:11 | ... -- | Unsigned decrementation in comparison ($@) - $@ | DecOverflowWhenComparing.c:115:9:115:16 | ... != ... | ... != ... | DecOverflowWhenComparing.c:121:9:121:9 | n | n |
8+
| DecOverflowWhenComparing.c:157:12:157:20 | ... -- | Unsigned decrementation in comparison ($@) - $@ | DecOverflowWhenComparing.c:157:12:157:24 | ... > ... | ... > ... | DecOverflowWhenComparing.c:160:9:160:15 | g_count | g_count |
9+
| DecOverflowWhenComparing.c:165:9:165:17 | ... -- | Unsigned decrementation in comparison ($@) - $@ | DecOverflowWhenComparing.c:165:9:165:22 | ... != ... | ... != ... | DecOverflowWhenComparing.c:168:9:168:15 | g_count | g_count |
10+
| DecOverflowWhenComparing.c:183:12:183:20 | ... -- | Unsigned decrementation in comparison ($@) - $@ | DecOverflowWhenComparing.c:183:12:183:24 | ... > ... | ... > ... | DecOverflowWhenComparing.c:186:9:186:15 | s_count | s_count |

0 commit comments

Comments
 (0)