Skip to content

Commit a0992aa

Browse files
jbjjbj
authored
Merge pull request github#3062 from geoffw0/alloc-size
C++: Improve hasUpperBoundsCheck
2 parents 0ccf397 + 66a0b78 commit a0992aa

File tree

5 files changed

+161
-24
lines changed

5 files changed

+161
-24
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
2424
| No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
2525
| Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | | This query is no longer run on LGTM. |
2626
| No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings. This approach produces fewer, more accurate results. |
27+
| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | Cases where the tainted allocation size is range checked are now more reliably excluded. |
2728
| Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |
2829
| Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) | | This query is no longer run on LGTM. |
2930
| Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | More correct results | This query now also looks for comparisons of the form `0 <= x`. |

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,17 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
144144
}
145145

146146
/**
147-
* A variable that has any kind of upper-bound check anywhere in the program
147+
* A variable that has any kind of upper-bound check anywhere in the program. This is
148+
* biased towards being inclusive because there are a lot of valid ways of doing an
149+
* upper bounds checks if we don't consider where it occurs, for example:
150+
* ```
151+
* if (x < 10) { sink(x); }
152+
*
153+
* if (10 > y) { sink(y); }
154+
*
155+
* if (z > 10) { z = 10; }
156+
* sink(z);
157+
* ```
148158
*/
149159
// TODO: This coarse overapproximation, ported from the old taint tracking
150160
// library, could be replaced with an actual semantic check that a particular
@@ -153,10 +163,10 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
153163
// previously suppressed by this predicate by coincidence.
154164
private predicate hasUpperBoundsCheck(Variable var) {
155165
exists(RelationalOperation oper, VariableAccess access |
156-
oper.getLeftOperand() = access and
166+
oper.getAnOperand() = access and
157167
access.getTarget() = var and
158168
// Comparing to 0 is not an upper bound check
159-
not oper.getRightOperand().getValue() = "0"
169+
not oper.getAnOperand().getValue() = "0"
160170
)
161171
}
162172

‎cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,14 +328,24 @@ GlobalOrNamespaceVariable globalVarFromId(string id) {
328328
}
329329

330330
/**
331-
* A variable that has any kind of upper-bound check anywhere in the program
331+
* A variable that has any kind of upper-bound check anywhere in the program. This is
332+
* biased towards being inclusive because there are a lot of valid ways of doing an
333+
* upper bounds checks if we don't consider where it occurs, for example:
334+
* ```
335+
* if (x < 10) { sink(x); }
336+
*
337+
* if (10 > y) { sink(y); }
338+
*
339+
* if (z > 10) { z = 10; }
340+
* sink(z);
341+
* ```
332342
*/
333343
private predicate hasUpperBoundsCheck(Variable var) {
334344
exists(RelationalOperation oper, VariableAccess access |
335-
oper.getLeftOperand() = access and
345+
oper.getAnOperand() = access and
336346
access.getTarget() = var and
337347
// Comparing to 0 is not an upper bound check
338-
not oper.getRightOperand().getValue() = "0"
348+
not oper.getAnOperand().getValue() = "0"
339349
)
340350
}
341351

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

Lines changed: 63 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,36 @@ edges
3939
| test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted |
4040
| test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted |
4141
| test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted |
42-
| test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | (size_t)... |
43-
| test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | size |
44-
| test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | size |
45-
| test.cpp:123:25:123:38 | (const char *)... | test.cpp:127:24:127:27 | (size_t)... |
46-
| test.cpp:123:25:123:38 | (const char *)... | test.cpp:127:24:127:27 | size |
47-
| test.cpp:123:25:123:38 | (const char *)... | test.cpp:127:24:127:27 | size |
42+
| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | (unsigned long)... |
43+
| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | size |
44+
| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | size |
45+
| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... |
46+
| test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... |
47+
| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:27 | (unsigned long)... |
48+
| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:27 | size |
49+
| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:27 | size |
50+
| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:41 | ... * ... |
51+
| test.cpp:123:18:123:31 | (const char *)... | test.cpp:127:24:127:41 | ... * ... |
52+
| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | (unsigned long)... |
53+
| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | size |
54+
| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | size |
55+
| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... |
56+
| test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... |
57+
| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:13 | (unsigned long)... |
58+
| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:13 | size |
59+
| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:13 | size |
60+
| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:27 | ... * ... |
61+
| test.cpp:132:19:132:32 | (const char *)... | test.cpp:134:10:134:27 | ... * ... |
62+
| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | (unsigned long)... |
63+
| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | size |
64+
| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | size |
65+
| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:28 | ... * ... |
66+
| test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:28 | ... * ... |
67+
| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:14 | (unsigned long)... |
68+
| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:14 | size |
69+
| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:14 | size |
70+
| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:28 | ... * ... |
71+
| test.cpp:138:19:138:32 | (const char *)... | test.cpp:142:11:142:28 | ... * ... |
4872
nodes
4973
| test.cpp:39:21:39:24 | argv | semmle.label | argv |
5074
| test.cpp:39:21:39:24 | argv | semmle.label | argv |
@@ -80,13 +104,36 @@ nodes
80104
| test.cpp:52:54:52:60 | tainted | semmle.label | tainted |
81105
| test.cpp:52:54:52:60 | tainted | semmle.label | tainted |
82106
| test.cpp:52:54:52:60 | tainted | semmle.label | tainted |
83-
| test.cpp:123:25:123:30 | call to getenv | semmle.label | call to getenv |
84-
| test.cpp:123:25:123:38 | (const char *)... | semmle.label | (const char *)... |
85-
| test.cpp:127:24:127:27 | (size_t)... | semmle.label | (size_t)... |
86-
| test.cpp:127:24:127:27 | (size_t)... | semmle.label | (size_t)... |
107+
| test.cpp:123:18:123:23 | call to getenv | semmle.label | call to getenv |
108+
| test.cpp:123:18:123:31 | (const char *)... | semmle.label | (const char *)... |
109+
| test.cpp:127:24:127:27 | (unsigned long)... | semmle.label | (unsigned long)... |
110+
| test.cpp:127:24:127:27 | (unsigned long)... | semmle.label | (unsigned long)... |
87111
| test.cpp:127:24:127:27 | size | semmle.label | size |
88112
| test.cpp:127:24:127:27 | size | semmle.label | size |
89113
| test.cpp:127:24:127:27 | size | semmle.label | size |
114+
| test.cpp:127:24:127:41 | ... * ... | semmle.label | ... * ... |
115+
| test.cpp:127:24:127:41 | ... * ... | semmle.label | ... * ... |
116+
| test.cpp:127:24:127:41 | ... * ... | semmle.label | ... * ... |
117+
| test.cpp:132:19:132:24 | call to getenv | semmle.label | call to getenv |
118+
| test.cpp:132:19:132:32 | (const char *)... | semmle.label | (const char *)... |
119+
| test.cpp:134:10:134:13 | (unsigned long)... | semmle.label | (unsigned long)... |
120+
| test.cpp:134:10:134:13 | (unsigned long)... | semmle.label | (unsigned long)... |
121+
| test.cpp:134:10:134:13 | size | semmle.label | size |
122+
| test.cpp:134:10:134:13 | size | semmle.label | size |
123+
| test.cpp:134:10:134:13 | size | semmle.label | size |
124+
| test.cpp:134:10:134:27 | ... * ... | semmle.label | ... * ... |
125+
| test.cpp:134:10:134:27 | ... * ... | semmle.label | ... * ... |
126+
| test.cpp:134:10:134:27 | ... * ... | semmle.label | ... * ... |
127+
| test.cpp:138:19:138:24 | call to getenv | semmle.label | call to getenv |
128+
| test.cpp:138:19:138:32 | (const char *)... | semmle.label | (const char *)... |
129+
| test.cpp:142:11:142:14 | (unsigned long)... | semmle.label | (unsigned long)... |
130+
| test.cpp:142:11:142:14 | (unsigned long)... | semmle.label | (unsigned long)... |
131+
| test.cpp:142:11:142:14 | size | semmle.label | size |
132+
| test.cpp:142:11:142:14 | size | semmle.label | size |
133+
| test.cpp:142:11:142:14 | size | semmle.label | size |
134+
| test.cpp:142:11:142:28 | ... * ... | semmle.label | ... * ... |
135+
| test.cpp:142:11:142:28 | ... * ... | semmle.label | ... * ... |
136+
| test.cpp:142:11:142:28 | ... * ... | semmle.label | ... * ... |
90137
#select
91138
| 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) |
92139
| test.cpp:43:31:43:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:63 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
@@ -96,4 +143,9 @@ nodes
96143
| test.cpp:49:17:49:30 | new[] | test.cpp:39:21:39:24 | argv | test.cpp:49:26:49:29 | size | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
97144
| test.cpp:52:21:52:27 | call to realloc | test.cpp:39:21:39:24 | argv | test.cpp:52:35:52:60 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
98145
| test.cpp:52:35:52:60 | ... * ... | test.cpp:39:21:39:24 | argv | test.cpp:52:54:52:60 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
99-
| test.cpp:127:17:127:22 | call to malloc | test.cpp:123:25:123:30 | call to getenv | test.cpp:127:24:127:27 | size | This allocation size is derived from $@ and might overflow | test.cpp:123:25:123:30 | call to getenv | user input (getenv) |
146+
| test.cpp:127:17:127:22 | call to malloc | test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:41 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) |
147+
| test.cpp:127:24:127:41 | ... * ... | test.cpp:123:18:123:23 | call to getenv | test.cpp:127:24:127:27 | size | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) |
148+
| test.cpp:134:3:134:8 | call to malloc | test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) |
149+
| test.cpp:134:10:134:27 | ... * ... | test.cpp:132:19:132:24 | call to getenv | test.cpp:134:10:134:13 | size | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) |
150+
| test.cpp:142:4:142:9 | call to malloc | test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |
151+
| test.cpp:142:11:142:28 | ... * ... | test.cpp:138:19:138:24 | call to getenv | test.cpp:142:11:142:14 | size | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |

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

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,10 @@ int main(int argc, char **argv) {
3939
int tainted = atoi(argv[1]);
4040

4141
MyStruct *arr1 = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD
42-
MyStruct *arr2 = (MyStruct *)malloc(tainted); // BAD
42+
MyStruct *arr2 = (MyStruct *)malloc(tainted); // DUBIOUS (not multiplied by anything)
4343
MyStruct *arr3 = (MyStruct *)malloc(tainted * sizeof(MyStruct)); // BAD
4444
MyStruct *arr4 = (MyStruct *)malloc(getTainted() * sizeof(MyStruct)); // BAD [NOT DETECTED]
45-
MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // BAD [NOT DETECTED]
45+
MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // DUBIOUS (not multiplied by anything)
4646

4747
int size = tainted * 8;
4848
char *chars1 = (char *)malloc(size); // BAD
@@ -52,7 +52,7 @@ int main(int argc, char **argv) {
5252
arr1 = (MyStruct *)realloc(arr1, sizeof(MyStruct) * tainted); // BAD
5353

5454
size = 8;
55-
chars3 = new char[size]; // GOOD [FALSE POSITIVE]
55+
chars3 = new char[size]; // GOOD
5656

5757
return 0;
5858
}
@@ -120,9 +120,73 @@ int bounded(int x, int limit) {
120120
}
121121

122122
void open_file_bounded () {
123-
int size = size = atoi(getenv("USER"));
123+
int size = atoi(getenv("USER"));
124124
int bounded_size = bounded(size, MAX_SIZE);
125125

126-
int* a = (int*)malloc(bounded_size); // GOOD
127-
int* b = (int*)malloc(size); // BAD
128-
}
126+
int* a = (int*)malloc(bounded_size * sizeof(int)); // GOOD
127+
int* b = (int*)malloc(size * sizeof(int)); // BAD
128+
}
129+
130+
void more_bounded_tests() {
131+
{
132+
int size = atoi(getenv("USER"));
133+
134+
malloc(size * sizeof(int)); // BAD
135+
}
136+
137+
{
138+
int size = atoi(getenv("USER"));
139+
140+
if (size > 0)
141+
{
142+
malloc(size * sizeof(int)); // BAD
143+
}
144+
}
145+
146+
{
147+
int size = atoi(getenv("USER"));
148+
149+
if (size < 100)
150+
{
151+
malloc(size * sizeof(int)); // BAD [NOT DETECTED]
152+
}
153+
}
154+
155+
{
156+
int size = atoi(getenv("USER"));
157+
158+
if ((size > 0) && (size < 100))
159+
{
160+
malloc(size * sizeof(int)); // GOOD
161+
}
162+
}
163+
164+
{
165+
int size = atoi(getenv("USER"));
166+
167+
if ((100 > size) && (0 < size))
168+
{
169+
malloc(size * sizeof(int)); // GOOD
170+
}
171+
}
172+
173+
{
174+
int size = atoi(getenv("USER"));
175+
176+
malloc(size * sizeof(int)); // BAD [NOT DETECTED]
177+
178+
if ((size > 0) && (size < 100))
179+
{
180+
// ...
181+
}
182+
}
183+
184+
{
185+
int size = atoi(getenv("USER"));
186+
187+
if (size > 100)
188+
{
189+
malloc(size * sizeof(int)); // BAD [NOT DETECTED]
190+
}
191+
}
192+
}

0 commit comments

Comments
 (0)