Skip to content

Shared: restrict flow after using implicit read #17262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 26, 2024
Merged

Shared: restrict flow after using implicit read #17262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81239dc
Java: add test case
asgerf Aug 20, 2024
c3b3632
Shared: prevent use-use flow through implicit reads (part 1)
asgerf Aug 22, 2024
6bc8407
Java: Update test output
asgerf Aug 20, 2024
9703f67
Test output updates that only affect nodes/edges
asgerf Aug 23, 2024
d27b28d
C++: update test output
asgerf Aug 21, 2024
8df7fbf
Swift: update test output
asgerf Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,7 @@ WARNING: module 'DataFlow' has been deprecated and may be removed in future (tai
WARNING: module 'DataFlow' has been deprecated and may be removed in future (taint.ql:68,25-33)
WARNING: module 'TaintTracking' has been deprecated and may be removed in future (taint.ql:73,20-33)
testFailures
| taint.cpp:453:23:453:42 | // $ ir MISSING: ast | Missing result:ir= |
| vector.cpp:118:12:118:30 | // $ ir MISSING:ast | Missing result:ir= |
| vector.cpp:119:12:119:30 | // $ ir MISSING:ast | Missing result:ir= |
failures
6 changes: 0 additions & 6 deletions java/ql/test-kotlin1/library-tests/dataflow/summaries/test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,14 @@ edges
| apply.kt:6:28:6:41 | $this$apply : String | apply.kt:6:35:6:38 | this | provenance | |
| apply.kt:7:14:7:25 | taint(...) : String | apply.kt:7:14:7:40 | apply(...) | provenance | MaD:31 |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:7:14:7:14 | l | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List [<element>] : String | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:9:19:9:19 | l : List [<element>] : String | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
| list.kt:6:16:6:25 | taint(...) : String | list.kt:6:9:6:9 | l [post update] : List [<element>] : String | provenance | MaD:27 |
| list.kt:8:14:8:14 | l : List | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
| list.kt:8:14:8:14 | l : List [<element>] : String | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
| list.kt:9:19:9:19 | l : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:14:14:14:14 | a | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:14 | a : String[] [[]] : String | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:16:19:16:19 | a : String[] [[]] : String | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
| list.kt:13:25:13:34 | taint(...) : String | list.kt:13:17:13:40 | {...} : String[] [[]] : String | provenance | |
| list.kt:15:14:15:14 | a : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
| list.kt:16:19:16:19 | a : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
Expand Down Expand Up @@ -134,7 +129,6 @@ nodes
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | semmle.label | l [post update] : List [<element>] : String |
| list.kt:6:16:6:25 | taint(...) : String | semmle.label | taint(...) : String |
| list.kt:7:14:7:14 | l | semmle.label | l |
| list.kt:8:14:8:14 | l : List | semmle.label | l : List |
| list.kt:8:14:8:14 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |
| list.kt:8:14:8:17 | get(...) | semmle.label | get(...) |
| list.kt:9:19:9:19 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |
Expand Down
6 changes: 0 additions & 6 deletions java/ql/test-kotlin2/library-tests/dataflow/summaries/test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,14 @@ edges
| apply.kt:6:28:6:41 | $this$apply : String | apply.kt:6:35:6:38 | this | provenance | |
| apply.kt:7:14:7:25 | taint(...) : String | apply.kt:7:14:7:40 | apply(...) | provenance | MaD:31 |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:7:14:7:14 | l | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:8:14:8:14 | l : List [<element>] : String | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:9:19:9:19 | l : List [<element>] : String | provenance | |
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
| list.kt:6:16:6:25 | taint(...) : String | list.kt:6:9:6:9 | l [post update] : List [<element>] : String | provenance | MaD:27 |
| list.kt:8:14:8:14 | l : List | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
| list.kt:8:14:8:14 | l : List [<element>] : String | list.kt:8:14:8:17 | get(...) | provenance | MaD:26 |
| list.kt:9:19:9:19 | l : List [<element>] : String | list.kt:10:18:10:18 | s | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:14:14:14:14 | a | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:14 | a : String[] [[]] : String | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:16:19:16:19 | a : String[] [[]] : String | provenance | |
| list.kt:13:17:13:40 | {...} : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
| list.kt:13:25:13:34 | taint(...) : String | list.kt:13:17:13:40 | {...} : String[] [[]] : String | provenance | |
| list.kt:15:14:15:14 | a : String[] [[]] : String | list.kt:15:14:15:17 | ...[...] | provenance | |
| list.kt:16:19:16:19 | a : String[] [[]] : String | list.kt:17:18:17:18 | s | provenance | |
Expand Down Expand Up @@ -134,7 +129,6 @@ nodes
| list.kt:6:9:6:9 | l [post update] : List [<element>] : String | semmle.label | l [post update] : List [<element>] : String |
| list.kt:6:16:6:25 | taint(...) : String | semmle.label | taint(...) : String |
| list.kt:7:14:7:14 | l | semmle.label | l |
| list.kt:8:14:8:14 | l : List | semmle.label | l : List |
| list.kt:8:14:8:14 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |
| list.kt:8:14:8:17 | get(...) | semmle.label | get(...) |
| list.kt:9:19:9:19 | l : List [<element>] : String | semmle.label | l : List [<element>] : String |
Expand Down
4 changes: 1 addition & 3 deletions java/ql/test/experimental/query-tests/security/CWE-200/SensitiveAndroidFileLeak.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ edges
| FileService.java:20:31:20:43 | intent : Intent | FileService.java:21:28:21:33 | intent : Intent | provenance | |
| FileService.java:21:28:21:33 | intent : Intent | FileService.java:21:28:21:64 | getStringExtra(...) : String | provenance | MaD:2 |
| FileService.java:21:28:21:64 | getStringExtra(...) : String | FileService.java:25:42:25:50 | localPath : String | provenance | |
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | FileService.java:40:41:40:55 | params : Object[] | provenance | Config |
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | provenance | |
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | FileService.java:40:41:40:55 | params : Object[] | provenance | Config |
| FileService.java:25:42:25:50 | localPath : String | FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | provenance | |
| FileService.java:25:42:25:50 | localPath : String | FileService.java:32:13:32:28 | sourceUri : String | provenance | |
| FileService.java:32:13:32:28 | sourceUri : String | FileService.java:35:17:35:25 | sourceUri : String | provenance | |
Expand All @@ -33,7 +32,6 @@ nodes
| FileService.java:20:31:20:43 | intent : Intent | semmle.label | intent : Intent |
| FileService.java:21:28:21:33 | intent : Intent | semmle.label | intent : Intent |
| FileService.java:21:28:21:64 | getStringExtra(...) : String | semmle.label | getStringExtra(...) : String |
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] | semmle.label | makeParamsToExecute(...) : Object[] |
| FileService.java:25:13:25:51 | makeParamsToExecute(...) : Object[] [[]] : String | semmle.label | makeParamsToExecute(...) : Object[] [[]] : String |
| FileService.java:25:42:25:50 | localPath : String | semmle.label | localPath : String |
| FileService.java:32:13:32:28 | sourceUri : String | semmle.label | sourceUri : String |
Expand Down
27 changes: 27 additions & 0 deletions java/ql/test/library-tests/dataflow/implicit-read/A.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
public class A {
String field;

static String source(String name) {
return name;
}

static void sink(Object o) {}

static String step(Object o) {
return "";
}

static Object getA() {
A a = new A();
a.field = source("source");
return a;
}

static void test() {
Object object = getA();

sink(step(object)); // $ hasTaintFlow=source
sink(object);
sink(((A)object).field); // $ hasTaintFlow=source
}
}
Empty file added java/ql/test/library-tests/dataflow/implicit-read/test.expected
View file
Open in desktop
Empty file.
22 changes: 22 additions & 0 deletions java/ql/test/library-tests/dataflow/implicit-read/test.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import java
import TestUtilities.InlineFlowTest

module TestConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { DefaultFlowConfig::isSource(source) }

predicate isSink(DataFlow::Node sink) { DefaultFlowConfig::isSink(sink) }

predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
exists(MethodCall call |
call.getMethod().getName() = "step" and
node1.asExpr() = call.getArgument(0) and
node2.asExpr() = call
)
}

predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet content) {
isAdditionalFlowStep(node, _) and content instanceof DataFlow::FieldContent
}
}

import TaintFlowTest<TestConfig>
5 changes: 0 additions & 5 deletions java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,12 +745,9 @@ edges
| ArrayUtilsTest.java:68:27:68:57 | {...} : int[] [[]] : Number | ArrayUtilsTest.java:69:56:69:66 | taintedInts : int[] [[]] : Number | provenance | |
| ArrayUtilsTest.java:68:39:68:55 | taint(...) : Number | ArrayUtilsTest.java:68:27:68:57 | {...} : int[] [[]] : Number | provenance | |
| ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | ArrayUtilsTest.java:70:12:70:27 | taintedBoxedInts | provenance | |
| ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] | provenance | |
| ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] [[]] : Number | provenance | |
| ArrayUtilsTest.java:69:56:69:66 | taintedInts : int[] [[]] : Number | ArrayUtilsTest.java:69:36:69:67 | toObject(...) : Integer[] [[]] : Number | provenance | MaD:53 |
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Number | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) | provenance | |
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Object | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) | provenance | |
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Object | provenance | MaD:54 |
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] [[]] : Number | ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Number | provenance | MaD:54 |
| ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) : int[] [[]] : Number | ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) | provenance | |
| ArrayUtilsTest.java:72:53:72:69 | taint(...) : Number | ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) : int[] [[]] : Number | provenance | MaD:55 |
Expand Down Expand Up @@ -3434,8 +3431,6 @@ nodes
| ArrayUtilsTest.java:70:12:70:27 | taintedBoxedInts | semmle.label | taintedBoxedInts |
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) | semmle.label | toPrimitive(...) |
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Number | semmle.label | toPrimitive(...) : int[] [[]] : Number |
| ArrayUtilsTest.java:71:12:71:51 | toPrimitive(...) : int[] [[]] : Object | semmle.label | toPrimitive(...) : int[] [[]] : Object |
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] | semmle.label | taintedBoxedInts : Integer[] |
| ArrayUtilsTest.java:71:35:71:50 | taintedBoxedInts : Integer[] [[]] : Number | semmle.label | taintedBoxedInts : Integer[] [[]] : Number |
| ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) | semmle.label | toPrimitive(...) |
| ArrayUtilsTest.java:72:12:72:70 | toPrimitive(...) : int[] [[]] : Number | semmle.label | toPrimitive(...) : int[] [[]] : Number |
Expand Down
46 changes: 27 additions & 19 deletions shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,11 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {

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

/** Gets the corresponding Node if this is a normal node or its post-implicit read node. */
Node asNodeOrImplicitRead() {
this = TNodeNormal(result) or this = TNodeImplicitRead(result, true)
}

predicate isImplicitReadNode(Node n, boolean hasRead) { this = TNodeImplicitRead(n, hasRead) }

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

/** If `node` corresponds to a sink, gets the normal node for that sink. */
pragma[nomagic]
private NodeEx toNormalSinkNodeEx(NodeEx node) {
exists(Node n |
node.asNodeOrImplicitRead() = n and
(Config::isSink(n) or Config::isSink(n, _)) and
result.asNode() = n
)
}

private predicate inBarrier(NodeEx node) {
exists(Node n |
node.asNode() = n and
Expand All @@ -260,7 +275,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {

private predicate outBarrier(NodeEx node) {
exists(Node n |
node.asNode() = n and
node.asNodeOrImplicitRead() = n and
Config::isBarrierOut(n)
|
Config::isSink(n, _)
Expand All @@ -272,7 +287,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
pragma[nomagic]
private predicate outBarrier(NodeEx node, FlowState state) {
exists(Node n |
node.asNode() = n and
node.asNodeOrImplicitRead() = n and
Config::isBarrierOut(n, state)
|
Config::isSink(n, state)
Expand Down Expand Up @@ -318,7 +333,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {

pragma[nomagic]
private predicate sinkNodeWithState(NodeEx node, FlowState state) {
Config::isSink(node.asNode(), state) and
Config::isSink(node.asNodeOrImplicitRead(), state) and
not fullBarrier(node) and
not stateBarrier(node, state)
}
Expand Down Expand Up @@ -380,26 +395,19 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
*/
private predicate additionalLocalFlowStep(NodeEx node1, NodeEx node2, string model) {
exists(Node n1, Node n2 |
node1.asNode() = n1 and
node1.asNodeOrImplicitRead() = n1 and
node2.asNode() = n2 and
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), pragma[only_bind_into](n2), model) and
getNodeEnclosingCallable(n1) = getNodeEnclosingCallable(n2) and
stepFilter(node1, node2)
)
or
exists(Node n |
node1.isImplicitReadNode(n, true) and
node2.asNode() = n and
not fullBarrier(node2) and
model = ""
)
}

private predicate additionalLocalStateStep(
NodeEx node1, FlowState s1, NodeEx node2, FlowState s2
) {
exists(Node n1, Node n2 |
node1.asNode() = n1 and
node1.asNodeOrImplicitRead() = n1 and
node2.asNode() = n2 and
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), s1, pragma[only_bind_into](n2), s2) and
getNodeEnclosingCallable(n1) = getNodeEnclosingCallable(n2) and
Expand All @@ -425,7 +433,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
*/
private predicate additionalJumpStep(NodeEx node1, NodeEx node2, string model) {
exists(Node n1, Node n2 |
node1.asNode() = n1 and
node1.asNodeOrImplicitRead() = n1 and
node2.asNode() = n2 and
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), pragma[only_bind_into](n2), model) and
getNodeEnclosingCallable(n1) != getNodeEnclosingCallable(n2) and
Expand All @@ -436,7 +444,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {

private predicate additionalJumpStateStep(NodeEx node1, FlowState s1, NodeEx node2, FlowState s2) {
exists(Node n1, Node n2 |
node1.asNode() = n1 and
node1.asNodeOrImplicitRead() = n1 and
node2.asNode() = n2 and
Config::isAdditionalFlowStep(pragma[only_bind_into](n1), s1, pragma[only_bind_into](n2), s2) and
getNodeEnclosingCallable(n1) != getNodeEnclosingCallable(n2) and
Expand Down Expand Up @@ -729,7 +737,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
additional predicate sinkNode(NodeEx node, FlowState state) {
fwdFlow(node) and
fwdFlowState(state) and
Config::isSink(node.asNode())
Config::isSink(node.asNodeOrImplicitRead())
or
fwdFlow(node) and
fwdFlowState(state) and
Expand Down Expand Up @@ -1052,7 +1060,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {

private predicate sinkModel(NodeEx node, string model) {
sinkNode(node, _) and
exists(Node n | n = node.asNode() |
exists(Node n | n = node.asNodeOrImplicitRead() |
knownSinkModel(n, model)
or
not knownSinkModel(n, _) and model = ""
Expand Down Expand Up @@ -2549,7 +2557,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
TPathNodeSink(NodeEx node, FlowState state) {
exists(PathNodeMid sink |
sink.isAtSink() and
node = sink.getNodeEx() and
node = toNormalSinkNodeEx(sink.getNodeEx()) and
state = sink.getState()
)
} or
Expand Down Expand Up @@ -2772,7 +2780,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
PathNodeSink projectToSink(string model) {
this.isAtSink() and
sinkModel(node, model) and
result.getNodeEx() = node and
result.getNodeEx() = toNormalSinkNodeEx(node) and
result.getState() = state
}
}
Expand Down Expand Up @@ -4851,7 +4859,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
private predicate revSinkNode(NodeEx node, FlowState state) {
sinkNodeWithState(node, state)
or
Config::isSink(node.asNode()) and
Config::isSink(node.asNodeOrImplicitRead()) and
relevantState(state) and
not fullBarrier(node) and
not stateBarrier(node, state)
Expand Down
Loading
Loading