Merge pull request #10360 from atorralba/atorralba/fix-taint-implicit-reads

atorralba · web-flow · commit 569fad667af3 · 2022-09-09T14:28:39.000+02:00
Dataflow: Fix implicit reads in taint tracking when FlowStates are used
diff --git a/cpp/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md b/cpp/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md
@@ -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.
diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking2/TaintTrackingImpl.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking3/TaintTrackingImpl.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/tainttracking3/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/csharp/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md b/csharp/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md
@@ -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.
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking3/TaintTrackingImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking3/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking4/TaintTrackingImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking4/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking5/TaintTrackingImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/tainttracking5/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/java/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md b/java/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md
@@ -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.
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking2/TaintTrackingImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking3/TaintTrackingImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking3/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/java/ql/test/library-tests/dataflow/state/A.java b/java/ql/test/library-tests/dataflow/state/A.java
@@ -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");
@@ -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
+  }
 }
diff --git a/java/ql/test/library-tests/dataflow/state/test.ql b/java/ql/test/library-tests/dataflow/state/test.ql
@@ -1,5 +1,5 @@
 import java
-import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.TaintTracking
 import TestUtilities.InlineExpectationsTest
 import DataFlow
 
@@ -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)
   }
 
diff --git a/python/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md b/python/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md
@@ -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.
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking1/TaintTrackingImpl.qll b/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking1/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking2/TaintTrackingImpl.qll b/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking2/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking3/TaintTrackingImpl.qll b/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking3/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking4/TaintTrackingImpl.qll b/python/ql/lib/semmle/python/dataflow/new/internal/tainttracking4/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/ruby/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md b/ruby/ql/lib/change-notes/2022-09-08-implicit-read-flowstates.md
@@ -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.
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforlibraries/TaintTrackingImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforlibraries/TaintTrackingImpl.qll
@@ -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)
   }
 
diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/swift/ql/lib/codeql/swift/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
@@ -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)
   }
 

-Original file 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.
Original file line number	Diff line number	Diff line change
`@@ -172,7 +172,12 @@ abstract class Configuration extends DataFlow::Configuration {`
`172`	`172`	`}`
`173`	`173`
`174`	`174`	`override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {`
`175`		`- (this.isSink(node) or this.isAdditionalTaintStep(node, _)) and`
	`175`	`+ (`
	`176`	`+ this.isSink(node) or`
	`177`	`+ this.isSink(node, _) or`
	`178`	`+ this.isAdditionalTaintStep(node, _) or`
	`179`	`+ this.isAdditionalTaintStep(node, _, _, _)`
	`180`	`+ ) and`
`176`	`181`	`defaultImplicitTaintRead(node, c)`
`177`	`182`	`}`
`178`	`183`