Skip to content

Commit 7b0df57

Browse files
MathiasVPMathiasVP
committed
C++: Remove the two configurations that depend on flow state to speed up performance on ChakraCore.
1 parent 761f6d3 commit 7b0df57

File tree

2 files changed

+68
-110
lines changed

2 files changed

+68
-110
lines changed

‎cpp/ql/src/Security/CWE/CWE-843/TypeConfusion.ql

Lines changed: 40 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212

1313
import cpp
1414
import semmle.code.cpp.dataflow.new.DataFlow
15-
import BadFlow::PathGraph
15+
import Flow::PathGraph
1616

1717
/**
1818
* Holds if `f` is a field located at byte offset `offset` in `c`.
@@ -179,23 +179,28 @@ class UnsafeCast extends Cast {
179179
}
180180

181181
/**
182-
* Holds if `source` is an allocation that allocates a value of type `state`.
182+
* Holds if `source` is an allocation that allocates a value of type `type`.
183183
*/
184-
predicate isSourceImpl(DataFlow::Node source, Class state) {
185-
state = source.asExpr().(AllocationExpr).getAllocatedElementType().stripType() and
184+
predicate isSourceImpl(DataFlow::Node source, Class type) {
185+
exists(AllocationExpr alloc |
186+
alloc = source.asExpr() and
187+
type = alloc.getAllocatedElementType().stripType() and
188+
not exists(
189+
alloc
190+
.(NewOrNewArrayExpr)
191+
.getAllocator()
192+
.(OperatorNewAllocationFunction)
193+
.getPlacementArgument()
194+
)
195+
) and
186196
exists(TypeDeclarationEntry tde |
187-
tde = state.getDefinition() and
197+
tde = type.getDefinition() and
188198
not tde.isFromUninstantiatedTemplate(_)
189199
)
190200
}
191201

192-
/**
193-
* The `RelevantStateConfig` configuration is used to find the set of
194-
* states for the `BadConfig` and `GoodConfig`. The flow computed by
195-
* `RelevantStateConfig` is used to implement the `relevantState` predicate
196-
* which is used to avoid a cartesian product in `isSinkImpl`.
197-
*/
198-
module RelevantStateConfig implements DataFlow::ConfigSig {
202+
/** A configuration describing flow from an allocation to a potentially unsafe cast. */
203+
module Config implements DataFlow::ConfigSig {
199204
predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) }
200205

201206
predicate isBarrier(DataFlow::Node node) {
@@ -212,122 +217,47 @@ module RelevantStateConfig implements DataFlow::ConfigSig {
212217
)
213218
}
214219

215-
predicate isSink(DataFlow::Node sink) {
216-
exists(UnsafeCast cast | sink.asExpr() = cast.getUnconverted())
217-
}
220+
predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(UnsafeCast cast).getUnconverted() }
218221

219222
int fieldFlowBranchLimit() { result = 0 }
220223
}
221224

222-
module RelevantStateFlow = DataFlow::Global<RelevantStateConfig>;
225+
module Flow = DataFlow::Global<Config>;
223226

224-
predicate relevantState(DataFlow::Node source, DataFlow::Node sink, Class state) {
225-
RelevantStateFlow::flow(source, sink) and
226-
isSourceImpl(source, state)
227+
predicate relevantType(DataFlow::Node sink, Class allocatedType) {
228+
exists(DataFlow::Node source |
229+
Flow::flow(source, sink) and
230+
isSourceImpl(source, allocatedType)
231+
)
227232
}
228233

229-
predicate isSinkImpl(DataFlow::Node sink, Class state, Type convertedType, boolean compatible) {
234+
predicate isSinkImpl(
235+
DataFlow::Node sink, Class allocatedType, Type convertedType, boolean compatible
236+
) {
230237
exists(UnsafeCast cast |
231-
relevantState(_, sink, state) and
238+
relevantType(sink, allocatedType) and
232239
sink.asExpr() = cast.getUnconverted() and
233240
convertedType = cast.getConvertedType()
234241
|
235-
if cast.compatibleWith(state) then compatible = true else compatible = false
242+
if cast.compatibleWith(allocatedType) then compatible = true else compatible = false
236243
)
237244
}
238245

239-
/**
240-
* The `BadConfig` configuration tracks flow from an allocation to an
241-
* incompatible cast.
242-
*
243-
* We use `FlowState` to track the type of the source, and compare the
244-
* flow state to the target of the cast in the `isSink` definition.
245-
*/
246-
module BadConfig implements DataFlow::StateConfigSig {
247-
class FlowState extends Class {
248-
FlowState() { relevantState(_, _, this) }
249-
}
250-
251-
predicate isSource(DataFlow::Node source, FlowState state) { relevantState(source, _, state) }
252-
253-
predicate isBarrier(DataFlow::Node node) { RelevantStateConfig::isBarrier(node) }
254-
255-
predicate isSink(DataFlow::Node sink, FlowState state) { isSinkImpl(sink, state, _, false) }
256-
257-
predicate isBarrierOut(DataFlow::Node sink, FlowState state) { isSink(sink, state) }
258-
259-
int fieldFlowBranchLimit() { result = 0 }
260-
}
261-
262-
module BadFlow = DataFlow::GlobalWithState<BadConfig>;
263-
264-
/**
265-
* The `GoodConfig` configuration tracks flow from an allocation to a
266-
* compatible cast.
267-
*
268-
* We use `GoodConfig` to reduce the number of FPs from infeasible paths.
269-
* For example, consider the following example:
270-
* ```cpp
271-
* struct Animal { virtual ~Animal(); };
272-
*
273-
* struct Cat : public Animal {
274-
* Cat();
275-
* ~Cat();
276-
* };
277-
*
278-
* struct Dog : public Animal {
279-
* Dog();
280-
* ~Dog();
281-
* };
282-
*
283-
* void test9(bool b) {
284-
* Animal* a;
285-
* if(b) {
286-
* a = new Cat;
287-
* } else {
288-
* a = new Dog;
289-
* }
290-
* if(b) {
291-
* Cat* d = static_cast<Cat*>(a);
292-
* }
293-
* }
294-
* ```
295-
* Here, `BadConfig` finds a flow from `a = new Dog` to `static_cast<Cat*>(a)`.
296-
* However, that path is never realized in an actual execution path. So in
297-
* order to remove this result we exclude results where there exists an
298-
* allocation of a type that's compatible with `static_cast<Cat*>(a)`.
299-
*
300-
* We use `FlowState` to track the type of the source, and compare the
301-
* flow state to the target of the cast in the `isSink` definition.
302-
*/
303-
module GoodConfig implements DataFlow::StateConfigSig {
304-
class FlowState = BadConfig::FlowState;
305-
306-
predicate isSource(DataFlow::Node source, FlowState state) { BadConfig::isSource(source, state) }
307-
308-
predicate isBarrier(DataFlow::Node node) { BadConfig::isBarrier(node) }
309-
310-
predicate isSink(DataFlow::Node sink, FlowState state) {
311-
isSinkImpl(sink, state, _, true) and
312-
BadFlow::flowTo(sink)
313-
}
314-
315-
int fieldFlowBranchLimit() { result = 0 }
316-
}
317-
318-
module GoodFlow = DataFlow::GlobalWithState<GoodConfig>;
319-
320246
from
321-
BadFlow::PathNode source, BadFlow::PathNode sink, Type sourceType, Type sinkType,
247+
Flow::PathNode source, Flow::PathNode sink, Type badSourceType, Type sinkType,
322248
DataFlow::Node sinkNode
323249
where
324-
BadFlow::flowPath(source, sink) and
250+
Flow::flowPath(source, sink) and
325251
sinkNode = sink.getNode() and
252+
isSourceImpl(source.getNode(), badSourceType) and
253+
isSinkImpl(sinkNode, badSourceType, sinkType, false) and
326254
// If there is any flow that would result in a valid cast then we don't
327255
// report an alert here. This reduces the number of FPs from infeasible paths
328256
// significantly.
329-
not GoodFlow::flowTo(sinkNode) and
330-
isSourceImpl(source.getNode(), sourceType) and
331-
isSinkImpl(sinkNode, _, sinkType, false)
332-
select sinkNode, source, sink, "Conversion from $@ to $@ is invalid.", sourceType,
333-
sourceType.toString(), sinkType, sinkType.toString()
257+
not exists(DataFlow::Node goodSource, Type goodSourceType |
258+
isSourceImpl(goodSource, goodSourceType) and
259+
isSinkImpl(sinkNode, goodSourceType, sinkType, true) and
260+
Flow::flow(goodSource, sinkNode)
261+
)
262+
select sinkNode, source, sink, "Conversion from $@ to $@ is invalid.", badSourceType,
263+
badSourceType.toString(), sinkType, sinkType.toString()

‎cpp/ql/test/query-tests/Security/CWE/CWE-843/TypeConfusion.expected

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,62 @@
11
edges
2+
| test.cpp:17:13:17:18 | new | test.cpp:18:21:18:47 | p | provenance | |
3+
| test.cpp:22:13:22:26 | new | test.cpp:23:12:23:30 | p | provenance | |
24
| test.cpp:27:13:27:18 | new | test.cpp:28:25:28:55 | p | provenance | |
35
| test.cpp:32:13:32:30 | new | test.cpp:33:12:33:30 | p | provenance | |
6+
| test.cpp:47:21:47:36 | new | test.cpp:48:22:48:55 | p | provenance | |
47
| test.cpp:66:15:66:21 | new | test.cpp:67:12:67:31 | a | provenance | |
8+
| test.cpp:76:15:76:21 | new | test.cpp:77:12:77:31 | a | provenance | |
9+
| test.cpp:83:9:83:15 | new | test.cpp:88:14:88:33 | a | provenance | |
510
| test.cpp:85:9:85:15 | new | test.cpp:88:14:88:33 | a | provenance | |
11+
| test.cpp:115:12:115:17 | new | test.cpp:116:20:116:51 | s2 | provenance | |
612
| test.cpp:127:12:127:17 | new | test.cpp:128:24:128:59 | s2 | provenance | |
13+
| test.cpp:140:12:140:17 | new | test.cpp:141:23:141:57 | s1 | provenance | |
714
| test.cpp:143:14:143:19 | new | test.cpp:145:28:145:68 | s1_2 | provenance | |
815
| test.cpp:153:9:153:15 | new | test.cpp:159:14:159:33 | a | provenance | |
16+
| test.cpp:166:9:166:15 | new | test.cpp:171:14:171:33 | a | provenance | |
917
| test.cpp:168:9:168:15 | new | test.cpp:171:14:171:33 | a | provenance | |
18+
| test.cpp:179:15:179:24 | new | test.cpp:181:15:181:25 | u64 | provenance | |
1019
| test.cpp:187:15:187:24 | new | test.cpp:189:25:189:45 | u64 | provenance | |
20+
| test.cpp:207:14:207:26 | new | test.cpp:209:17:209:28 | si | provenance | |
1121
| test.cpp:217:13:217:18 | new | test.cpp:218:30:218:65 | p | provenance | |
1222
| test.cpp:226:13:226:18 | new | test.cpp:227:29:227:63 | p | provenance | |
1323
nodes
24+
| test.cpp:17:13:17:18 | new | semmle.label | new |
25+
| test.cpp:18:21:18:47 | p | semmle.label | p |
26+
| test.cpp:22:13:22:26 | new | semmle.label | new |
27+
| test.cpp:23:12:23:30 | p | semmle.label | p |
1428
| test.cpp:27:13:27:18 | new | semmle.label | new |
1529
| test.cpp:28:25:28:55 | p | semmle.label | p |
1630
| test.cpp:32:13:32:30 | new | semmle.label | new |
1731
| test.cpp:33:12:33:30 | p | semmle.label | p |
32+
| test.cpp:47:21:47:36 | new | semmle.label | new |
33+
| test.cpp:48:22:48:55 | p | semmle.label | p |
1834
| test.cpp:66:15:66:21 | new | semmle.label | new |
1935
| test.cpp:67:12:67:31 | a | semmle.label | a |
36+
| test.cpp:76:15:76:21 | new | semmle.label | new |
37+
| test.cpp:77:12:77:31 | a | semmle.label | a |
38+
| test.cpp:83:9:83:15 | new | semmle.label | new |
2039
| test.cpp:85:9:85:15 | new | semmle.label | new |
2140
| test.cpp:88:14:88:33 | a | semmle.label | a |
41+
| test.cpp:115:12:115:17 | new | semmle.label | new |
42+
| test.cpp:116:20:116:51 | s2 | semmle.label | s2 |
2243
| test.cpp:127:12:127:17 | new | semmle.label | new |
2344
| test.cpp:128:24:128:59 | s2 | semmle.label | s2 |
45+
| test.cpp:140:12:140:17 | new | semmle.label | new |
46+
| test.cpp:141:23:141:57 | s1 | semmle.label | s1 |
2447
| test.cpp:143:14:143:19 | new | semmle.label | new |
2548
| test.cpp:145:28:145:68 | s1_2 | semmle.label | s1_2 |
2649
| test.cpp:153:9:153:15 | new | semmle.label | new |
2750
| test.cpp:159:14:159:33 | a | semmle.label | a |
51+
| test.cpp:166:9:166:15 | new | semmle.label | new |
2852
| test.cpp:168:9:168:15 | new | semmle.label | new |
2953
| test.cpp:171:14:171:33 | a | semmle.label | a |
54+
| test.cpp:179:15:179:24 | new | semmle.label | new |
55+
| test.cpp:181:15:181:25 | u64 | semmle.label | u64 |
3056
| test.cpp:187:15:187:24 | new | semmle.label | new |
3157
| test.cpp:189:25:189:45 | u64 | semmle.label | u64 |
58+
| test.cpp:207:14:207:26 | new | semmle.label | new |
59+
| test.cpp:209:17:209:28 | si | semmle.label | si |
3260
| test.cpp:217:13:217:18 | new | semmle.label | new |
3361
| test.cpp:218:30:218:65 | p | semmle.label | p |
3462
| test.cpp:226:13:226:18 | new | semmle.label | new |

0 commit comments

Comments
 (0)