Skip to content

Commit 7d79be7

Browse files
author
Robert Marsh
committed
C++: taint tracking conf in DefaultTaintTracking
Switch from using additional flow steps with a DataFlow::Configuration in DefaultTaintTracking to using a TaintTracking::Configuration. This makes future improvements to TaintTracking::Configuration reflected in DefaultTaintTracking without further effort. It also removes the predictability constraint in DefaultTaintTracking, which increases the number of results, with both new true positives and new false positives. Those may need to be addressed on a per-query basis. There are some additional regressions from losing pointer/object conflation for arguments. Those can be worked around by adding that conflation to TaintTracking::Configuration until precise indirect parameter flow is ready.
1 parent 2f20486 commit 7d79be7

9 files changed

Lines changed: 42 additions & 301 deletions

File tree

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ private import semmle.code.cpp.ir.dataflow.internal.DataFlowDispatch as Dispatch
99
private import semmle.code.cpp.controlflow.IRGuards
1010
private import semmle.code.cpp.models.interfaces.Taint
1111
private import semmle.code.cpp.models.interfaces.DataFlow
12+
private import semmle.code.cpp.ir.dataflow.TaintTracking2
1213

1314
/**
1415
* A predictable instruction is one where an external user can predict
@@ -113,7 +114,7 @@ private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
113114
override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
114115
}
115116

116-
private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {
117+
private class FromGlobalVarTaintTrackingCfg extends DataFlow3::Configuration {
117118
FromGlobalVarTaintTrackingCfg() { this = "FromGlobalVarTaintTrackingCfg" }
118119

119120
override predicate isSource(DataFlow::Node source) {
@@ -560,7 +561,7 @@ module TaintedWithPath {
560561
string toString() { result = "TaintTrackingConfiguration" }
561562
}
562563

563-
private class AdjustedConfiguration extends DataFlow3::Configuration {
564+
private class AdjustedConfiguration extends TaintTracking2::Configuration {
564565
AdjustedConfiguration() { this = "AdjustedConfiguration" }
565566

566567
override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
@@ -569,19 +570,17 @@ module TaintedWithPath {
569570
exists(TaintTrackingConfiguration cfg | cfg.isSink(adjustedSink(sink)))
570571
}
571572

572-
override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
573-
commonTaintStep(n1, n2)
574-
or
573+
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
575574
exists(TaintTrackingConfiguration cfg | cfg.taintThroughGlobals() |
576575
writesVariable(n1.asInstruction(), n2.asVariable().(GlobalOrNamespaceVariable))
577576
or
578577
readsVariable(n2.asInstruction(), n1.asVariable().(GlobalOrNamespaceVariable))
579578
)
580579
}
581580

582-
override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
581+
override predicate isSanitizer(DataFlow::Node node) { nodeIsBarrier(node) }
583582

584-
override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
583+
override predicate isSanitizerIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
585584
}
586585

587586
/*
@@ -599,11 +598,11 @@ module TaintedWithPath {
599598
*/
600599

601600
private newtype TPathNode =
602-
TWrapPathNode(DataFlow3::PathNode n) or
601+
TWrapPathNode(DataFlow2::PathNode n) or
603602
// There's a single newtype constructor for both sources and sinks since
604603
// that makes it easiest to deal with the case where source = sink.
605604
TEndpointPathNode(Element e) {
606-
exists(AdjustedConfiguration cfg, DataFlow3::Node sourceNode, DataFlow3::Node sinkNode |
605+
exists(AdjustedConfiguration cfg, DataFlow2::Node sourceNode, DataFlow2::Node sinkNode |
607606
cfg.hasFlow(sourceNode, sinkNode)
608607
|
609608
sourceNode = getNodeForSource(e)
@@ -633,7 +632,7 @@ module TaintedWithPath {
633632
}
634633

635634
private class WrapPathNode extends PathNode, TWrapPathNode {
636-
DataFlow3::PathNode inner() { this = TWrapPathNode(result) }
635+
DataFlow2::PathNode inner() { this = TWrapPathNode(result) }
637636

638637
override string toString() { result = this.inner().toString() }
639638

@@ -671,25 +670,25 @@ module TaintedWithPath {
671670

672671
/** Holds if `(a,b)` is an edge in the graph of data flow path explanations. */
673672
query predicate edges(PathNode a, PathNode b) {
674-
DataFlow3::PathGraph::edges(a.(WrapPathNode).inner(), b.(WrapPathNode).inner())
673+
DataFlow2::PathGraph::edges(a.(WrapPathNode).inner(), b.(WrapPathNode).inner())
675674
or
676675
// To avoid showing trivial-looking steps, we _replace_ the last node instead
677676
// of adding an edge out of it.
678677
exists(WrapPathNode sinkNode |
679-
DataFlow3::PathGraph::edges(a.(WrapPathNode).inner(), sinkNode.inner()) and
678+
DataFlow2::PathGraph::edges(a.(WrapPathNode).inner(), sinkNode.inner()) and
680679
b.(FinalPathNode).inner() = adjustedSink(sinkNode.inner().getNode())
681680
)
682681
or
683682
// Same for the first node
684683
exists(WrapPathNode sourceNode |
685-
DataFlow3::PathGraph::edges(sourceNode.inner(), b.(WrapPathNode).inner()) and
684+
DataFlow2::PathGraph::edges(sourceNode.inner(), b.(WrapPathNode).inner()) and
686685
sourceNode.inner().getNode() = getNodeForSource(a.(InitialPathNode).inner())
687686
)
688687
or
689688
// Finally, handle the case where the path goes directly from a source to a
690689
// sink, meaning that they both need to be translated.
691690
exists(WrapPathNode sinkNode, WrapPathNode sourceNode |
692-
DataFlow3::PathGraph::edges(sourceNode.inner(), sinkNode.inner()) and
691+
DataFlow2::PathGraph::edges(sourceNode.inner(), sinkNode.inner()) and
693692
sourceNode.inner().getNode() = getNodeForSource(a.(InitialPathNode).inner()) and
694693
b.(FinalPathNode).inner() = adjustedSink(sinkNode.inner().getNode())
695694
)
@@ -711,7 +710,7 @@ module TaintedWithPath {
711710
* the computation.
712711
*/
713712
predicate taintedWithPath(Expr source, Element tainted, PathNode sourceNode, PathNode sinkNode) {
714-
exists(AdjustedConfiguration cfg, DataFlow3::Node flowSource, DataFlow3::Node flowSink |
713+
exists(AdjustedConfiguration cfg, DataFlow2::Node flowSource, DataFlow2::Node flowSink |
715714
source = sourceNode.(InitialPathNode).inner() and
716715
flowSource = getNodeForSource(source) and
717716
cfg.hasFlow(flowSource, flowSink) and

‎cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/TaintedPath.expected‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,3 @@
11
edges
2-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | (const char *)... |
3-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | (const char *)... |
4-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
5-
| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
62
nodes
7-
| test.c:9:23:9:26 | argv | semmle.label | argv |
8-
| test.c:9:23:9:26 | argv | semmle.label | argv |
9-
| test.c:17:11:17:18 | (const char *)... | semmle.label | (const char *)... |
10-
| test.c:17:11:17:18 | (const char *)... | semmle.label | (const char *)... |
11-
| test.c:17:11:17:18 | fileName | semmle.label | fileName |
123
#select
13-
| test.c:17:11:17:18 | fileName | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName | This argument to a file access function is derived from $@ and then passed to fopen(filename) | test.c:9:23:9:26 | argv | user input (argv) |

‎cpp/ql/test/query-tests/Security/CWE/CWE-114/semmle/UncontrolledProcessOperation/UncontrolledProcessOperation.expected‎

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,75 +1,27 @@
11
edges
2-
| test.cpp:24:30:24:36 | *command | test.cpp:26:10:26:16 | command |
3-
| test.cpp:24:30:24:36 | *command | test.cpp:26:10:26:16 | command |
42
| test.cpp:24:30:24:36 | command | test.cpp:26:10:26:16 | command |
53
| test.cpp:24:30:24:36 | command | test.cpp:26:10:26:16 | command |
6-
| test.cpp:29:30:29:36 | *command | test.cpp:31:10:31:16 | command |
7-
| test.cpp:29:30:29:36 | *command | test.cpp:31:10:31:16 | command |
84
| test.cpp:29:30:29:36 | command | test.cpp:31:10:31:16 | command |
95
| test.cpp:29:30:29:36 | command | test.cpp:31:10:31:16 | command |
10-
| test.cpp:42:18:42:23 | call to getenv | test.cpp:24:30:24:36 | *command |
116
| test.cpp:42:18:42:23 | call to getenv | test.cpp:24:30:24:36 | command |
12-
| test.cpp:42:18:42:34 | (const char *)... | test.cpp:24:30:24:36 | *command |
137
| test.cpp:42:18:42:34 | (const char *)... | test.cpp:24:30:24:36 | command |
14-
| test.cpp:43:18:43:23 | call to getenv | test.cpp:29:30:29:36 | *command |
158
| test.cpp:43:18:43:23 | call to getenv | test.cpp:29:30:29:36 | command |
16-
| test.cpp:43:18:43:34 | (const char *)... | test.cpp:29:30:29:36 | *command |
179
| test.cpp:43:18:43:34 | (const char *)... | test.cpp:29:30:29:36 | command |
18-
| test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | (const char *)... |
19-
| test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer |
20-
| test.cpp:56:12:56:17 | buffer | test.cpp:63:10:63:13 | (const char *)... |
21-
| test.cpp:56:12:56:17 | buffer | test.cpp:63:10:63:13 | data |
22-
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:62:10:62:15 | (const char *)... |
23-
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:62:10:62:15 | buffer |
24-
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:63:10:63:13 | (const char *)... |
25-
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:63:10:63:13 | data |
26-
| test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | (const char *)... |
27-
| test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer |
28-
| test.cpp:76:12:76:17 | buffer | test.cpp:79:10:79:13 | (const char *)... |
29-
| test.cpp:76:12:76:17 | buffer | test.cpp:79:10:79:13 | data |
30-
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:78:10:78:15 | (const char *)... |
31-
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:78:10:78:15 | buffer |
32-
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:79:10:79:13 | (const char *)... |
33-
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:79:10:79:13 | data |
3410
nodes
35-
| test.cpp:24:30:24:36 | *command | semmle.label | *command |
3611
| test.cpp:24:30:24:36 | command | semmle.label | command |
3712
| test.cpp:26:10:26:16 | command | semmle.label | command |
3813
| test.cpp:26:10:26:16 | command | semmle.label | command |
3914
| test.cpp:26:10:26:16 | command | semmle.label | command |
40-
| test.cpp:29:30:29:36 | *command | semmle.label | *command |
4115
| test.cpp:29:30:29:36 | command | semmle.label | command |
4216
| test.cpp:31:10:31:16 | command | semmle.label | command |
4317
| test.cpp:31:10:31:16 | command | semmle.label | command |
4418
| test.cpp:31:10:31:16 | command | semmle.label | command |
4519
| test.cpp:42:7:42:16 | Argument 0 | semmle.label | Argument 0 |
4620
| test.cpp:42:18:42:23 | call to getenv | semmle.label | call to getenv |
4721
| test.cpp:42:18:42:34 | (const char *)... | semmle.label | (const char *)... |
48-
| test.cpp:42:18:42:34 | Argument 0 indirection | semmle.label | Argument 0 indirection |
4922
| test.cpp:43:7:43:16 | Argument 0 | semmle.label | Argument 0 |
5023
| test.cpp:43:18:43:23 | call to getenv | semmle.label | call to getenv |
5124
| test.cpp:43:18:43:34 | (const char *)... | semmle.label | (const char *)... |
52-
| test.cpp:43:18:43:34 | Argument 0 indirection | semmle.label | Argument 0 indirection |
53-
| test.cpp:56:12:56:17 | buffer | semmle.label | buffer |
54-
| test.cpp:56:12:56:17 | fgets output argument | semmle.label | fgets output argument |
55-
| test.cpp:62:10:62:15 | (const char *)... | semmle.label | (const char *)... |
56-
| test.cpp:62:10:62:15 | (const char *)... | semmle.label | (const char *)... |
57-
| test.cpp:62:10:62:15 | buffer | semmle.label | buffer |
58-
| test.cpp:63:10:63:13 | (const char *)... | semmle.label | (const char *)... |
59-
| test.cpp:63:10:63:13 | (const char *)... | semmle.label | (const char *)... |
60-
| test.cpp:63:10:63:13 | data | semmle.label | data |
61-
| test.cpp:76:12:76:17 | buffer | semmle.label | buffer |
62-
| test.cpp:76:12:76:17 | fgets output argument | semmle.label | fgets output argument |
63-
| test.cpp:78:10:78:15 | (const char *)... | semmle.label | (const char *)... |
64-
| test.cpp:78:10:78:15 | (const char *)... | semmle.label | (const char *)... |
65-
| test.cpp:78:10:78:15 | buffer | semmle.label | buffer |
66-
| test.cpp:79:10:79:13 | (const char *)... | semmle.label | (const char *)... |
67-
| test.cpp:79:10:79:13 | (const char *)... | semmle.label | (const char *)... |
68-
| test.cpp:79:10:79:13 | data | semmle.label | data |
6925
#select
7026
| test.cpp:26:10:26:16 | command | test.cpp:42:18:42:23 | call to getenv | test.cpp:26:10:26:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:42:18:42:23 | call to getenv | call to getenv |
7127
| test.cpp:31:10:31:16 | command | test.cpp:43:18:43:23 | call to getenv | test.cpp:31:10:31:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:43:18:43:23 | call to getenv | call to getenv |
72-
| test.cpp:62:10:62:15 | buffer | test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer | The value of this argument may come from $@ and is being passed to system | test.cpp:56:12:56:17 | buffer | buffer |
73-
| test.cpp:63:10:63:13 | data | test.cpp:56:12:56:17 | buffer | test.cpp:63:10:63:13 | data | The value of this argument may come from $@ and is being passed to system | test.cpp:56:12:56:17 | buffer | buffer |
74-
| test.cpp:78:10:78:15 | buffer | test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer | The value of this argument may come from $@ and is being passed to system | test.cpp:76:12:76:17 | buffer | buffer |
75-
| test.cpp:79:10:79:13 | data | test.cpp:76:12:76:17 | buffer | test.cpp:79:10:79:13 | data | The value of this argument may come from $@ and is being passed to system | test.cpp:76:12:76:17 | buffer | buffer |

0 commit comments

Comments
 (0)