Skip to content

Commit a6937be

Browse files
geoffw0geoffw0
committed
Merge branch 'main' into sqltaint
2 parents b5bcbd3 + b794fcb commit a6937be

File tree

169 files changed

+16257
-4945
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

169 files changed

+16257
-4945
lines changed

‎CONTRIBUTING.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ If you have an idea for a query that you would like to share with other CodeQL u
3838

3939
- The queries and libraries must be autoformatted, for example using the "Format Document" command in [CodeQL for Visual Studio Code](https://help.semmle.com/codeql/codeql-for-vscode/procedures/about-codeql-for-vscode.html).
4040

41+
If you prefer, you can use this [pre-commit hook](misc/scripts/pre-commit) that automatically checks whether your files are correctly formatted. See the [pre-commit hook installation guide](docs/install-pre-commit-hook.md) for instructions on how to install the hook.
42+
4143
4. **Compilation**
4244

4345
- Compilation of the query and any associated libraries and tests must be resilient to future development of the [supported](docs/supported-queries.md) libraries. This means that the functionality cannot use internal libraries, cannot depend on the output of `getAQlClass`, and cannot make use of regexp matching on `toString`.

‎cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
2+
image::image(int width, int height)
3+
{
4+
int x, y;
5+
6+
// allocate width * height pixels
7+
pixels = new uint32_t[width * height];
8+
9+
// fill width * height pixels
10+
for (y = 0; y < height; y++)
11+
{
12+
for (x = 0; x < width; x++)
13+
{
14+
pixels[(y * width) + height] = 0;
15+
}
16+
}
17+
18+
// ...
19+
}

‎cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.qhelp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>The result of a multiplication is used in the size of an allocation. If the multiplication can be made to overflow, a much smaller amount of memory may be allocated than the rest of the code expects. This may lead to overflowing writes when the buffer is accessed later.</p>
8+
</overview>
9+
10+
<recommendation>
11+
<p>To fix this issue, ensure that the arithmetic used in the size of an allocation cannot overflow before memory is allocated.</p>
12+
</recommendation>
13+
14+
<example>
15+
<p>In the following example, an array of size <code>width * height</code> is allocated and stored as <code>pixels</code>. If <code>width</code> and <code>height</code> are set such that the multiplication overflows and wraps to a small value (say, 4) then the initialization code that follows the allocation will write beyond the end of the array.</p>
16+
<sample src="AllocMultiplicationOverflow.cpp"/>
17+
</example>
18+
19+
<references>
20+
<li>
21+
Cplusplus.com: <a href="http://www.cplusplus.com/articles/DE18T05o/">Integer overflow</a>.
22+
</li>
23+
</references>
24+
25+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/**
2+
* @name Multiplication result may overflow and be used in allocation
3+
* @description Using a multiplication result that may overflow in the size of an allocation may lead to buffer overflows when the allocated memory is used.
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @precision low
7+
* @tags security
8+
* correctness
9+
* external/cwe/cwe-190
10+
* external/cwe/cwe-128
11+
* @id cpp/multiplication-overflow-in-alloc
12+
*/
13+
14+
import cpp
15+
import semmle.code.cpp.models.interfaces.Allocation
16+
import semmle.code.cpp.dataflow.DataFlow
17+
import DataFlow::PathGraph
18+
19+
class MultToAllocConfig extends DataFlow::Configuration {
20+
MultToAllocConfig() { this = "MultToAllocConfig" }
21+
22+
override predicate isSource(DataFlow::Node node) {
23+
// a multiplication of two non-constant expressions
24+
exists(MulExpr me |
25+
me = node.asExpr() and
26+
forall(Expr e | e = me.getAnOperand() | not exists(e.getValue()))
27+
)
28+
}
29+
30+
override predicate isSink(DataFlow::Node node) {
31+
// something that affects an allocation size
32+
node.asExpr() = any(AllocationExpr ae).getSizeExpr().getAChild*()
33+
}
34+
}
35+
36+
from MultToAllocConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
37+
where config.hasFlowPath(source, sink)
38+
select sink, source, sink,
39+
"Potentially overflowing value from $@ is used in the size of this allocation.", source,
40+
"multiplication"

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.expected‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
edges
2+
| test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 |
3+
nodes
4+
| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
5+
| test.cpp:15:31:15:35 | ... * ... | semmle.label | ... * ... |
6+
| test.cpp:19:34:19:38 | ... * ... | semmle.label | ... * ... |
7+
| test.cpp:22:17:22:21 | ... * ... | semmle.label | ... * ... |
8+
| test.cpp:23:33:23:37 | size1 | semmle.label | size1 |
9+
| test.cpp:30:27:30:31 | ... * ... | semmle.label | ... * ... |
10+
| test.cpp:31:27:31:31 | ... * ... | semmle.label | ... * ... |
11+
#select
12+
| test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:13:33:13:37 | ... * ... | multiplication |
13+
| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | ... * ... | multiplication |
14+
| test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:19:34:19:38 | ... * ... | multiplication |
15+
| test.cpp:23:33:23:37 | size1 | test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:22:17:22:21 | ... * ... | multiplication |
16+
| test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:30:27:30:31 | ... * ... | multiplication |
17+
| test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:31:27:31:31 | ... * ... | multiplication |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/test.cpp‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
2+
typedef unsigned long size_t;
3+
void *malloc(size_t size);
4+
5+
int getAnInt();
6+
7+
void test()
8+
{
9+
int x = getAnInt();
10+
int y = getAnInt();
11+
12+
char *buffer1 = (char *)malloc(x + y); // GOOD
13+
char *buffer2 = (char *)malloc(x * y); // BAD
14+
int *buffer3 = (int *)malloc(x * sizeof(int)); // GOOD
15+
int *buffer4 = (int *)malloc(x * y * sizeof(int)); // BAD
16+
17+
if ((x <= 1000) && (y <= 1000))
18+
{
19+
char *buffer5 = (char *)malloc(x * y); // GOOD [FALSE POSITIVE]
20+
}
21+
22+
size_t size1 = x * y;
23+
char *buffer5 = (char *)malloc(size1); // BAD
24+
25+
size_t size2 = x;
26+
size2 *= y;
27+
char *buffer6 = (char *)malloc(size2); // BAD [NOT DETECTED]
28+
29+
char *buffer7 = new char[x * 10]; // GOOD
30+
char *buffer8 = new char[x * y]; // BAD
31+
char *buffer9 = new char[x * x]; // BAD
32+
}

‎cpp/ql/test/library-tests/dataflow/fields/conflated.cpp‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
int user_input();
2+
void sink(int);
3+
4+
struct A {
5+
int* p;
6+
int x;
7+
};
8+
9+
void pointer_without_allocation(const A& ra) {
10+
*ra.p = user_input();
11+
sink(*ra.p); // $ MISSING: ast,ir
12+
}
13+
14+
void argument_source(void*);
15+
void sink(void*);
16+
17+
void pointer_without_allocation_2() {
18+
char *raw;
19+
argument_source(raw);
20+
sink(raw); // $ ast MISSING: ir
21+
}
22+
23+
A* makeA() {
24+
return new A;
25+
}
26+
27+
void no_InitializeDynamicAllocation_instruction() {
28+
A* pa = makeA();
29+
pa->x = user_input();
30+
sink(pa->x); // $ ast MISSING: ir
31+
}
32+
33+
void fresh_or_arg(A* arg, bool unknown) {
34+
A* pa;
35+
pa = unknown ? arg : new A;
36+
pa->x = user_input();
37+
sink(pa->x); // $ ast MISSING: ir
38+
}
39+
40+
struct LinkedList {
41+
LinkedList* next;
42+
int y;
43+
44+
LinkedList() = default;
45+
LinkedList(LinkedList* next) : next(next) {}
46+
};
47+
48+
// Note: This example also suffers from #113: there is no ChiInstruction that merges the result of the
49+
// InitializeDynamicAllocation instruction into {AllAliasedMemory}. But even when that's fixed there's
50+
// still no dataflow because `ll->next->y = user_input()` writes to {AllAliasedMemory}.
51+
void too_many_indirections() {
52+
LinkedList* ll = new LinkedList;
53+
ll->next = new LinkedList;
54+
ll->next->y = user_input();
55+
sink(ll->next->y); // $ ast MISSING: ir
56+
}
57+
58+
void too_many_indirections_2(LinkedList* next) {
59+
LinkedList* ll = new LinkedList(next);
60+
ll->next->y = user_input();
61+
sink(ll->next->y); // $ ast MISSING: ir
62+
}

‎cpp/ql/test/library-tests/dataflow/fields/dataflow-consistency.expected‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,13 @@ postWithInFlow
121121
| by_reference.cpp:127:30:127:38 | inner_ptr [inner post update] | PostUpdateNode should not be the target of local flow. |
122122
| complex.cpp:11:22:11:23 | a_ [post update] | PostUpdateNode should not be the target of local flow. |
123123
| complex.cpp:12:22:12:23 | b_ [post update] | PostUpdateNode should not be the target of local flow. |
124+
| conflated.cpp:10:3:10:7 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
125+
| conflated.cpp:10:7:10:7 | p [inner post update] | PostUpdateNode should not be the target of local flow. |
126+
| conflated.cpp:29:7:29:7 | x [post update] | PostUpdateNode should not be the target of local flow. |
127+
| conflated.cpp:36:7:36:7 | x [post update] | PostUpdateNode should not be the target of local flow. |
128+
| conflated.cpp:53:7:53:10 | next [post update] | PostUpdateNode should not be the target of local flow. |
129+
| conflated.cpp:54:13:54:13 | y [post update] | PostUpdateNode should not be the target of local flow. |
130+
| conflated.cpp:60:13:60:13 | y [post update] | PostUpdateNode should not be the target of local flow. |
124131
| constructors.cpp:20:24:20:25 | a_ [post update] | PostUpdateNode should not be the target of local flow. |
125132
| constructors.cpp:21:24:21:25 | b_ [post update] | PostUpdateNode should not be the target of local flow. |
126133
| qualifiers.cpp:9:36:9:36 | a [post update] | PostUpdateNode should not be the target of local flow. |

‎cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
uniqueEnclosingCallable
22
uniqueType
33
uniqueNodeLocation
4+
| E.cpp:15:31:15:33 | buf | Node should have one location but has 2. |
5+
| aliasing.cpp:2:11:2:13 | (unnamed parameter 0) | Node should have one location but has 2. |
6+
| conflated.cpp:2:11:2:13 | (unnamed parameter 0) | Node should have one location but has 2. |
7+
| conflated.cpp:14:22:14:25 | buf | Node should have one location but has 2. |
48
| file://:0:0:0:0 | (unnamed parameter 0) | Node should have one location but has 0. |
59
| file://:0:0:0:0 | (unnamed parameter 0) | Node should have one location but has 0. |
610
| file://:0:0:0:0 | (unnamed parameter 0) | Node should have one location but has 0. |
@@ -129,6 +133,8 @@ postWithInFlow
129133
| complex.cpp:54:12:54:12 | Chi | PostUpdateNode should not be the target of local flow. |
130134
| complex.cpp:55:12:55:12 | Chi | PostUpdateNode should not be the target of local flow. |
131135
| complex.cpp:56:12:56:12 | Chi | PostUpdateNode should not be the target of local flow. |
136+
| conflated.cpp:45:39:45:42 | Chi | PostUpdateNode should not be the target of local flow. |
137+
| conflated.cpp:53:3:53:27 | Chi | PostUpdateNode should not be the target of local flow. |
132138
| constructors.cpp:20:24:20:29 | Chi | PostUpdateNode should not be the target of local flow. |
133139
| constructors.cpp:21:24:21:29 | Chi | PostUpdateNode should not be the target of local flow. |
134140
| constructors.cpp:23:28:23:28 | Chi | PostUpdateNode should not be the target of local flow. |

0 commit comments

Comments
 (0)