Skip to content

Commit bc09720

Browse files
jbjjbj
authored
Merge pull request github#3479 from geoffw0/fp2762
C++: Allow equality to block taint (security taint tracking)
2 parents 3d58e6f + 9babd5d commit bc09720

File tree

6 files changed

+156
-2
lines changed

6 files changed

+156
-2
lines changed

‎change-notes/1.25/analysis-cpp.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,3 +39,5 @@ The following changes in version 1.25 affect C/C++ analysis in all applications.
3939
}
4040
};
4141
```
42+
* The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) now considers that equality checks may block the flow of taint. This results in fewer false positive results from queries that use this library.
43+

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ private import semmle.code.cpp.ir.dataflow.DataFlow2
55
private import semmle.code.cpp.ir.dataflow.DataFlow3
66
private import semmle.code.cpp.ir.IR
77
private import semmle.code.cpp.ir.dataflow.internal.DataFlowDispatch as Dispatch
8+
private import semmle.code.cpp.controlflow.IRGuards
89
private import semmle.code.cpp.models.interfaces.Taint
910
private import semmle.code.cpp.models.interfaces.DataFlow
1011

@@ -170,11 +171,34 @@ private predicate hasUpperBoundsCheck(Variable var) {
170171
)
171172
}
172173

174+
private predicate nodeIsBarrierEqualityCandidate(
175+
DataFlow::Node node, Operand access, Variable checkedVar
176+
) {
177+
readsVariable(node.asInstruction(), checkedVar) and
178+
any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
179+
}
180+
173181
private predicate nodeIsBarrier(DataFlow::Node node) {
174182
exists(Variable checkedVar |
175183
readsVariable(node.asInstruction(), checkedVar) and
176184
hasUpperBoundsCheck(checkedVar)
177185
)
186+
or
187+
exists(Variable checkedVar, Operand access |
188+
/*
189+
* This node is guarded by a condition that forces the accessed variable
190+
* to equal something else. For example:
191+
* ```
192+
* x = taintsource()
193+
* if (x == 10) {
194+
* taintsink(x); // not considered tainted
195+
* }
196+
* ```
197+
*/
198+
199+
nodeIsBarrierEqualityCandidate(node, access, checkedVar) and
200+
readsVariable(access.getDef(), checkedVar)
201+
)
178202
}
179203

180204
private predicate nodeIsBarrierIn(DataFlow::Node node) {

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,26 @@ edges
6868
| test.cpp:227:24:227:37 | (const char *)... | test.cpp:237:10:237:19 | (size_t)... |
6969
| test.cpp:235:11:235:20 | (size_t)... | test.cpp:214:23:214:23 | s |
7070
| test.cpp:237:10:237:19 | (size_t)... | test.cpp:220:21:220:21 | s |
71+
| test.cpp:241:2:241:32 | Chi | test.cpp:279:17:279:20 | get_size output argument |
72+
| test.cpp:241:2:241:32 | Chi | test.cpp:295:18:295:21 | get_size output argument |
73+
| test.cpp:241:18:241:23 | call to getenv | test.cpp:241:2:241:32 | Chi |
74+
| test.cpp:241:18:241:31 | (const char *)... | test.cpp:241:2:241:32 | Chi |
75+
| test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... |
76+
| test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... |
77+
| test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... |
78+
| test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... |
79+
| test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... |
80+
| test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... |
81+
| test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... |
82+
| test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... |
83+
| test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... |
84+
| test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... |
85+
| test.cpp:301:19:301:32 | (const char *)... | test.cpp:305:11:305:28 | ... * ... |
86+
| test.cpp:301:19:301:32 | (const char *)... | test.cpp:305:11:305:28 | ... * ... |
87+
| test.cpp:309:19:309:24 | call to getenv | test.cpp:314:10:314:27 | ... * ... |
88+
| test.cpp:309:19:309:24 | call to getenv | test.cpp:314:10:314:27 | ... * ... |
89+
| test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... |
90+
| test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... |
7191
nodes
7292
| field_conflation.c:12:22:12:27 | call to getenv | semmle.label | call to getenv |
7393
| field_conflation.c:12:22:12:34 | (const char *)... | semmle.label | (const char *)... |
@@ -140,6 +160,32 @@ nodes
140160
| test.cpp:231:9:231:24 | call to get_tainted_size | semmle.label | call to get_tainted_size |
141161
| test.cpp:235:11:235:20 | (size_t)... | semmle.label | (size_t)... |
142162
| test.cpp:237:10:237:19 | (size_t)... | semmle.label | (size_t)... |
163+
| test.cpp:241:2:241:32 | Chi | semmle.label | Chi |
164+
| test.cpp:241:18:241:23 | call to getenv | semmle.label | call to getenv |
165+
| test.cpp:241:18:241:31 | (const char *)... | semmle.label | (const char *)... |
166+
| test.cpp:249:20:249:25 | call to getenv | semmle.label | call to getenv |
167+
| test.cpp:249:20:249:33 | (const char *)... | semmle.label | (const char *)... |
168+
| test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... |
169+
| test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... |
170+
| test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... |
171+
| test.cpp:279:17:279:20 | get_size output argument | semmle.label | get_size output argument |
172+
| test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... |
173+
| test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... |
174+
| test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... |
175+
| test.cpp:295:18:295:21 | get_size output argument | semmle.label | get_size output argument |
176+
| test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... |
177+
| test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... |
178+
| test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... |
179+
| test.cpp:301:19:301:24 | call to getenv | semmle.label | call to getenv |
180+
| test.cpp:301:19:301:32 | (const char *)... | semmle.label | (const char *)... |
181+
| test.cpp:305:11:305:28 | ... * ... | semmle.label | ... * ... |
182+
| test.cpp:305:11:305:28 | ... * ... | semmle.label | ... * ... |
183+
| test.cpp:305:11:305:28 | ... * ... | semmle.label | ... * ... |
184+
| test.cpp:309:19:309:24 | call to getenv | semmle.label | call to getenv |
185+
| test.cpp:309:19:309:32 | (const char *)... | semmle.label | (const char *)... |
186+
| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... |
187+
| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... |
188+
| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... |
143189
#select
144190
| field_conflation.c:20:3:20:8 | call to malloc | field_conflation.c:12:22:12:27 | call to getenv | field_conflation.c:20:13:20:13 | x | This allocation size is derived from $@ and might overflow | field_conflation.c:12:22:12:27 | call to getenv | user input (getenv) |
145191
| test.cpp:42:31:42:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
@@ -155,3 +201,8 @@ nodes
155201
| test.cpp:221:14:221:19 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:221:21:221:21 | s | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) |
156202
| test.cpp:229:2:229:7 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | local_size | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) |
157203
| test.cpp:231:2:231:7 | call to malloc | test.cpp:201:14:201:19 | call to getenv | test.cpp:231:9:231:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow | test.cpp:201:14:201:19 | call to getenv | user input (getenv) |
204+
| test.cpp:253:4:253:9 | call to malloc | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:249:20:249:25 | call to getenv | user input (getenv) |
205+
| test.cpp:281:4:281:9 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:281:11:281:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) |
206+
| test.cpp:298:3:298:8 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:298:10:298:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) |
207+
| test.cpp:305:4:305:9 | call to malloc | test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:301:19:301:24 | call to getenv | user input (getenv) |
208+
| test.cpp:314:3:314:8 | call to malloc | test.cpp:309:19:309:24 | call to getenv | test.cpp:314:10:314:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:309:19:309:24 | call to getenv | user input (getenv) |

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,3 +236,81 @@ void more_cases() {
236236
my_func(100); // GOOD
237237
my_func(local_size); // GOOD
238238
}
239+
240+
bool get_size(int &out_size) {
241+
out_size = atoi(getenv("USER"));
242+
243+
return true;
244+
}
245+
246+
void equality_cases() {
247+
{
248+
int size1 = atoi(getenv("USER"));
249+
int size2 = atoi(getenv("USER"));
250+
251+
if (size1 == 100)
252+
{
253+
malloc(size2 * sizeof(int)); // BAD
254+
}
255+
if (size2 == 100)
256+
{
257+
malloc(size2 * sizeof(int)); // GOOD
258+
}
259+
}
260+
{
261+
int size = atoi(getenv("USER"));
262+
263+
if (size != 100)
264+
return;
265+
266+
malloc(size * sizeof(int)); // GOOD
267+
}
268+
{
269+
int size;
270+
271+
if ((get_size(size)) && (size == 100))
272+
{
273+
malloc(size * sizeof(int)); // GOOD
274+
}
275+
}
276+
{
277+
int size;
278+
279+
if ((get_size(size)) && (size != 100))
280+
{
281+
malloc(size * sizeof(int)); // BAD
282+
}
283+
}
284+
{
285+
int size;
286+
287+
if ((!get_size(size)) || (size != 100))
288+
return;
289+
290+
malloc(size * sizeof(int)); // GOOD
291+
}
292+
{
293+
int size;
294+
295+
if ((!get_size(size)) || (size == 100))
296+
return;
297+
298+
malloc(size * sizeof(int)); // BAD
299+
}
300+
{
301+
int size = atoi(getenv("USER"));
302+
303+
if ((size == 50) || (size == 100))
304+
{
305+
malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE]
306+
}
307+
}
308+
{
309+
int size = atoi(getenv("USER"));
310+
311+
if (size != 50 && size != 100)
312+
return;
313+
314+
malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE]
315+
}
316+
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,4 @@
55
| test.c:63:3:63:5 | sc8 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:62:9:62:16 | - ... | Extreme value |
66
| test.c:75:3:75:5 | sc1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:74:9:74:16 | 127 | Extreme value |
77
| test.c:76:3:76:5 | sc1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:74:9:74:16 | 127 | Extreme value |
8-
| test.c:114:9:114:9 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:108:17:108:23 | 2147483647 | Extreme value |
98
| test.c:124:9:124:9 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:118:17:118:23 | 2147483647 | Extreme value |

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ void test_guards3(int cond) {
111111

112112
if (x != 0) return;
113113

114-
return x + 1; // GOOD [FALSE POSITIVE]
114+
return x + 1; // GOOD
115115
}
116116

117117
void test_guards4(int cond) {

0 commit comments

Comments
 (0)