Skip to content

Commit 7eee589

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#12569 from andersfugmann/andersfugmann/use_after_free
C++: Implement use-after-free and double-free queries using the new IR use-use dataflow
2 parents ccb57f2 + fa5ed04 commit 7eee589

File tree

20 files changed

+951
-81
lines changed

20 files changed

+951
-81
lines changed

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -897,23 +897,6 @@ private class MyConsistencyConfiguration extends Consistency::ConsistencyConfigu
897897
}
898898
}
899899

900-
/**
901-
* Gets the basic block of `node`.
902-
*/
903-
IRBlock getBasicBlock(Node node) {
904-
node.asInstruction().getBlock() = result
905-
or
906-
node.asOperand().getUse().getBlock() = result
907-
or
908-
node.(SsaPhiNode).getPhiNode().getBasicBlock() = result
909-
or
910-
node.(RawIndirectOperand).getOperand().getUse().getBlock() = result
911-
or
912-
node.(RawIndirectInstruction).getInstruction().getBlock() = result
913-
or
914-
result = getBasicBlock(node.(PostUpdateNode).getPreUpdateNode())
915-
}
916-
917900
/**
918901
* A local flow relation that includes both local steps, read steps and
919902
* argument-to-return flow through summarized functions.
@@ -999,7 +982,8 @@ private int countNumberOfBranchesUsingParameter(SwitchInstruction switch, Parame
999982
// we pick the one with the highest edge count.
1000983
result =
1001984
max(SsaPhiNode phi |
1002-
switch.getSuccessor(caseOrDefaultEdge()).getBlock().dominanceFrontier() = getBasicBlock(phi) and
985+
switch.getSuccessor(caseOrDefaultEdge()).getBlock().dominanceFrontier() =
986+
phi.getBasicBlock() and
1003987
phi.getSourceVariable() = sv
1004988
|
1005989
strictcount(phi.getAnInput())

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,28 @@ class Node extends TIRDataFlowNode {
160160
/** Gets the operands corresponding to this node, if any. */
161161
Operand asOperand() { result = this.(OperandNode).getOperand() }
162162

163+
/**
164+
* Holds if this node is at index `i` in basic block `block`.
165+
*
166+
* Note: Phi nodes are considered to be at index `-1`.
167+
*/
168+
final predicate hasIndexInBlock(IRBlock block, int i) {
169+
this.asInstruction() = block.getInstruction(i)
170+
or
171+
this.asOperand().getUse() = block.getInstruction(i)
172+
or
173+
this.(SsaPhiNode).getPhiNode().getBasicBlock() = block and i = -1
174+
or
175+
this.(RawIndirectOperand).getOperand().getUse() = block.getInstruction(i)
176+
or
177+
this.(RawIndirectInstruction).getInstruction() = block.getInstruction(i)
178+
or
179+
this.(PostUpdateNode).getPreUpdateNode().hasIndexInBlock(block, i)
180+
}
181+
182+
/** Gets the basic block of this node, if any. */
183+
final IRBlock getBasicBlock() { this.hasIndexInBlock(result, _) }
184+
163185
/**
164186
* Gets the non-conversion expression corresponding to this node, if any.
165187
* This predicate only has a result on nodes that represent the value of
@@ -530,7 +552,7 @@ class SsaPhiNode extends Node, TSsaPhiNode {
530552
*/
531553
final Node getAnInput(boolean fromBackEdge) {
532554
localFlowStep(result, this) and
533-
if phi.getBasicBlock().dominates(getBasicBlock(result))
555+
if phi.getBasicBlock().dominates(result.getBasicBlock())
534556
then fromBackEdge = true
535557
else fromBackEdge = false
536558
}
@@ -1887,7 +1909,7 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
18871909
e = value.getAnInstruction().getConvertedResultExpression() and
18881910
result.getConvertedExpr() = e and
18891911
guardChecks(g, value.getAnInstruction().getConvertedResultExpression(), edge) and
1890-
g.controls(getBasicBlock(result), edge)
1912+
g.controls(result.getBasicBlock(), edge)
18911913
)
18921914
}
18931915
}

‎cpp/ql/src/Critical/DoubleFree.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
int* f() {
2+
int *buff = malloc(SIZE*sizeof(int));
3+
do_stuff(buff);
4+
free(buff);
5+
int *new_buffer = malloc(SIZE*sizeof(int));
6+
free(buff); // BAD: If new_buffer is assigned the same address as buff,
7+
// the memory allocator will free the new buffer memory region,
8+
// leading to use-after-free problems and memory corruption.
9+
return new_buffer;
10+
}

‎cpp/ql/src/Critical/DoubleFree.qhelp

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>
9+
Deallocating memory more than once can lead to a double-free vulnerability. This can be exploited to
10+
corrupt the allocator's internal data structures, which can lead to denial-of-service attacks by crashing
11+
the program, or security vulnerabilities, by allowing an attacker to overwrite arbitrary memory locations.
12+
</p>
13+
14+
</overview>
15+
<recommendation>
16+
<p>
17+
Ensure that all execution paths deallocate the allocated memory at most once. If possible, reassign
18+
the pointer to a null value after deallocating it. This will prevent double-free vulnerabilities since
19+
most deallocation functions will perform a null-pointer check before attempting to deallocate the memory.
20+
</p>
21+
22+
</recommendation>
23+
<example><sample src="DoubleFree.cpp" />
24+
</example>
25+
<references>
26+
27+
<li>
28+
OWASP:
29+
<a href="https://owasp.org/www-community/vulnerabilities/Doubly_freeing_memory">Doubly freeing memory</a>.
30+
</li>
31+
32+
</references>
33+
</qhelp>

‎cpp/ql/src/Critical/DoubleFree.ql

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/**
2+
* @name Potential double free
3+
* @description Freeing a resource more than once can lead to undefined behavior and cause memory corruption.
4+
* @kind path-problem
5+
* @precision medium
6+
* @id cpp/double-free
7+
* @problem.severity warning
8+
* @security-severity 9.3
9+
* @tags reliability
10+
* security
11+
* external/cwe/cwe-415
12+
*/
13+
14+
import cpp
15+
import semmle.code.cpp.dataflow.new.DataFlow
16+
import FlowAfterFree
17+
import DoubleFree::PathGraph
18+
19+
predicate isFree(DataFlow::Node n, Expr e) { isFree(n, e, _) }
20+
21+
/**
22+
* `dealloc1` is a deallocation expression and `e` is an expression such
23+
* that is deallocated by a deallocation expression, and the `(dealloc1, e)` pair
24+
* should be excluded by the `FlowFromFree` library.
25+
*
26+
* Note that `e` is not necessarily the expression deallocated by `dealloc1`. It will
27+
* be bound to the second deallocation as identified by the `FlowFromFree` library.
28+
*/
29+
bindingset[dealloc1, e]
30+
predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) {
31+
exists(DeallocationExpr dealloc2 | isFree(_, e, dealloc2) |
32+
dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and
33+
// From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl:
34+
// "After calling MmFreePagesFromMdl, the caller must also call ExFreePool
35+
// to release the memory that was allocated for the MDL structure."
36+
isExFreePoolCall(dealloc2, _)
37+
)
38+
}
39+
40+
module DoubleFree = FlowFromFree<isFree/2, isExcludeFreePair/2>;
41+
42+
from DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2
43+
where
44+
DoubleFree::flowPath(source, sink) and
45+
isFree(source.getNode(), _, dealloc) and
46+
isFree(sink.getNode(), e2)
47+
select sink.getNode(), source, sink,
48+
"Memory pointed to by '" + e2.toString() + "' may already have been freed by $@.", dealloc,
49+
dealloc.toString()

‎cpp/ql/src/Critical/FlowAfterFree.qll

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
import cpp
2+
import semmle.code.cpp.dataflow.new.DataFlow
3+
private import semmle.code.cpp.ir.IR
4+
5+
/**
6+
* Signature for a predicate that holds if `n.asExpr() = e` and `n` is a sink in
7+
* the `FlowFromFreeConfig` module.
8+
*/
9+
private signature predicate isSinkSig(DataFlow::Node n, Expr e);
10+
11+
/**
12+
* Holds if `dealloc` is a deallocation expression and `e` is an expression such
13+
* that `isFree(_, e)` holds for some `isFree` predicate satisfying `isSinkSig`,
14+
* and this source-sink pair should be excluded from the analysis.
15+
*/
16+
bindingset[dealloc, e]
17+
private signature predicate isExcludedSig(DeallocationExpr dealloc, Expr e);
18+
19+
/**
20+
* Holds if `(b1, i1)` strictly post-dominates `(b2, i2)`
21+
*/
22+
bindingset[i1, i2]
23+
predicate strictlyPostDominates(IRBlock b1, int i1, IRBlock b2, int i2) {
24+
b1 = b2 and
25+
i1 > i2
26+
or
27+
b1.strictlyPostDominates(b2)
28+
}
29+
30+
/**
31+
* Holds if `(b1, i1)` strictly dominates `(b2, i2)`
32+
*/
33+
bindingset[i1, i2]
34+
predicate strictlyDominates(IRBlock b1, int i1, IRBlock b2, int i2) {
35+
b1 = b2 and
36+
i1 < i2
37+
or
38+
b1.strictlyDominates(b2)
39+
}
40+
41+
/**
42+
* Constructs a `FlowFromFreeConfig` module that can be used to find flow between
43+
* a pointer being freed by some deallocation function, and a user-specified sink.
44+
*
45+
* In order to reduce false positives, the set of sinks is restricted to only those
46+
* that satisfy at least one of the following two criteria:
47+
* 1. The source dominates the sink, or
48+
* 2. The sink post-dominates the source.
49+
*/
50+
module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
51+
module FlowFromFreeConfig implements DataFlow::StateConfigSig {
52+
class FlowState instanceof Expr {
53+
FlowState() { isFree(_, this, _) }
54+
55+
string toString() { result = super.toString() }
56+
}
57+
58+
predicate isSource(DataFlow::Node node, FlowState state) { isFree(node, state, _) }
59+
60+
pragma[inline]
61+
predicate isSink(DataFlow::Node sink, FlowState state) {
62+
exists(
63+
Expr e, DataFlow::Node source, IRBlock b1, int i1, IRBlock b2, int i2,
64+
DeallocationExpr dealloc
65+
|
66+
isASink(sink, e) and
67+
isFree(source, state, dealloc) and
68+
e != state and
69+
source.hasIndexInBlock(b1, i1) and
70+
sink.hasIndexInBlock(b2, i2) and
71+
not isExcluded(dealloc, e)
72+
|
73+
strictlyDominates(b1, i1, b2, i2)
74+
or
75+
strictlyPostDominates(b2, i2, b1, i1)
76+
)
77+
}
78+
79+
predicate isBarrierIn(DataFlow::Node n) {
80+
n.asIndirectExpr() = any(AddressOfExpr aoe)
81+
or
82+
n.asIndirectExpr() = any(Call call).getAnArgument()
83+
or
84+
exists(Expr e |
85+
n.asIndirectExpr() = e.(PointerDereferenceExpr).getOperand() or
86+
n.asIndirectExpr() = e.(ArrayExpr).getArrayBase()
87+
|
88+
e = any(StoreInstruction store).getDestinationAddress().getUnconvertedResultExpression()
89+
)
90+
}
91+
92+
predicate isBarrier(DataFlow::Node n, FlowState state) { none() }
93+
94+
predicate isAdditionalFlowStep(
95+
DataFlow::Node n1, FlowState state1, DataFlow::Node n2, FlowState state2
96+
) {
97+
none()
98+
}
99+
}
100+
101+
import DataFlow::GlobalWithState<FlowFromFreeConfig>
102+
}
103+
104+
/**
105+
* Holds if `n` is a dataflow node such that `n.asExpr() = e` and `e`
106+
* is being freed by a deallocation expression `dealloc`.
107+
*/
108+
predicate isFree(DataFlow::Node n, Expr e, DeallocationExpr dealloc) {
109+
e = dealloc.getFreedExpr() and
110+
e = n.asExpr() and
111+
// Ignore realloc functions
112+
not exists(dealloc.(FunctionCall).getTarget().(AllocationFunction).getReallocPtrArg())
113+
}
114+
115+
/**
116+
* Holds if `fc` is a function call that is the result of expanding
117+
* the `ExFreePool` macro.
118+
*/
119+
predicate isExFreePoolCall(FunctionCall fc, Expr e) {
120+
e = fc.getArgument(0) and
121+
(
122+
exists(MacroInvocation mi |
123+
mi.getMacroName() = "ExFreePool" and
124+
mi.getExpr() = fc
125+
)
126+
or
127+
fc.getTarget().hasGlobalName("ExFreePool")
128+
)
129+
}

0 commit comments

Comments
 (0)