Skip to content

Dataflow: Fix implicit reads in taint tracking when FlowStates are used #10360

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 4 commits into from
Sep 9, 2022
Merged

Dataflow: Fix implicit reads in taint tracking when FlowStates are used #10360

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5f101d
Add implicit read FlowState test
atorralba Sep 8, 2022
1b87167
Add implicit reads for FlowState sinks and steps
atorralba Sep 8, 2022
7db1eb9
Sync files
atorralba Sep 8, 2022
1078cf0
Add change notes for all languages
atorralba Sep 9, 2022
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
7 changes: 6 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking3/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking3/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking4/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking5/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
4 changes: 4 additions & 0 deletions java/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: fix
---
* Fixed an issue in the taint tracking analysis where implicit reads were not allowed by default in sinks or additional taint steps that used flow states.
7 changes: 6 additions & 1 deletion java/ql/lib/semmle/code/java/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion java/ql/lib/semmle/code/java/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion java/ql/lib/semmle/code/java/dataflow/internal/tainttracking3/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
24 changes: 19 additions & 5 deletions java/ql/test/library-tests/dataflow/state/A.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import java.util.Map;
import java.util.function.*;

public class A {
Object source(String state) { return null; }
Object source(String state) {
return null;
}

void sink(Object x, String state) { }
void sink(Object x, String state) {}

void stateBarrier(Object x, String state) { }
void stateBarrier(Object x, String state) {}

Object step(Object x, String s1, String s2) { return null; }
Object step(Object x, String s1, String s2) {
return null;
}

void check(Object x) { }
void check(Object x) {}

void test1() {
Object x = source("A");
Expand All @@ -31,4 +36,13 @@ void test2(Supplier<Boolean> b) {
sink(x, "B"); // $ flow=B
sink(x, "C"); // $ flow=B
}

void test3(Map m) {
// Test implicit reads
Object x = source("A");
m.put("k", x);
sink(m, "A"); // $ flow=A
Object y = step(m, "A", "B");
sink(y, "B"); // $ flow=A
}
}
8 changes: 4 additions & 4 deletions java/ql/test/library-tests/dataflow/state/test.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import java
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.TaintTracking
import TestUtilities.InlineExpectationsTest
import DataFlow

Expand Down Expand Up @@ -39,16 +39,16 @@ predicate step(Node n1, Node n2, string s1, string s2) {

predicate checkNode(Node n) { n.asExpr().(Argument).getCall().getCallee().hasName("check") }

class Conf extends Configuration {
class Conf extends TaintTracking::Configuration {
Conf() { this = "qltest:state" }

override predicate isSource(Node n, FlowState s) { src(n, s) }

override predicate isSink(Node n, FlowState s) { sink(n, s) }

override predicate isBarrier(Node n, FlowState s) { bar(n, s) }
override predicate isSanitizer(Node n, FlowState s) { bar(n, s) }

override predicate isAdditionalFlowStep(Node n1, FlowState s1, Node n2, FlowState s2) {
override predicate isAdditionalTaintStep(Node n1, FlowState s1, Node n2, FlowState s2) {
step(n1, n2, s1, s2)
}

Expand Down
7 changes: 6 additions & 1 deletion python/ql/lib/semmle/python/dataflow/new/internal/tainttracking1/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion python/ql/lib/semmle/python/dataflow/new/internal/tainttracking2/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion python/ql/lib/semmle/python/dataflow/new/internal/tainttracking3/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion python/ql/lib/semmle/python/dataflow/new/internal/tainttracking4/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion ruby/ql/lib/codeql/ruby/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforlibraries/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down
7 changes: 6 additions & 1 deletion swift/ql/lib/codeql/swift/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
(this.isSink(node) or this.isAdditionalTaintStep(node, _)) and
(
this.isSink(node) or
this.isSink(node, _) or
this.isAdditionalTaintStep(node, _) or
this.isAdditionalTaintStep(node, _, _, _)
) and
defaultImplicitTaintRead(node, c)
}

Expand Down