Skip to content

Commit 13a67c9

Browse files
MathiasVPMathiasVP
authored
Merge pull request #4810 from geoffw0/multtoalloc
C++: Query for multiplications used in allocations.
2 parents 7db5a99 + dc4ca9b commit 13a67c9

File tree

6 files changed

+134
-0
lines changed

6 files changed

+134
-0
lines changed

‎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+
}

0 commit comments

Comments
 (0)