Skip to content

Commit 7c41219

Browse files
MathiasVPMathiasVP
authored
Merge pull request #10438 from MathiasVP/invalid-pointer-deref-query
C++: Add a `cpp/invalid-pointer-deref` query to experimental
2 parents addab09 + 0207607 commit 7c41219

File tree

15 files changed

+1322
-44
lines changed

15 files changed

+1322
-44
lines changed

‎cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import semmle.code.cpp.ir.dataflow.DataFlow
2-
import semmle.code.cpp.ir.dataflow.DataFlow2
1+
import experimental.semmle.code.cpp.ir.dataflow.DataFlow
2+
import experimental.semmle.code.cpp.ir.dataflow.DataFlow2
33

44
module ProductFlow {
55
abstract class Configuration extends string {
@@ -11,14 +11,43 @@ module ProductFlow {
1111
*
1212
* `source1` and `source2` must belong to the same callable.
1313
*/
14-
abstract predicate isSourcePair(DataFlow::Node source1, DataFlow::Node source2);
14+
predicate isSourcePair(DataFlow::Node source1, DataFlow::Node source2) { none() }
15+
16+
/**
17+
* Holds if `(source1, source2)` is a relevant data flow source with initial states `state1`
18+
* and `state2`, respectively.
19+
*
20+
* `source1` and `source2` must belong to the same callable.
21+
*/
22+
predicate isSourcePair(
23+
DataFlow::Node source1, string state1, DataFlow::Node source2, string state2
24+
) {
25+
state1 = "" and
26+
state2 = "" and
27+
this.isSourcePair(source1, source2)
28+
}
1529

1630
/**
1731
* Holds if `(sink1, sink2)` is a relevant data flow sink.
1832
*
1933
* `sink1` and `sink2` must belong to the same callable.
2034
*/
21-
abstract predicate isSinkPair(DataFlow::Node sink1, DataFlow::Node sink2);
35+
predicate isSinkPair(DataFlow::Node sink1, DataFlow::Node sink2) { none() }
36+
37+
/**
38+
* Holds if `(sink1, sink2)` is a relevant data flow sink with final states `state1`
39+
* and `state2`, respectively.
40+
*
41+
* `sink1` and `sink2` must belong to the same callable.
42+
*/
43+
predicate isSinkPair(
44+
DataFlow::Node sink1, DataFlow::FlowState state1, DataFlow::Node sink2,
45+
DataFlow::FlowState state2
46+
) {
47+
state1 = "" and
48+
state2 = "" and
49+
this.isSinkPair(sink1, sink2)
50+
}
2251

2352
predicate hasFlowPath(
2453
DataFlow::PathNode source1, DataFlow2::PathNode source2, DataFlow::PathNode sink1,
@@ -34,28 +63,28 @@ module ProductFlow {
3463
class Conf1 extends DataFlow::Configuration {
3564
Conf1() { this = "Conf1" }
3665

37-
override predicate isSource(DataFlow::Node source) {
38-
exists(Configuration conf | conf.isSourcePair(source, _))
66+
override predicate isSource(DataFlow::Node source, string state) {
67+
exists(Configuration conf | conf.isSourcePair(source, state, _, _))
3968
}
4069

41-
override predicate isSink(DataFlow::Node sink) {
42-
exists(Configuration conf | conf.isSinkPair(sink, _))
70+
override predicate isSink(DataFlow::Node sink, string state) {
71+
exists(Configuration conf | conf.isSinkPair(sink, state, _, _))
4372
}
4473
}
4574

4675
class Conf2 extends DataFlow2::Configuration {
4776
Conf2() { this = "Conf2" }
4877

49-
override predicate isSource(DataFlow::Node source) {
78+
override predicate isSource(DataFlow::Node source, string state) {
5079
exists(Configuration conf, DataFlow::Node source1 |
51-
conf.isSourcePair(source1, source) and
80+
conf.isSourcePair(source1, _, source, state) and
5281
any(Conf1 c).hasFlow(source1, _)
5382
)
5483
}
5584

56-
override predicate isSink(DataFlow::Node sink) {
85+
override predicate isSink(DataFlow::Node sink, string state) {
5786
exists(Configuration conf, DataFlow::Node sink1 |
58-
conf.isSinkPair(sink1, sink) and any(Conf1 c).hasFlow(_, sink1)
87+
conf.isSinkPair(sink1, _, sink, state) and any(Conf1 c).hasFlow(_, sink1)
5988
)
6089
}
6190
}
@@ -65,7 +94,7 @@ module ProductFlow {
6594
Configuration conf, DataFlow::PathNode source1, DataFlow2::PathNode source2,
6695
DataFlow::PathNode node1, DataFlow2::PathNode node2
6796
) {
68-
conf.isSourcePair(node1.getNode(), node2.getNode()) and
97+
conf.isSourcePair(node1.getNode(), _, node2.getNode(), _) and
6998
node1 = source1 and
7099
node2 = source2
71100
or
@@ -128,7 +157,7 @@ module ProductFlow {
128157
) {
129158
exists(DataFlow::PathNode mid1, DataFlow2::PathNode mid2 |
130159
reachableInterprocEntry(conf, source1, source2, mid1, mid2) and
131-
conf.isSinkPair(sink1.getNode(), sink2.getNode()) and
160+
conf.isSinkPair(sink1.getNode(), _, sink2.getNode(), _) and
132161
localPathStep1*(mid1, sink1) and
133162
localPathStep2*(mid2, sink2)
134163
)

‎cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/Bound.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ private newtype TBound =
2828
i.(LoadInstruction).getSourceAddress() instanceof FieldAddressInstruction
2929
or
3030
i.getAUse() instanceof ArgumentOperand
31+
or
32+
i instanceof PointerArithmeticInstruction
33+
or
34+
i.getAUse() instanceof AddressOperand
3135
)
3236
}
3337

‎cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExpr.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,11 +178,11 @@ class SemRelationalExpr extends SemBinaryExpr {
178178
}
179179

180180
class SemAddExpr extends SemBinaryExpr {
181-
SemAddExpr() { opcode instanceof Opcode::Add }
181+
SemAddExpr() { opcode instanceof Opcode::Add or opcode instanceof Opcode::PointerAdd }
182182
}
183183

184184
class SemSubExpr extends SemBinaryExpr {
185-
SemSubExpr() { opcode instanceof Opcode::Sub }
185+
SemSubExpr() { opcode instanceof Opcode::Sub or opcode instanceof Opcode::PointerSub }
186186
}
187187

188188
class SemMulExpr extends SemBinaryExpr {

‎cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticOpcode.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,18 @@ module Opcode {
6565
override string toString() { result = "Add" }
6666
}
6767

68+
class PointerAdd extends Opcode, TPointerAdd {
69+
override string toString() { result = "PointerAdd" }
70+
}
71+
6872
class Sub extends Opcode, TSub {
6973
override string toString() { result = "Sub" }
7074
}
7175

76+
class PointerSub extends Opcode, TPointerSub {
77+
override string toString() { result = "PointerSub" }
78+
}
79+
7280
class Mul extends Opcode, TMul {
7381
override string toString() { result = "Mul" }
7482
}

‎cpp/ql/lib/experimental/semmle/code/cpp/semantic/analysis/RangeAnalysis.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,9 @@ private SemGuard boundFlowCond(
223223
else resultIsStrict = testIsTrue.booleanNot()
224224
) and
225225
(
226-
if getTrackedTypeForSsaVariable(v) instanceof SemIntegerType
226+
if
227+
getTrackedTypeForSsaVariable(v) instanceof SemIntegerType or
228+
getTrackedTypeForSsaVariable(v) instanceof SemAddressType
227229
then
228230
upper = true and strengthen = -1
229231
or

‎cpp/ql/src/experimental/Security/CWE/CWE-193/InvalidPointerDeref.cpp

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
void *malloc(unsigned);
2+
unsigned get_size();
3+
void write_data(const unsigned char*, const unsigned char*);
4+
5+
int main(int argc, char* argv[]) {
6+
unsigned size = get_size();
7+
8+
{
9+
unsigned char *begin = (unsigned char*)malloc(size);
10+
if(!begin) return -1;
11+
12+
unsigned char* end = begin + size;
13+
write_data(begin, end);
14+
*end = '\0'; // BAD: Out-of-bounds write
15+
}
16+
17+
{
18+
unsigned char *begin = (unsigned char*)malloc(size);
19+
if(!begin) return -1;
20+
21+
unsigned char* end = begin + size;
22+
write_data(begin, end);
23+
*(end - 1) = '\0'; // GOOD: writing to the last byte
24+
}
25+
26+
}

‎cpp/ql/src/experimental/Security/CWE/CWE-193/InvalidPointerDeref.qhelp

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>The program performs an out-of-bounds read or write operation. In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.</p>
7+
8+
</overview>
9+
<recommendation>
10+
11+
<p>Ensure that pointer dereferences are properly guarded to ensure that they cannot be used to read or write past the end of the allocation.</p>
12+
13+
</recommendation>
14+
<example>
15+
<p>The first example allocates a buffer of size <code>size</code> and creates a local variable that stores the location that is one byte past the end of the allocation.
16+
This local variable is then dereferenced which results in an out-of-bounds write.
17+
The second example subtracts one from the <code>end</code> variable before dereferencing it. This subtraction ensures that the write correctly updates the final byte of the allocation.</p>
18+
<sample src="InvalidPointerDeref.cpp" />
19+
20+
</example>
21+
<references>
22+
23+
<li>CERT C Coding Standard:
24+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts</a>.</li>
25+
<li>
26+
OWASP:
27+
<a href="https://owasp.org/www-community/vulnerabilities/Buffer_Overflow">Buffer Overflow</a>.
28+
</li>
29+
30+
</references>
31+
</qhelp>

0 commit comments

Comments
 (0)