Skip to content

Commit c2dfbd4

Browse files
rdmarsh2rdmarsh2
authored
Merge pull request #10398 from MathiasVP/further-work-on-buffer-over-queries
C++: Further work on buffer-overflow queries
2 parents 8c13738 + 639aaff commit c2dfbd4

File tree

6 files changed

+206
-43
lines changed

6 files changed

+206
-43
lines changed

‎cpp/ql/src/experimental/Likely Bugs/ArrayAccessProductFlow.ql

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
/**
2+
* @name Off-by-one in array access
3+
* @description TODO
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @id cpp/off-by-one-array-access
7+
* @tags reliability
8+
* security
9+
*/
10+
111
import cpp
212
import experimental.semmle.code.cpp.dataflow.ProductFlow
313
import experimental.semmle.code.cpp.semantic.analysis.RangeAnalysis
@@ -7,6 +17,21 @@ import semmle.code.cpp.ir.IR
717
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
818
import semmle.code.cpp.models.interfaces.Allocation
919
import semmle.code.cpp.ir.IRConfiguration
20+
import DataFlow::PathGraph
21+
22+
// temporary - custom allocator for ffmpeg
23+
class AvBufferAlloc extends AllocationFunction {
24+
AvBufferAlloc() { this.hasGlobalName(["av_mallocz", "av_malloc"]) }
25+
26+
override int getSizeArg() { result = 0 }
27+
}
28+
29+
// temporary - custom allocator for php
30+
class PhpEmalloc extends AllocationFunction {
31+
PhpEmalloc() { this.hasGlobalName(["_emalloc"]) }
32+
33+
override int getSizeArg() { result = 0 }
34+
}
1035

1136
predicate bounded(Instruction i, Bound b, int delta, boolean upper) {
1237
// TODO: reason
@@ -17,18 +42,12 @@ class ArraySizeConfiguration extends ProductFlow::Configuration {
1742
ArraySizeConfiguration() { this = "ArraySizeConfiguration" }
1843

1944
override predicate isSourcePair(DataFlow::Node source1, DataFlow::Node source2) {
20-
exists(GVN sizeGvn |
21-
source1.asConvertedExpr().(AllocationExpr).getSizeExpr() = sizeGvn.getAnExpr() and
22-
source2.asConvertedExpr() = sizeGvn.getAnExpr()
23-
)
45+
source1.asConvertedExpr().(AllocationExpr).getSizeExpr() = source2.asConvertedExpr()
2446
}
2547

2648
override predicate isSinkPair(DataFlow::Node sink1, DataFlow::Node sink2) {
27-
exists(PointerAddInstruction pai, Instruction index, Bound b, int delta |
28-
pai.getRight() = index and
29-
pai.getLeft() = sink1.asInstruction() and
30-
bounded(index, b, delta, true) and
31-
sink2.asInstruction() = b.getInstruction() and
49+
exists(PointerAddInstruction pai, int delta |
50+
isSinkPair1(sink1, sink2, pai, delta) and
3251
(
3352
delta = 0 and
3453
exists(DataFlow::Node paiNode, DataFlow::Node derefNode |
@@ -43,9 +62,22 @@ class ArraySizeConfiguration extends ProductFlow::Configuration {
4362
}
4463
}
4564

65+
pragma[nomagic]
66+
predicate isSinkPair1(
67+
DataFlow::Node sink1, DataFlow::Node sink2, PointerAddInstruction pai, int delta
68+
) {
69+
exists(Instruction index, ValueNumberBound b |
70+
pai.getRight() = index and
71+
pai.getLeft() = sink1.asInstruction() and
72+
bounded(index, b, delta, true) and
73+
sink2.asInstruction() = b.getInstruction()
74+
)
75+
}
76+
4677
from
4778
ArraySizeConfiguration conf, DataFlow::PathNode source1, DataFlow2::PathNode source2,
4879
DataFlow::PathNode sink1, DataFlow2::PathNode sink2
4980
where conf.hasFlowPath(source1, source2, sink1, sink2)
5081
// TODO: pull delta out and display it
51-
select source1, source2, sink1, sink2
82+
select sink1.getNode(), source1, sink1, "Off-by one error allocated at $@ bounded by $@.", source1,
83+
source1.toString(), sink2, sink2.toString()

‎cpp/ql/src/experimental/Likely Bugs/OverrunWriteProductFlow.ql

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,26 @@
1+
/**
2+
* @name Overrunning write
3+
* @description TODO
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @id cpp/overrun-write
7+
* @tags reliability
8+
* security
9+
*/
10+
111
import cpp
212
import experimental.semmle.code.cpp.dataflow.ProductFlow
313
import semmle.code.cpp.ir.IR
414
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
515
import semmle.code.cpp.models.interfaces.Allocation
616
import semmle.code.cpp.models.interfaces.ArrayFunction
17+
import DataFlow::PathGraph
718

819
class StringSizeConfiguration extends ProductFlow::Configuration {
920
StringSizeConfiguration() { this = "StringSizeConfiguration" }
1021

1122
override predicate isSourcePair(DataFlow::Node bufSource, DataFlow::Node sizeSource) {
12-
exists(
13-
GVN sizeGvn // TODO: use-use flow instead of GVN
14-
|
15-
bufSource.asConvertedExpr().(AllocationExpr).getSizeExpr() = sizeGvn.getAnExpr() and
16-
sizeSource.asConvertedExpr() = sizeGvn.getAnExpr()
17-
)
23+
bufSource.asConvertedExpr().(AllocationExpr).getSizeExpr() = sizeSource.asConvertedExpr()
1824
}
1925

2026
override predicate isSinkPair(DataFlow::Node bufSink, DataFlow::Node sizeSink) {
@@ -31,4 +37,6 @@ from
3137
StringSizeConfiguration conf, DataFlow::PathNode source1, DataFlow2::PathNode source2,
3238
DataFlow::PathNode sink1, DataFlow2::PathNode sink2
3339
where conf.hasFlowPath(source1, source2, sink1, sink2)
34-
select source1, source2, sink1, sink2
40+
// TODO: pull delta out and display it
41+
select sink1.getNode(), source1, sink1, "Overrunning write allocated at $@ bounded by $@.", source1,
42+
source1.toString(), sink2, sink2.toString()

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/OverrunWriteProductFlow.expected

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,33 @@
1-
| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:26:18:26:23 | Load | test.cpp:26:31:26:39 | Convert |
2-
| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:30:18:30:23 | Load | test.cpp:30:31:30:39 | Convert |
1+
edges
2+
| test.cpp:16:11:16:21 | VariableAddress indirection [string] | test.cpp:24:21:24:31 | Call indirection [string] |
3+
| test.cpp:16:11:16:21 | VariableAddress indirection [string] | test.cpp:34:21:34:31 | Call indirection [string] |
4+
| test.cpp:18:5:18:30 | Store | test.cpp:18:10:18:15 | Load indirection [post update] [string] |
5+
| test.cpp:18:10:18:15 | Load indirection [post update] [string] | test.cpp:16:11:16:21 | VariableAddress indirection [string] |
6+
| test.cpp:18:19:18:24 | call to malloc | test.cpp:18:5:18:30 | Store |
7+
| test.cpp:24:21:24:31 | Call indirection [string] | test.cpp:26:13:26:15 | Load indirection [string] |
8+
| test.cpp:26:13:26:15 | Load indirection [string] | test.cpp:26:18:26:23 | FieldAddress indirection |
9+
| test.cpp:26:18:26:23 | FieldAddress indirection | test.cpp:26:18:26:23 | Load |
10+
| test.cpp:29:32:29:34 | str indirection [string] | test.cpp:30:13:30:15 | Load indirection [string] |
11+
| test.cpp:30:13:30:15 | Load indirection [string] | test.cpp:30:18:30:23 | FieldAddress indirection |
12+
| test.cpp:30:18:30:23 | FieldAddress indirection | test.cpp:30:18:30:23 | Load |
13+
| test.cpp:34:21:34:31 | Call indirection [string] | test.cpp:35:21:35:23 | str indirection [string] |
14+
| test.cpp:35:21:35:23 | str indirection [string] | test.cpp:29:32:29:34 | str indirection [string] |
15+
nodes
16+
| test.cpp:16:11:16:21 | VariableAddress indirection [string] | semmle.label | VariableAddress indirection [string] |
17+
| test.cpp:18:5:18:30 | Store | semmle.label | Store |
18+
| test.cpp:18:10:18:15 | Load indirection [post update] [string] | semmle.label | Load indirection [post update] [string] |
19+
| test.cpp:18:19:18:24 | call to malloc | semmle.label | call to malloc |
20+
| test.cpp:24:21:24:31 | Call indirection [string] | semmle.label | Call indirection [string] |
21+
| test.cpp:26:13:26:15 | Load indirection [string] | semmle.label | Load indirection [string] |
22+
| test.cpp:26:18:26:23 | FieldAddress indirection | semmle.label | FieldAddress indirection |
23+
| test.cpp:26:18:26:23 | Load | semmle.label | Load |
24+
| test.cpp:29:32:29:34 | str indirection [string] | semmle.label | str indirection [string] |
25+
| test.cpp:30:13:30:15 | Load indirection [string] | semmle.label | Load indirection [string] |
26+
| test.cpp:30:18:30:23 | FieldAddress indirection | semmle.label | FieldAddress indirection |
27+
| test.cpp:30:18:30:23 | Load | semmle.label | Load |
28+
| test.cpp:34:21:34:31 | Call indirection [string] | semmle.label | Call indirection [string] |
29+
| test.cpp:35:21:35:23 | str indirection [string] | semmle.label | str indirection [string] |
30+
subpaths
31+
#select
32+
| test.cpp:26:18:26:23 | Load | test.cpp:18:19:18:24 | call to malloc | test.cpp:26:18:26:23 | Load | Overrunning write allocated at $@ bounded by $@. | test.cpp:18:19:18:24 | call to malloc | call to malloc | test.cpp:26:31:26:39 | Convert | Convert |
33+
| test.cpp:30:18:30:23 | Load | test.cpp:18:19:18:24 | call to malloc | test.cpp:30:18:30:23 | Load | Overrunning write allocated at $@ bounded by $@. | test.cpp:18:19:18:24 | call to malloc | call to malloc | test.cpp:30:31:30:39 | Convert | Convert |

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ typedef struct
1515

1616
string_t *mk_string_t(int size) {
1717
string_t *str = (string_t *) malloc(sizeof(string_t));
18-
str->size = size;
1918
str->string = malloc(size);
19+
str->size = size;
2020
return str;
2121
}
2222

0 commit comments

Comments
 (0)