Skip to content

Commit b1ae3a0

Browse files
jketemajketema
committed
Merge remote-tracking branch 'upstream/main' into clears-content
2 parents 2628552 + df61eaf commit b1ae3a0

File tree

73 files changed

+2633
-169
lines changed

Some content is hidden

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

73 files changed

+2633
-169
lines changed

‎cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticSSA.qll

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,27 @@ predicate semBackEdge(SemSsaPhiNode phi, SemSsaVariable inp, SemSsaReadPositionP
7070
// Conservatively assume that every edge is a back edge if we don't have dominance information.
7171
(
7272
phi.getBasicBlock().bbDominates(edge.getOrigBlock()) or
73+
irreducibleSccEdge(phi.getBasicBlock(), edge.getOrigBlock()) or
7374
not edge.getOrigBlock().hasDominanceInformation()
7475
)
7576
}
77+
78+
/**
79+
* Holds if the edge from b1 to b2 is part of a multiple-entry cycle in an irreducible control flow
80+
* graph.
81+
*
82+
* An ireducible control flow graph is one where the usual dominance-based back edge detection does
83+
* not work, because there is a cycle with multiple entry points, meaning there are
84+
* mutually-reachable basic blocks where neither dominates the other. For such a graph, we first
85+
* remove all detectable back-edges using the normal condition that the predecessor block is
86+
* dominated by the successor block, then mark all edges in a cycle in the resulting graph as back
87+
* edges.
88+
*/
89+
private predicate irreducibleSccEdge(SemBasicBlock b1, SemBasicBlock b2) {
90+
trimmedEdge(b1, b2) and trimmedEdge+(b2, b1)
91+
}
92+
93+
private predicate trimmedEdge(SemBasicBlock pred, SemBasicBlock succ) {
94+
pred.getASuccessor() = succ and
95+
not succ.bbDominates(pred)
96+
}

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

Lines changed: 66 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysi
1414
import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
1515
import semmle.code.cpp.ir.IR
1616
import semmle.code.cpp.ir.dataflow.DataFlow
17-
import FieldAddressToDerefFlow::PathGraph
17+
import ArrayAddressToDerefFlow::PathGraph
1818

1919
pragma[nomagic]
2020
Instruction getABoundIn(SemBound b, IRFunction func) {
@@ -78,28 +78,45 @@ predicate isInvalidPointerDerefSink2(DataFlow::Node sink, Instruction i, string
7878
)
7979
}
8080

81+
predicate arrayTypeCand(ArrayType arrayType) {
82+
any(Variable v).getUnspecifiedType() = arrayType and
83+
exists(arrayType.getArraySize())
84+
}
85+
8186
pragma[nomagic]
8287
predicate arrayTypeHasSizes(ArrayType arr, int baseTypeSize, int arraySize) {
88+
arrayTypeCand(arr) and
8389
arr.getBaseType().getSize() = baseTypeSize and
8490
arr.getArraySize() = arraySize
8591
}
8692

87-
predicate pointerArithOverflow0(
88-
PointerArithmeticInstruction pai, Field f, int size, int bound, int delta
89-
) {
90-
not f.getNamespace() instanceof StdNamespace and
91-
arrayTypeHasSizes(f.getUnspecifiedType(), pai.getElementSize(), size) and
92-
semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and
93-
delta = bound - size and
94-
delta >= 0 and
95-
size != 0 and
96-
size != 1
93+
bindingset[pai]
94+
pragma[inline_late]
95+
predicate constantUpperBounded(PointerArithmeticInstruction pai, int delta) {
96+
semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), delta, true, _)
97+
}
98+
99+
bindingset[pai, size]
100+
predicate pointerArithOverflow0Impl(PointerArithmeticInstruction pai, int size, int delta) {
101+
exists(int bound |
102+
constantUpperBounded(pai, bound) and
103+
delta = bound - size and
104+
delta >= 0 and
105+
size != 0 and
106+
size != 1
107+
)
108+
}
109+
110+
pragma[nomagic]
111+
predicate pointerArithOverflow0(PointerArithmeticInstruction pai, int delta) {
112+
exists(int size |
113+
arrayTypeHasSizes(_, pai.getElementSize(), size) and
114+
pointerArithOverflow0Impl(pai, size, delta)
115+
)
97116
}
98117

99118
module PointerArithmeticToDerefConfig implements DataFlow::ConfigSig {
100-
predicate isSource(DataFlow::Node source) {
101-
pointerArithOverflow0(source.asInstruction(), _, _, _, _)
102-
}
119+
predicate isSource(DataFlow::Node source) { pointerArithOverflow0(source.asInstruction(), _) }
103120

104121
predicate isBarrierIn(DataFlow::Node node) { isSource(node) }
105122

@@ -110,25 +127,38 @@ module PointerArithmeticToDerefConfig implements DataFlow::ConfigSig {
110127

111128
module PointerArithmeticToDerefFlow = DataFlow::Global<PointerArithmeticToDerefConfig>;
112129

113-
predicate pointerArithOverflow(
114-
PointerArithmeticInstruction pai, Field f, int size, int bound, int delta
115-
) {
116-
pointerArithOverflow0(pai, f, size, bound, delta) and
130+
predicate pointerArithOverflow(PointerArithmeticInstruction pai, int delta) {
131+
pointerArithOverflow0(pai, delta) and
117132
PointerArithmeticToDerefFlow::flow(DataFlow::instructionNode(pai), _)
118133
}
119134

120-
module FieldAddressToDerefConfig implements DataFlow::StateConfigSig {
135+
bindingset[v]
136+
predicate finalPointerArithOverflow(Variable v, PointerArithmeticInstruction pai, int delta) {
137+
exists(int size |
138+
arrayTypeHasSizes(pragma[only_bind_out](v.getUnspecifiedType()), pai.getElementSize(), size) and
139+
pointerArithOverflow0Impl(pai, size, delta)
140+
)
141+
}
142+
143+
predicate isSourceImpl(DataFlow::Node source, Variable v) {
144+
(
145+
source.asInstruction().(FieldAddressInstruction).getField() = v
146+
or
147+
source.asInstruction().(VariableAddressInstruction).getAstVariable() = v
148+
) and
149+
arrayTypeCand(v.getUnspecifiedType())
150+
}
151+
152+
module ArrayAddressToDerefConfig implements DataFlow::StateConfigSig {
121153
newtype FlowState =
122-
additional TArray(Field f) { pointerArithOverflow(_, f, _, _, _) } or
154+
additional TArray() or
123155
additional TOverflowArithmetic(PointerArithmeticInstruction pai) {
124-
pointerArithOverflow(pai, _, _, _, _)
156+
pointerArithOverflow(pai, _)
125157
}
126158

127159
predicate isSource(DataFlow::Node source, FlowState state) {
128-
exists(Field f |
129-
source.asInstruction().(FieldAddressInstruction).getField() = f and
130-
state = TArray(f)
131-
)
160+
isSourceImpl(source, _) and
161+
state = TArray()
132162
}
133163

134164
predicate isSink(DataFlow::Node sink, FlowState state) {
@@ -147,27 +177,27 @@ module FieldAddressToDerefConfig implements DataFlow::StateConfigSig {
147177
predicate isAdditionalFlowStep(
148178
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
149179
) {
150-
exists(PointerArithmeticInstruction pai, Field f |
151-
state1 = TArray(f) and
180+
exists(PointerArithmeticInstruction pai |
181+
state1 = TArray() and
152182
state2 = TOverflowArithmetic(pai) and
153183
pai.getLeft() = node1.asInstruction() and
154184
node2.asInstruction() = pai and
155-
pointerArithOverflow(pai, f, _, _, _)
185+
pointerArithOverflow(pai, _)
156186
)
157187
}
158188
}
159189

160-
module FieldAddressToDerefFlow = DataFlow::GlobalWithState<FieldAddressToDerefConfig>;
190+
module ArrayAddressToDerefFlow = DataFlow::GlobalWithState<ArrayAddressToDerefConfig>;
161191

162192
from
163-
Field f, FieldAddressToDerefFlow::PathNode source, PointerArithmeticInstruction pai,
164-
FieldAddressToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
193+
Variable v, ArrayAddressToDerefFlow::PathNode source, PointerArithmeticInstruction pai,
194+
ArrayAddressToDerefFlow::PathNode sink, Instruction deref, string operation, int delta
165195
where
166-
FieldAddressToDerefFlow::flowPath(source, sink) and
196+
ArrayAddressToDerefFlow::flowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and
167197
isInvalidPointerDerefSink2(sink.getNode(), deref, operation) and
168-
source.getState() = FieldAddressToDerefConfig::TArray(f) and
169-
sink.getState() = FieldAddressToDerefConfig::TOverflowArithmetic(pai) and
170-
pointerArithOverflow(pai, f, _, _, delta)
198+
pragma[only_bind_out](sink.getState()) = ArrayAddressToDerefConfig::TOverflowArithmetic(pai) and
199+
isSourceImpl(source.getNode(), v) and
200+
finalPointerArithOverflow(v, pai, delta)
171201
select pai, source, sink,
172202
"This pointer arithmetic may have an off-by-" + (delta + 1) +
173-
" error allowing it to overrun $@ at this $@.", f, f.getName(), deref, operation
203+
" error allowing it to overrun $@ at this $@.", v, v.getName(), deref, operation

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

Lines changed: 129 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import cpp
1919
import semmle.code.cpp.ir.dataflow.internal.ProductFlow
2020
import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysis
2121
import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
22+
import semmle.code.cpp.ir.ValueNumbering
23+
import semmle.code.cpp.controlflow.IRGuards
2224
import semmle.code.cpp.ir.IR
2325
import codeql.util.Unit
2426

@@ -67,6 +69,86 @@ predicate hasSize(HeuristicAllocationExpr alloc, DataFlow::Node n, int state) {
6769
)
6870
}
6971

72+
/**
73+
* A module that encapsulates a barrier guard to remove false positives from flow like:
74+
* ```cpp
75+
* char *p = new char[size];
76+
* // ...
77+
* unsigned n = size;
78+
* // ...
79+
* if(n < size) {
80+
* use(*p[n]);
81+
* }
82+
* ```
83+
* In this case, the sink pair identified by the product flow library (without any additional barriers)
84+
* would be `(p, n)` (where `n` is the `n` in `p[n]`), because there exists a pointer-arithmetic
85+
* instruction `pai` such that:
86+
* 1. The left-hand of `pai` flows from the allocation, and
87+
* 2. The right-hand of `pai` is non-strictly upper bounded by `n` (where `n` is the `n` in `p[n]`)
88+
* but because there's a strict comparison that compares `n` against the size of the allocation this
89+
* snippet is fine.
90+
*/
91+
module Barrier2 {
92+
private class FlowState2 = AllocToInvalidPointerConfig::FlowState2;
93+
94+
private module BarrierConfig2 implements DataFlow::ConfigSig {
95+
predicate isSource(DataFlow::Node source) {
96+
// The sources is the same as in the sources for the second
97+
// projection in the `AllocToInvalidPointerConfig` module.
98+
hasSize(_, source, _)
99+
}
100+
101+
additional predicate isSink(
102+
DataFlow::Node left, DataFlow::Node right, IRGuardCondition g, FlowState2 state,
103+
boolean testIsTrue
104+
) {
105+
// The sink is any "large" side of a relational comparison.
106+
g.comparesLt(left.asOperand(), right.asOperand(), state, true, testIsTrue)
107+
}
108+
109+
predicate isSink(DataFlow::Node sink) { isSink(_, sink, _, _, _) }
110+
}
111+
112+
private import DataFlow::Global<BarrierConfig2>
113+
114+
private FlowState2 getAFlowStateForNode(DataFlow::Node node) {
115+
exists(DataFlow::Node source |
116+
flow(source, node) and
117+
hasSize(_, source, result)
118+
)
119+
}
120+
121+
private predicate operandGuardChecks(
122+
IRGuardCondition g, Operand left, Operand right, FlowState2 state, boolean edge
123+
) {
124+
exists(DataFlow::Node nLeft, DataFlow::Node nRight, FlowState2 state0 |
125+
nRight.asOperand() = right and
126+
nLeft.asOperand() = left and
127+
BarrierConfig2::isSink(nLeft, nRight, g, state0, edge) and
128+
state = getAFlowStateForNode(nRight) and
129+
state0 <= state
130+
)
131+
}
132+
133+
Instruction getABarrierInstruction(FlowState2 state) {
134+
exists(IRGuardCondition g, ValueNumber value, Operand use, boolean edge |
135+
use = value.getAUse() and
136+
operandGuardChecks(pragma[only_bind_into](g), pragma[only_bind_into](use), _,
137+
pragma[only_bind_into](state), pragma[only_bind_into](edge)) and
138+
result = value.getAnInstruction() and
139+
g.controls(result.getBlock(), edge)
140+
)
141+
}
142+
143+
DataFlow::Node getABarrierNode(FlowState2 state) {
144+
result.asOperand() = getABarrierInstruction(state).getAUse()
145+
}
146+
147+
IRBlock getABarrierBlock(FlowState2 state) {
148+
result.getAnInstruction() = getABarrierInstruction(state)
149+
}
150+
}
151+
70152
/**
71153
* A product-flow configuration for flow from an (allocation, size) pair to a
72154
* pointer-arithmetic operation that is non-strictly upper-bounded by `allocation + size`.
@@ -111,15 +193,14 @@ module AllocToInvalidPointerConfig implements ProductFlow::StateConfigSig {
111193
exists(state1) and
112194
// We check that the delta computed by the range analysis matches the
113195
// state value that we set in `isSourcePair`.
114-
exists(int delta |
115-
isSinkImpl(_, sink1, sink2, delta) and
116-
state2 = delta
117-
)
196+
isSinkImpl(_, sink1, sink2, state2)
118197
}
119198

120199
predicate isBarrier1(DataFlow::Node node, FlowState1 state) { none() }
121200

122-
predicate isBarrier2(DataFlow::Node node, FlowState2 state) { none() }
201+
predicate isBarrier2(DataFlow::Node node, FlowState2 state) {
202+
node = Barrier2::getABarrierNode(state)
203+
}
123204

124205
predicate isBarrierIn1(DataFlow::Node node) { isSourcePair(node, _, _, _) }
125206

@@ -160,13 +241,40 @@ pragma[nomagic]
160241
predicate pointerAddInstructionHasBounds(
161242
PointerAddInstruction pai, DataFlow::Node sink1, DataFlow::Node sink2, int delta
162243
) {
163-
exists(Instruction right |
244+
InterestingPointerAddInstruction::isInteresting(pragma[only_bind_into](pai)) and
245+
exists(Instruction right, Instruction instr2 |
164246
pai.getRight() = right and
165247
pai.getLeft() = sink1.asInstruction() and
166-
bounded1(right, sink2.asInstruction(), delta)
248+
instr2 = sink2.asInstruction() and
249+
bounded1(right, instr2, delta) and
250+
not right = Barrier2::getABarrierInstruction(delta) and
251+
not instr2 = Barrier2::getABarrierInstruction(delta)
167252
)
168253
}
169254

255+
module InterestingPointerAddInstruction {
256+
private module PointerAddInstructionConfig implements DataFlow::ConfigSig {
257+
predicate isSource(DataFlow::Node source) {
258+
// The sources is the same as in the sources for the second
259+
// projection in the `AllocToInvalidPointerConfig` module.
260+
hasSize(source.asConvertedExpr(), _, _)
261+
}
262+
263+
predicate isSink(DataFlow::Node sink) {
264+
sink.asInstruction() = any(PointerAddInstruction pai).getLeft()
265+
}
266+
}
267+
268+
private import DataFlow::Global<PointerAddInstructionConfig>
269+
270+
predicate isInteresting(PointerAddInstruction pai) {
271+
exists(DataFlow::Node n |
272+
n.asInstruction() = pai.getLeft() and
273+
flowTo(n)
274+
)
275+
}
276+
}
277+
170278
/**
171279
* Holds if `pai` is non-strictly upper bounded by `sink2 + delta` and `sink1` is the
172280
* left operand of the pointer-arithmetic operation.
@@ -246,12 +354,21 @@ module InvalidPointerToDerefFlow = DataFlow::Global<InvalidPointerToDerefConfig>
246354
predicate invalidPointerToDerefSource(
247355
PointerArithmeticInstruction pai, DataFlow::Node source, int delta
248356
) {
249-
exists(AllocToInvalidPointerFlow::PathNode1 p, DataFlow::Node sink1 |
250-
pragma[only_bind_out](p.getNode()) = sink1 and
251-
AllocToInvalidPointerFlow::flowPath(_, _, pragma[only_bind_into](p), _) and
252-
isSinkImpl(pai, sink1, _, _) and
357+
exists(
358+
AllocToInvalidPointerFlow::PathNode1 p1, AllocToInvalidPointerFlow::PathNode2 p2,
359+
DataFlow::Node sink1, DataFlow::Node sink2, int delta0
360+
|
361+
pragma[only_bind_out](p1.getNode()) = sink1 and
362+
pragma[only_bind_out](p2.getNode()) = sink2 and
363+
AllocToInvalidPointerFlow::flowPath(_, _, pragma[only_bind_into](p1), pragma[only_bind_into](p2)) and
364+
// Note that `delta` is not necessarily equal to `delta0`:
365+
// `delta0` is the constant offset added to the size of the allocation, and
366+
// delta is the constant difference between the pointer-arithmetic instruction
367+
// and the instruction computing the address for which we will search for a dereference.
368+
isSinkImpl(pai, sink1, sink2, delta0) and
253369
bounded2(source.asInstruction(), pai, delta) and
254-
delta >= 0
370+
delta >= 0 and
371+
not source.getBasicBlock() = Barrier2::getABarrierBlock(delta0)
255372
)
256373
}
257374

0 commit comments

Comments
 (0)