Skip to content

Commit 592e2ea

Browse files
asgerfasgerf
authored
Merge pull request #17262 from asgerf/shared/implicit-read
Shared: restrict flow after using implicit read
2 parents c4c8c9d + 8df7fbf commit 592e2ea

File tree

14 files changed

+84
-112
lines changed

14 files changed

+84
-112
lines changed

‎cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,7 @@ WARNING: module 'DataFlow' has been deprecated and may be removed in future (tai
44
WARNING: module 'DataFlow' has been deprecated and may be removed in future (taint.ql:68,25-33)
55
WARNING: module 'TaintTracking' has been deprecated and may be removed in future (taint.ql:73,20-33)
66
testFailures
7+
| taint.cpp:453:23:453:42 | // $ ir MISSING: ast | Missing result:ir= |
8+
| vector.cpp:118:12:118:30 | // $ ir MISSING:ast | Missing result:ir= |
9+
| vector.cpp:119:12:119:30 | // $ ir MISSING:ast | Missing result:ir= |
710
failures

‎java/ql/test-kotlin1/library-tests/dataflow/summaries/test.expected

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,19 +36,14 @@ edges
3636
| apply.kt:6:28:6:41 | $this$apply : String | apply.kt:6:35:6:38 | this | provenance | |
3737
| apply.kt:7:14:7:25 | taint(...) : String | apply.kt:7:14:7:40 | apply(...) | provenance | MaD:31 |
3838
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:7:14:7:14 | l | provenance | |
39-
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List | provenance | |
4039
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List [<element>] : String | provenance | |
4140
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:9:19:9:19 | l : List [<element>] : String | provenance | |
42-
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
4341
| list.kt:6:16:6:25 | taint(...) : String | list.kt:6:9:6:9 | l [post update] : List [<element>] : String | provenance | MaD:27 |
44-
| list.kt:8:14:8:14 | l : List | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
4542
| list.kt:8:14:8:14 | l : List [<element>] : String | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
4643
| list.kt:9:19:9:19 | l : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
4744
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:14:14:14:14 | a | provenance | |
4845
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:14 | a : String[] [[]] : String | provenance | |
49-
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
5046
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:16:19:16:19 | a : String[] [[]] : String | provenance | |
51-
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
5247
| list.kt:13:25:13:34 | taint(...) : String | list.kt:13:17:13:40 | {...} : String[] [[]] : String | provenance | |
5348
| list.kt:15:14:15:14 | a : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
5449
| list.kt:16:19:16:19 | a : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
@@ -134,7 +129,6 @@ nodes
134129
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | semmle.label | l [post update] : List [<element>] : String |
135130
| list.kt:6:16:6:25 | taint(...) : String | semmle.label | taint(...) : String |
136131
| list.kt:7:14:7:14 | l | semmle.label | l |
137-
| list.kt:8:14:8:14 | l : List | semmle.label | l : List |
138132
| list.kt:8:14:8:14 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |
139133
| list.kt:8:14:8:17 | get(...) | semmle.label | get(...) |
140134
| list.kt:9:19:9:19 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |

‎java/ql/test-kotlin2/library-tests/dataflow/summaries/test.expected

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,19 +36,14 @@ edges
3636
| apply.kt:6:28:6:41 | $this$apply : String | apply.kt:6:35:6:38 | this | provenance | |
3737
| apply.kt:7:14:7:25 | taint(...) : String | apply.kt:7:14:7:40 | apply(...) | provenance | MaD:31 |
3838
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:7:14:7:14 | l | provenance | |
39-
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List | provenance | |
4039
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List [<element>] : String | provenance | |
4140
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:9:19:9:19 | l : List [<element>] : String | provenance | |
42-
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
4341
| list.kt:6:16:6:25 | taint(...) : String | list.kt:6:9:6:9 | l [post update] : List [<element>] : String | provenance | MaD:27 |
44-
| list.kt:8:14:8:14 | l : List | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
4542
| list.kt:8:14:8:14 | l : List [<element>] : String | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
4643
| list.kt:9:19:9:19 | l : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
4744
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:14:14:14:14 | a | provenance | |
4845
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:14 | a : String[] [[]] : String | provenance | |
49-
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
5046
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:16:19:16:19 | a : String[] [[]] : String | provenance | |
51-
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
5247
| list.kt:13:25:13:34 | taint(...) : String | list.kt:13:17:13:40 | {...} : String[] [[]] : String | provenance | |
5348
| list.kt:15:14:15:14 | a : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
5449
| list.kt:16:19:16:19 | a : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
@@ -134,7 +129,6 @@ nodes
134129
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | semmle.label | l [post update] : List [<element>] : String |
135130
| list.kt:6:16:6:25 | taint(...) : String | semmle.label | taint(...) : String |
136131
| list.kt:7:14:7:14 | l | semmle.label | l |
137-
| list.kt:8:14:8:14 | l : List | semmle.label | l : List |
138132
| list.kt:8:14:8:14 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |
139133
| list.kt:8:14:8:17 | get(...) | semmle.label | get(...) |
140134
| list.kt:9:19:9:19 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |

‎java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,7 @@ edges
66
| FileService.java:20:31:20:43 | intent : Intent | FileService.java:21:28:21:33 | intent : Intent | provenance | |
77
| FileService.java:21:28:21:33 | intent : Intent | FileService.java:21:28:21:64 | getStringExtra(...) : String | provenance | MaD:2 |
88
| FileService.java:21:28:21:64 | getStringExtra(...) : String | FileService.java:25:42:25:50 | localPath : String | provenance | |
9-
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | FileService.java:40:41:40:55 | params : Object[] | provenance | Config |
10-
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | provenance | |
9+
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | FileService.java:40:41:40:55 | params : Object[] | provenance | Config |
1110
| FileService.java:25:42:25:50 | localPath : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | provenance | |
1211
| FileService.java:25:42:25:50 | localPath : String | FileService.java:32:13:32:28 | sourceUri : String | provenance | |
1312
| FileService.java:32:13:32:28 | sourceUri : String | FileService.java:35:17:35:25 | sourceUri : String | provenance | |
@@ -33,7 +32,6 @@ nodes
3332
| FileService.java:20:31:20:43 | intent : Intent | semmle.label | intent : Intent |
3433
| FileService.java:21:28:21:33 | intent : Intent | semmle.label | intent : Intent |
3534
| FileService.java:21:28:21:64 | getStringExtra(...) : String | semmle.label | getStringExtra(...) : String |
36-
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | semmle.label | makeParamsToExecute(...) : Object[] |
3735
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | semmle.label | makeParamsToExecute(...) : Object[] [[]] : String |
3836
| FileService.java:25:42:25:50 | localPath : String | semmle.label | localPath : String |
3937
| FileService.java:32:13:32:28 | sourceUri : String | semmle.label | sourceUri : String |

‎java/ql/test/library-tests/dataflow/implicit-read/A.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
public class A {
2+
String field;
3+
4+
static String source(String name) {
5+
return name;
6+
}
7+
8+
static void sink(Object o) {}
9+
10+
static String step(Object o) {
11+
return "";
12+
}
13+
14+
static Object getA() {
15+
A a = new A();
16+
a.field = source("source");
17+
return a;
18+
}
19+
20+
static void test() {
21+
Object object = getA();
22+
23+
sink(step(object)); // $ hasTaintFlow=source
24+
sink(object);
25+
sink(((A)object).field); // $ hasTaintFlow=source
26+
}
27+
}

‎java/ql/test/library-tests/dataflow/implicit-read/test.expected

Whitespace-only changes.

‎java/ql/test/library-tests/dataflow/implicit-read/test.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import java
2+
import TestUtilities.InlineFlowTest
3+
4+
module TestConfig implements DataFlow::ConfigSig {
5+
predicate isSource(DataFlow::Node source) { DefaultFlowConfig::isSource(source) }
6+
7+
predicate isSink(DataFlow::Node sink) { DefaultFlowConfig::isSink(sink) }
8+
9+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
10+
exists(MethodCall call |
11+
call.getMethod().getName() = "step" and
12+
node1.asExpr() = call.getArgument(0) and
13+
node2.asExpr() = call
14+
)
15+
}
16+
17+
predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet content) {
18+
isAdditionalFlowStep(node, _) and content instanceof DataFlow::FieldContent
19+
}
20+
}
21+
22+
import TaintFlowTest<TestConfig>

‎java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.expected

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -745,12 +745,9 @@ edges
745745
| ArrayUtilsTest.java:68:27:68:57 | {...} : int[] [[]] : Number | ArrayUtilsTest.java:69:56:69:66 | taintedInts : int[] [[]] : Number | provenance | |
746746
| ArrayUtilsTest.java:68:39:68:55 | taint(...) : Number | ArrayUtilsTest.java:68:27:68:57 | {...} : int[] [[]] : Number | provenance | |
747747
| ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | ArrayUtilsTest.java:70:12:70:27 | taintedBoxedInts | provenance | |
748-
| ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] | provenance | |
749748
| ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] [[]] : Number | provenance | |
750749
| ArrayUtilsTest.java:69:56:69:66 | taintedInts : int[] [[]] : Number | ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | provenance | MaD:53 |
751750
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Number | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) | provenance | |
752-
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Object | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) | provenance | |
753-
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Object | provenance | MaD:54 |
754751
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] [[]] : Number | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Number | provenance | MaD:54 |
755752
| ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) : int[] [[]] : Number | ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) | provenance | |
756753
| ArrayUtilsTest.java:72:53:72:69 | taint(...) : Number | ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) : int[] [[]] : Number | provenance | MaD:55 |
@@ -3434,8 +3431,6 @@ nodes
34343431
| ArrayUtilsTest.java:70:12:70:27 | taintedBoxedInts | semmle.label | taintedBoxedInts |
34353432
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) | semmle.label | toPrimitive(...) |
34363433
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Number | semmle.label | toPrimitive(...) : int[] [[]] : Number |
3437-
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Object | semmle.label | toPrimitive(...) : int[] [[]] : Object |
3438-
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] | semmle.label | taintedBoxedInts : Integer[] |
34393434
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] [[]] : Number | semmle.label | taintedBoxedInts : Integer[] [[]] : Number |
34403435
| ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) | semmle.label | toPrimitive(...) |
34413436
| ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) : int[] [[]] : Number | semmle.label | toPrimitive(...) : int[] [[]] : Number |

‎shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,11 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
173173

174174
Node asNode() { this = TNodeNormal(result) }
175175

176+
/** Gets the corresponding Node if this is a normal node or its post-implicit read node. */
177+
Node asNodeOrImplicitRead() {
178+
this = TNodeNormal(result) or this = TNodeImplicitRead(result, true)
179+
}
180+
176181
predicate isImplicitReadNode(Node n, boolean hasRead) { this = TNodeImplicitRead(n, hasRead) }
177182

178183
ParameterNode asParamReturnNode() { this = TParamReturnNode(result, _) }
@@ -241,6 +246,16 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
241246
ReturnKindExt getKind() { result = pos.getKind() }
242247
}
243248

249+
/** If `node` corresponds to a sink, gets the normal node for that sink. */
250+
pragma[nomagic]
251+
private NodeEx toNormalSinkNodeEx(NodeEx node) {
252+
exists(Node n |
253+
node.asNodeOrImplicitRead() = n and
254+
(Config::isSink(n) or Config::isSink(n, _)) and
255+
result.asNode() = n
256+
)
257+
}
258+
244259
private predicate inBarrier(NodeEx node) {
245260
exists(Node n |
246261
node.asNode() = n and
@@ -260,7 +275,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
260275

261276
private predicate outBarrier(NodeEx node) {
262277
exists(Node n |
263-
node.asNode() = n and
278+
node.asNodeOrImplicitRead() = n and
264279
Config::isBarrierOut(n)
265280
|
266281
Config::isSink(n, _)
@@ -272,7 +287,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
272287
pragma[nomagic]
273288
private predicate outBarrier(NodeEx node, FlowState state) {
274289
exists(Node n |
275-
node.asNode() = n and
290+
node.asNodeOrImplicitRead() = n and
276291
Config::isBarrierOut(n, state)
277292
|
278293
Config::isSink(n, state)
@@ -318,7 +333,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
318333

319334
pragma[nomagic]
320335
private predicate sinkNodeWithState(NodeEx node, FlowState state) {
321-
Config::isSink(node.asNode(), state) and
336+
Config::isSink(node.asNodeOrImplicitRead(), state) and
322337
not fullBarrier(node) and
323338
not stateBarrier(node, state)
324339
}
@@ -380,26 +395,19 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
380395
*/
381396
private predicate additionalLocalFlowStep(NodeEx node1, NodeEx node2, string model) {
382397
exists(Node n1, Node n2 |
383-
node1.asNode() = n1 and
398+
node1.asNodeOrImplicitRead() = n1 and
384399
node2.asNode() = n2 and
385400
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), pragma[only_bind_into](n2), model) and
386401
getNodeEnclosingCallable(n1) = getNodeEnclosingCallable(n2) and
387402
stepFilter(node1, node2)
388403
)
389-
or
390-
exists(Node n |
391-
node1.isImplicitReadNode(n, true) and
392-
node2.asNode() = n and
393-
not fullBarrier(node2) and
394-
model = ""
395-
)
396404
}
397405

398406
private predicate additionalLocalStateStep(
399407
NodeEx node1, FlowState s1, NodeEx node2, FlowState s2
400408
) {
401409
exists(Node n1, Node n2 |
402-
node1.asNode() = n1 and
410+
node1.asNodeOrImplicitRead() = n1 and
403411
node2.asNode() = n2 and
404412
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), s1, pragma[only_bind_into](n2), s2) and
405413
getNodeEnclosingCallable(n1) = getNodeEnclosingCallable(n2) and
@@ -425,7 +433,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
425433
*/
426434
private predicate additionalJumpStep(NodeEx node1, NodeEx node2, string model) {
427435
exists(Node n1, Node n2 |
428-
node1.asNode() = n1 and
436+
node1.asNodeOrImplicitRead() = n1 and
429437
node2.asNode() = n2 and
430438
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), pragma[only_bind_into](n2), model) and
431439
getNodeEnclosingCallable(n1) != getNodeEnclosingCallable(n2) and
@@ -436,7 +444,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
436444

437445
private predicate additionalJumpStateStep(NodeEx node1, FlowState s1, NodeEx node2, FlowState s2) {
438446
exists(Node n1, Node n2 |
439-
node1.asNode() = n1 and
447+
node1.asNodeOrImplicitRead() = n1 and
440448
node2.asNode() = n2 and
441449
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), s1, pragma[only_bind_into](n2), s2) and
442450
getNodeEnclosingCallable(n1) != getNodeEnclosingCallable(n2) and
@@ -729,7 +737,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
729737
additional predicate sinkNode(NodeEx node, FlowState state) {
730738
fwdFlow(node) and
731739
fwdFlowState(state) and
732-
Config::isSink(node.asNode())
740+
Config::isSink(node.asNodeOrImplicitRead())
733741
or
734742
fwdFlow(node) and
735743
fwdFlowState(state) and
@@ -1052,7 +1060,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
10521060

10531061
private predicate sinkModel(NodeEx node, string model) {
10541062
sinkNode(node, _) and
1055-
exists(Node n | n = node.asNode() |
1063+
exists(Node n | n = node.asNodeOrImplicitRead() |
10561064
knownSinkModel(n, model)
10571065
or
10581066
not knownSinkModel(n, _) and model = ""
@@ -2549,7 +2557,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
25492557
TPathNodeSink(NodeEx node, FlowState state) {
25502558
exists(PathNodeMid sink |
25512559
sink.isAtSink() and
2552-
node = sink.getNodeEx() and
2560+
node = toNormalSinkNodeEx(sink.getNodeEx()) and
25532561
state = sink.getState()
25542562
)
25552563
} or
@@ -2772,7 +2780,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
27722780
PathNodeSink projectToSink(string model) {
27732781
this.isAtSink() and
27742782
sinkModel(node, model) and
2775-
result.getNodeEx() = node and
2783+
result.getNodeEx() = toNormalSinkNodeEx(node) and
27762784
result.getState() = state
27772785
}
27782786
}
@@ -4851,7 +4859,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
48514859
private predicate revSinkNode(NodeEx node, FlowState state) {
48524860
sinkNodeWithState(node, state)
48534861
or
4854-
Config::isSink(node.asNode()) and
4862+
Config::isSink(node.asNodeOrImplicitRead()) and
48554863
relevantState(state) and
48564864
not fullBarrier(node) and
48574865
not stateBarrier(node, state)

0 commit comments

Comments
 (0)