Skip to content

Commit a25d9c7

Browse files
geoffw0geoffw0
authored
Merge pull request #17220 from paldepind/reuse-unbounded-in-tainted-allocation-size
C++: Reuse bounded predicate in TaintedAllocationSize query
2 parents 2933a3b + 1665bad commit a25d9c7

File tree

5 files changed

+73
-64
lines changed

5 files changed

+73
-64
lines changed

‎cpp/ql/src/Security/CWE/CWE-190/Bounded.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ private predicate boundedBitwiseAnd(Expr e, Expr andExpr, Expr operand1, Expr op
2424
* operation that may greatly reduce the range of possible values.
2525
*/
2626
predicate bounded(Expr e) {
27+
// There can be two separate reasons for `convertedExprMightOverflow` not holding:
28+
// 1. `e` really cannot overflow.
29+
// 2. `e` isn't analyzable.
30+
// If we didn't rule out case 2 we would declare anything that isn't analyzable as bounded.
2731
(
2832
e instanceof UnaryArithmeticOperation or
2933
e instanceof BinaryArithmeticOperation or

‎cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import semmle.code.cpp.ir.IR
2020
import semmle.code.cpp.controlflow.IRGuards
2121
import semmle.code.cpp.security.FlowSources
2222
import TaintedAllocationSize::PathGraph
23+
import Bounded
2324

2425
/**
2526
* Holds if `alloc` is an allocation, and `tainted` is a child of it that is a
@@ -61,16 +62,7 @@ module TaintedAllocationSizeConfig implements DataFlow::ConfigSig {
6162

6263
predicate isBarrier(DataFlow::Node node) {
6364
exists(Expr e | e = node.asExpr() |
64-
// There can be two separate reasons for `convertedExprMightOverflow` not holding:
65-
// 1. `e` really cannot overflow.
66-
// 2. `e` isn't analyzable.
67-
// If we didn't rule out case 2 we would place barriers on anything that isn't analyzable.
68-
(
69-
e instanceof UnaryArithmeticOperation or
70-
e instanceof BinaryArithmeticOperation or
71-
e instanceof AssignArithmeticOperation
72-
) and
73-
not convertedExprMightOverflow(e)
65+
bounded(e)
7466
or
7567
// Subtracting two pointers is either well-defined (and the result will likely be small), or
7668
// terribly undefined and dangerous. Here, we assume that the programmer has ensured that the

‎cpp/ql/src/change-notes/2024-08-16-uncontrolled-allocation-size.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `cpp/uncontrolled-allocation-size` ("Uncontrolled allocation size") query now considers arithmetic operations that might reduce the size of user input as a barrier. The query therefore produces fewer false positive results.

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

Lines changed: 54 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -13,26 +13,26 @@ edges
1313
| test.cpp:133:19:133:32 | *call to getenv | test.cpp:133:14:133:17 | call to atoi | provenance | TaintFunction |
1414
| test.cpp:148:15:148:18 | call to atol | test.cpp:152:11:152:28 | ... * ... | provenance | |
1515
| test.cpp:148:20:148:33 | *call to getenv | test.cpp:148:15:148:18 | call to atol | provenance | TaintFunction |
16-
| test.cpp:209:8:209:23 | *get_tainted_size | test.cpp:241:9:241:24 | call to get_tainted_size | provenance | |
17-
| test.cpp:211:9:211:42 | ... * ... | test.cpp:209:8:209:23 | *get_tainted_size | provenance | |
18-
| test.cpp:211:14:211:27 | *call to getenv | test.cpp:211:9:211:42 | ... * ... | provenance | TaintFunction |
19-
| test.cpp:230:21:230:21 | s | test.cpp:231:21:231:21 | s | provenance | |
20-
| test.cpp:237:19:237:52 | ... * ... | test.cpp:239:9:239:18 | local_size | provenance | |
21-
| test.cpp:237:19:237:52 | ... * ... | test.cpp:245:11:245:20 | local_size | provenance | |
22-
| test.cpp:237:19:237:52 | ... * ... | test.cpp:247:10:247:19 | local_size | provenance | |
23-
| test.cpp:237:24:237:37 | *call to getenv | test.cpp:237:19:237:52 | ... * ... | provenance | TaintFunction |
24-
| test.cpp:247:10:247:19 | local_size | test.cpp:230:21:230:21 | s | provenance | |
25-
| test.cpp:250:20:250:27 | *out_size | test.cpp:289:17:289:20 | get_size output argument | provenance | |
26-
| test.cpp:250:20:250:27 | *out_size | test.cpp:305:18:305:21 | get_size output argument | provenance | |
27-
| test.cpp:251:2:251:32 | ... = ... | test.cpp:250:20:250:27 | *out_size | provenance | |
28-
| test.cpp:251:18:251:31 | *call to getenv | test.cpp:251:2:251:32 | ... = ... | provenance | TaintFunction |
29-
| test.cpp:259:15:259:18 | call to atoi | test.cpp:263:11:263:29 | ... * ... | provenance | |
30-
| test.cpp:259:20:259:33 | *call to getenv | test.cpp:259:15:259:18 | call to atoi | provenance | TaintFunction |
31-
| test.cpp:289:17:289:20 | get_size output argument | test.cpp:291:11:291:28 | ... * ... | provenance | |
32-
| test.cpp:305:18:305:21 | get_size output argument | test.cpp:308:10:308:27 | ... * ... | provenance | |
33-
| test.cpp:353:13:353:16 | call to atoi | test.cpp:355:35:355:38 | size | provenance | |
34-
| test.cpp:353:13:353:16 | call to atoi | test.cpp:356:35:356:38 | size | provenance | |
35-
| test.cpp:353:18:353:31 | *call to getenv | test.cpp:353:13:353:16 | call to atoi | provenance | TaintFunction |
16+
| test.cpp:218:8:218:23 | *get_tainted_size | test.cpp:250:9:250:24 | call to get_tainted_size | provenance | |
17+
| test.cpp:220:9:220:42 | ... * ... | test.cpp:218:8:218:23 | *get_tainted_size | provenance | |
18+
| test.cpp:220:14:220:27 | *call to getenv | test.cpp:220:9:220:42 | ... * ... | provenance | TaintFunction |
19+
| test.cpp:239:21:239:21 | s | test.cpp:240:21:240:21 | s | provenance | |
20+
| test.cpp:246:19:246:52 | ... * ... | test.cpp:248:9:248:18 | local_size | provenance | |
21+
| test.cpp:246:19:246:52 | ... * ... | test.cpp:254:11:254:20 | local_size | provenance | |
22+
| test.cpp:246:19:246:52 | ... * ... | test.cpp:256:10:256:19 | local_size | provenance | |
23+
| test.cpp:246:24:246:37 | *call to getenv | test.cpp:246:19:246:52 | ... * ... | provenance | TaintFunction |
24+
| test.cpp:256:10:256:19 | local_size | test.cpp:239:21:239:21 | s | provenance | |
25+
| test.cpp:259:20:259:27 | *out_size | test.cpp:298:17:298:20 | get_size output argument | provenance | |
26+
| test.cpp:259:20:259:27 | *out_size | test.cpp:314:18:314:21 | get_size output argument | provenance | |
27+
| test.cpp:260:2:260:32 | ... = ... | test.cpp:259:20:259:27 | *out_size | provenance | |
28+
| test.cpp:260:18:260:31 | *call to getenv | test.cpp:260:2:260:32 | ... = ... | provenance | TaintFunction |
29+
| test.cpp:268:15:268:18 | call to atoi | test.cpp:272:11:272:29 | ... * ... | provenance | |
30+
| test.cpp:268:20:268:33 | *call to getenv | test.cpp:268:15:268:18 | call to atoi | provenance | TaintFunction |
31+
| test.cpp:298:17:298:20 | get_size output argument | test.cpp:300:11:300:28 | ... * ... | provenance | |
32+
| test.cpp:314:18:314:21 | get_size output argument | test.cpp:317:10:317:27 | ... * ... | provenance | |
33+
| test.cpp:362:13:362:16 | call to atoi | test.cpp:364:35:364:38 | size | provenance | |
34+
| test.cpp:362:13:362:16 | call to atoi | test.cpp:365:35:365:38 | size | provenance | |
35+
| test.cpp:362:18:362:31 | *call to getenv | test.cpp:362:13:362:16 | call to atoi | provenance | TaintFunction |
3636
nodes
3737
| test.cpp:39:27:39:30 | **argv | semmle.label | **argv |
3838
| test.cpp:40:16:40:19 | call to atoi | semmle.label | call to atoi |
@@ -52,31 +52,31 @@ nodes
5252
| test.cpp:148:15:148:18 | call to atol | semmle.label | call to atol |
5353
| test.cpp:148:20:148:33 | *call to getenv | semmle.label | *call to getenv |
5454
| test.cpp:152:11:152:28 | ... * ... | semmle.label | ... * ... |
55-
| test.cpp:209:8:209:23 | *get_tainted_size | semmle.label | *get_tainted_size |
56-
| test.cpp:211:9:211:42 | ... * ... | semmle.label | ... * ... |
57-
| test.cpp:211:14:211:27 | *call to getenv | semmle.label | *call to getenv |
58-
| test.cpp:230:21:230:21 | s | semmle.label | s |
59-
| test.cpp:231:21:231:21 | s | semmle.label | s |
60-
| test.cpp:237:19:237:52 | ... * ... | semmle.label | ... * ... |
61-
| test.cpp:237:24:237:37 | *call to getenv | semmle.label | *call to getenv |
62-
| test.cpp:239:9:239:18 | local_size | semmle.label | local_size |
63-
| test.cpp:241:9:241:24 | call to get_tainted_size | semmle.label | call to get_tainted_size |
64-
| test.cpp:245:11:245:20 | local_size | semmle.label | local_size |
65-
| test.cpp:247:10:247:19 | local_size | semmle.label | local_size |
66-
| test.cpp:250:20:250:27 | *out_size | semmle.label | *out_size |
67-
| test.cpp:251:2:251:32 | ... = ... | semmle.label | ... = ... |
68-
| test.cpp:251:18:251:31 | *call to getenv | semmle.label | *call to getenv |
69-
| test.cpp:259:15:259:18 | call to atoi | semmle.label | call to atoi |
70-
| test.cpp:259:20:259:33 | *call to getenv | semmle.label | *call to getenv |
71-
| test.cpp:263:11:263:29 | ... * ... | semmle.label | ... * ... |
72-
| test.cpp:289:17:289:20 | get_size output argument | semmle.label | get_size output argument |
73-
| test.cpp:291:11:291:28 | ... * ... | semmle.label | ... * ... |
74-
| test.cpp:305:18:305:21 | get_size output argument | semmle.label | get_size output argument |
75-
| test.cpp:308:10:308:27 | ... * ... | semmle.label | ... * ... |
76-
| test.cpp:353:13:353:16 | call to atoi | semmle.label | call to atoi |
77-
| test.cpp:353:18:353:31 | *call to getenv | semmle.label | *call to getenv |
78-
| test.cpp:355:35:355:38 | size | semmle.label | size |
79-
| test.cpp:356:35:356:38 | size | semmle.label | size |
55+
| test.cpp:218:8:218:23 | *get_tainted_size | semmle.label | *get_tainted_size |
56+
| test.cpp:220:9:220:42 | ... * ... | semmle.label | ... * ... |
57+
| test.cpp:220:14:220:27 | *call to getenv | semmle.label | *call to getenv |
58+
| test.cpp:239:21:239:21 | s | semmle.label | s |
59+
| test.cpp:240:21:240:21 | s | semmle.label | s |
60+
| test.cpp:246:19:246:52 | ... * ... | semmle.label | ... * ... |
61+
| test.cpp:246:24:246:37 | *call to getenv | semmle.label | *call to getenv |
62+
| test.cpp:248:9:248:18 | local_size | semmle.label | local_size |
63+
| test.cpp:250:9:250:24 | call to get_tainted_size | semmle.label | call to get_tainted_size |
64+
| test.cpp:254:11:254:20 | local_size | semmle.label | local_size |
65+
| test.cpp:256:10:256:19 | local_size | semmle.label | local_size |
66+
| test.cpp:259:20:259:27 | *out_size | semmle.label | *out_size |
67+
| test.cpp:260:2:260:32 | ... = ... | semmle.label | ... = ... |
68+
| test.cpp:260:18:260:31 | *call to getenv | semmle.label | *call to getenv |
69+
| test.cpp:268:15:268:18 | call to atoi | semmle.label | call to atoi |
70+
| test.cpp:268:20:268:33 | *call to getenv | semmle.label | *call to getenv |
71+
| test.cpp:272:11:272:29 | ... * ... | semmle.label | ... * ... |
72+
| test.cpp:298:17:298:20 | get_size output argument | semmle.label | get_size output argument |
73+
| test.cpp:300:11:300:28 | ... * ... | semmle.label | ... * ... |
74+
| test.cpp:314:18:314:21 | get_size output argument | semmle.label | get_size output argument |
75+
| test.cpp:317:10:317:27 | ... * ... | semmle.label | ... * ... |
76+
| test.cpp:362:13:362:16 | call to atoi | semmle.label | call to atoi |
77+
| test.cpp:362:18:362:31 | *call to getenv | semmle.label | *call to getenv |
78+
| test.cpp:364:35:364:38 | size | semmle.label | size |
79+
| test.cpp:365:35:365:38 | size | semmle.label | size |
8080
subpaths
8181
#select
8282
| test.cpp:43:31:43:36 | call to malloc | test.cpp:39:27:39:30 | **argv | test.cpp:43:38:43:44 | tainted | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
@@ -88,12 +88,12 @@ subpaths
8888
| test.cpp:128:17:128:22 | call to malloc | test.cpp:124:18:124:31 | *call to getenv | test.cpp:128:24:128:41 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:124:18:124:31 | *call to getenv | user input (an environment variable) |
8989
| test.cpp:135:3:135:8 | call to malloc | test.cpp:133:19:133:32 | *call to getenv | test.cpp:135:10:135:27 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:133:19:133:32 | *call to getenv | user input (an environment variable) |
9090
| test.cpp:152:4:152:9 | call to malloc | test.cpp:148:20:148:33 | *call to getenv | test.cpp:152:11:152:28 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:148:20:148:33 | *call to getenv | user input (an environment variable) |
91-
| test.cpp:231:14:231:19 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:231:21:231:21 | s | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
92-
| test.cpp:239:2:239:7 | call to malloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:239:9:239:18 | local_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
93-
| test.cpp:241:2:241:7 | call to malloc | test.cpp:211:14:211:27 | *call to getenv | test.cpp:241:9:241:24 | call to get_tainted_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:211:14:211:27 | *call to getenv | user input (an environment variable) |
94-
| test.cpp:245:2:245:9 | call to my_alloc | test.cpp:237:24:237:37 | *call to getenv | test.cpp:245:11:245:20 | local_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:237:24:237:37 | *call to getenv | user input (an environment variable) |
95-
| test.cpp:263:4:263:9 | call to malloc | test.cpp:259:20:259:33 | *call to getenv | test.cpp:263:11:263:29 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:259:20:259:33 | *call to getenv | user input (an environment variable) |
96-
| test.cpp:291:4:291:9 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:291:11:291:28 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) |
97-
| test.cpp:308:3:308:8 | call to malloc | test.cpp:251:18:251:31 | *call to getenv | test.cpp:308:10:308:27 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:251:18:251:31 | *call to getenv | user input (an environment variable) |
98-
| test.cpp:355:25:355:33 | call to MyMalloc1 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:355:35:355:38 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) |
99-
| test.cpp:356:25:356:33 | call to MyMalloc2 | test.cpp:353:18:353:31 | *call to getenv | test.cpp:356:35:356:38 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:353:18:353:31 | *call to getenv | user input (an environment variable) |
91+
| test.cpp:240:14:240:19 | call to malloc | test.cpp:246:24:246:37 | *call to getenv | test.cpp:240:21:240:21 | s | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:246:24:246:37 | *call to getenv | user input (an environment variable) |
92+
| test.cpp:248:2:248:7 | call to malloc | test.cpp:246:24:246:37 | *call to getenv | test.cpp:248:9:248:18 | local_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:246:24:246:37 | *call to getenv | user input (an environment variable) |
93+
| test.cpp:250:2:250:7 | call to malloc | test.cpp:220:14:220:27 | *call to getenv | test.cpp:250:9:250:24 | call to get_tainted_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:220:14:220:27 | *call to getenv | user input (an environment variable) |
94+
| test.cpp:254:2:254:9 | call to my_alloc | test.cpp:246:24:246:37 | *call to getenv | test.cpp:254:11:254:20 | local_size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:246:24:246:37 | *call to getenv | user input (an environment variable) |
95+
| test.cpp:272:4:272:9 | call to malloc | test.cpp:268:20:268:33 | *call to getenv | test.cpp:272:11:272:29 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:268:20:268:33 | *call to getenv | user input (an environment variable) |
96+
| test.cpp:300:4:300:9 | call to malloc | test.cpp:260:18:260:31 | *call to getenv | test.cpp:300:11:300:28 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:260:18:260:31 | *call to getenv | user input (an environment variable) |
97+
| test.cpp:317:3:317:8 | call to malloc | test.cpp:260:18:260:31 | *call to getenv | test.cpp:317:10:317:27 | ... * ... | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:260:18:260:31 | *call to getenv | user input (an environment variable) |
98+
| test.cpp:364:25:364:33 | call to MyMalloc1 | test.cpp:362:18:362:31 | *call to getenv | test.cpp:364:35:364:38 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:362:18:362:31 | *call to getenv | user input (an environment variable) |
99+
| test.cpp:365:25:365:33 | call to MyMalloc2 | test.cpp:362:18:362:31 | *call to getenv | test.cpp:365:35:365:38 | size | This allocation size is derived from $@ and could allocate arbitrary amounts of memory. | test.cpp:362:18:362:31 | *call to getenv | user input (an environment variable) |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,15 @@ void more_bounded_tests() {
180180
}
181181
}
182182

183+
{
184+
int size = atoi(getenv("USER"));
185+
186+
if (size % 100)
187+
{
188+
malloc(size * sizeof(int)); // GOOD
189+
}
190+
}
191+
183192
{
184193
int size = atoi(getenv("USER"));
185194

0 commit comments

Comments
 (0)