From 00e0e5a61a9570b19ede598668ca6eb074ddedbf Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Wed, 19 Jul 2023 11:36:09 +0200
Subject: [PATCH 01/12] Java: Add taint step for InputStream wrappers

---
 java/ql/lib/semmle/code/java/JDK.qll          | 48 ++++++++++
 .../code/java/dataflow/RangeAnalysis.qll      |  2 +-
 .../library-tests/dataflow/stream-read/A.java | 87 +++++++++++++++++++
 .../dataflow/stream-read/test.expected        |  2 +
 .../dataflow/stream-read/test.ql              |  2 +
 5 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 java/ql/test/library-tests/dataflow/stream-read/A.java
 create mode 100644 java/ql/test/library-tests/dataflow/stream-read/test.expected
 create mode 100644 java/ql/test/library-tests/dataflow/stream-read/test.ql

diff --git a/java/ql/lib/semmle/code/java/JDK.qll b/java/ql/lib/semmle/code/java/JDK.qll
index 156cbbc0f93..59c132e16c9 100644
--- a/java/ql/lib/semmle/code/java/JDK.qll
+++ b/java/ql/lib/semmle/code/java/JDK.qll
@@ -4,6 +4,7 @@
 
 import Member
 import semmle.code.java.security.ExternalProcess
+private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSteps
 
 // --- Standard types ---
@@ -177,6 +178,11 @@ class TypeObjectInputStream extends RefType {
   TypeObjectInputStream() { this.hasQualifiedName("java.io", "ObjectInputStream") }
 }
 
+/** The class `java.io.InputStream`. */
+class TypeInputStream extends RefType {
+  TypeInputStream() { this.hasQualifiedName("java.io", "InputStream") }
+}
+
 /** The class `java.nio.file.Paths`. */
 class TypePaths extends Class {
   TypePaths() { this.hasQualifiedName("java.nio.file", "Paths") }
@@ -197,6 +203,48 @@ class TypeFile extends Class {
   TypeFile() { this.hasQualifiedName("java.io", "File") }
 }
 
+/**
+ * A taint step from an update of the `bytes[]` parameter in an override of the `InputStream.read` method
+ * to a class instance expression of the type extending `InputStream`.
+ *
+ * This models how a subtype of `InputStream` could be tainted by the definition of its methods, which will
+ * normally only happen in anonymous classes.
+ */
+private class InputStreamWrapperAnonymousStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(Method m, AnonymousClass wrapper |
+      m.hasName("read") and
+      m.getDeclaringType() = wrapper and
+      wrapper.getASourceSupertype+() instanceof TypeInputStream
+    |
+      n1.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess() and
+      n2.asExpr() = wrapper.getClassInstanceExpr()
+    )
+  }
+}
+
+/**
+ * A taint step from an `InputStream` argument of the constructor of an `InputStream` subtype
+ * to the call of the constructor, only if the argument is assigned to a class field.
+ *
+ * This models how it's assumed that an `InputStream` wrapper is tainted by the wrapped stream,
+ * and is a workaround to low `fieldFlowBranchLimit`s in dataflow configurations.
+ */
+private class InputStreamWrapperConstructorStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(ClassInstanceExpr cc, Argument a, AssignExpr ae |
+      cc.getConstructedType().getASourceSupertype+() instanceof TypeInputStream and
+      cc.getAnArgument() = a and
+      cc.getCallee().getParameter(a.getParameterPos()).getAnAccess() = ae.getRhs() and
+      ae.getDest().(FieldWrite).getField().getType().(RefType).getASourceSupertype*() instanceof
+        TypeInputStream
+    |
+      n1.asExpr() = a and
+      n2.asExpr() = cc
+    )
+  }
+}
+
 // --- Standard methods ---
 /**
  * DEPRECATED: Any constructor of class `java.lang.ProcessBuilder`.
diff --git a/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll b/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll
index c061f559251..53c0a83a536 100644
--- a/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll
@@ -757,7 +757,7 @@ private predicate baseBound(Expr e, int b, boolean upper) {
   or
   exists(Method read |
     e.(MethodAccess).getMethod().overrides*(read) and
-    read.getDeclaringType().hasQualifiedName("java.io", "InputStream") and
+    read.getDeclaringType() instanceof TypeInputStream and
     read.hasName("read") and
     read.getNumberOfParameters() = 0
   |
diff --git a/java/ql/test/library-tests/dataflow/stream-read/A.java b/java/ql/test/library-tests/dataflow/stream-read/A.java
new file mode 100644
index 00000000000..a1b208a898a
--- /dev/null
+++ b/java/ql/test/library-tests/dataflow/stream-read/A.java
@@ -0,0 +1,87 @@
+import java.io.InputStream;
+import java.io.IOException;
+
+public class A {
+
+    private static InputStream source() {
+        return null;
+    }
+
+    private static void sink(Object s) {}
+
+    static class MyStream extends InputStream {
+        private InputStream wrapped;
+
+        MyStream(InputStream wrapped) {
+            this.wrapped = wrapped;
+        }
+
+        @Override
+        public int read() throws IOException {
+            return 0;
+        }
+
+        @Override
+        public int read(byte[] b) throws IOException {
+            return wrapped.read(b);
+        }
+    }
+
+    public static void testSeveralWrappers() {
+        InputStream src = source();
+        InputStream wrapper1 = new MyStream(src);
+        sink(wrapper1); // $ hasTaintFlow
+        InputStream wrapper2 = new MyStream(wrapper1);
+        sink(wrapper2); // $ hasTaintFlow
+        InputStream wrapper3 = new MyStream(wrapper2);
+        sink(wrapper3); // $ hasTaintFlow
+
+        InputStream wrapper4 = new InputStream() {
+            @Override
+            public int read() throws IOException {
+                return 0;
+            }
+
+            @Override
+            public int read(byte[] b) throws IOException {
+                return wrapper3.read(b);
+
+            }
+        };
+        sink(wrapper4); // $ hasTaintFlow
+    }
+
+    public static void testAnonymous() throws Exception {
+        InputStream wrapper = new InputStream() {
+            @Override
+            public int read() throws IOException {
+                return 0;
+            }
+
+            @Override
+            public int read(byte[] b) throws IOException {
+                InputStream in = source();
+                return in.read(b);
+            }
+        };
+        sink(wrapper); // $ hasTaintFlow
+    }
+
+    public static void testAnonymousVarCapture() throws Exception {
+        InputStream in = source();
+        InputStream wrapper = new InputStream() {
+            @Override
+            public int read() throws IOException {
+                return 0;
+            }
+
+            @Override
+            public int read(byte[] b) throws IOException {
+                return in.read(b);
+
+            }
+        };
+        sink(wrapper); // $ hasTaintFlow
+    }
+
+}
diff --git a/java/ql/test/library-tests/dataflow/stream-read/test.expected b/java/ql/test/library-tests/dataflow/stream-read/test.expected
new file mode 100644
index 00000000000..48de9172b36
--- /dev/null
+++ b/java/ql/test/library-tests/dataflow/stream-read/test.expected
@@ -0,0 +1,2 @@
+failures
+testFailures
diff --git a/java/ql/test/library-tests/dataflow/stream-read/test.ql b/java/ql/test/library-tests/dataflow/stream-read/test.ql
new file mode 100644
index 00000000000..50e3f8d2f7d
--- /dev/null
+++ b/java/ql/test/library-tests/dataflow/stream-read/test.ql
@@ -0,0 +1,2 @@
+import TestUtilities.InlineFlowTest
+import DefaultFlowTest

From 5330ce12cc1be4080af977698dee4c00b9a491da Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Wed, 19 Jul 2023 11:36:57 +0200
Subject: [PATCH 02/12] Use new TypeInputStream

---
 .../semmle/code/java/dataflow/internal/TaintTrackingUtil.qll  | 4 ++--
 java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll     | 2 +-
 .../code/java/frameworks/javaee/ejb/EJBRestrictions.qll       | 2 +-
 .../semmle/code/java/frameworks/spring/SpringController.qll   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll
index c992f92ee8a..6f8dbb1771b 100644
--- a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll
@@ -239,7 +239,7 @@ private class BulkData extends RefType {
     this.(Array).getElementType().(PrimitiveType).hasName(["byte", "char"])
     or
     exists(RefType t | this.getASourceSupertype*() = t |
-      t.hasQualifiedName("java.io", "InputStream") or
+      t instanceof TypeInputStream or
       t.hasQualifiedName("java.nio", "ByteBuffer") or
       t.hasQualifiedName("java.lang", "Readable") or
       t.hasQualifiedName("java.io", "DataInput") or
@@ -259,7 +259,7 @@ private class BulkData extends RefType {
 private predicate inputStreamWrapper(Constructor c, int argi) {
   not c.fromSource() and
   c.getParameterType(argi) instanceof BulkData and
-  c.getDeclaringType().getASourceSupertype+().hasQualifiedName("java.io", "InputStream")
+  c.getDeclaringType().getASourceSupertype+() instanceof TypeInputStream
 }
 
 /** An object construction that preserves the data flow status of any of its arguments. */
diff --git a/java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll b/java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll
index c22f77725a1..64f26685b68 100644
--- a/java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll
+++ b/java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll
@@ -102,7 +102,7 @@ private module Dispatch {
     or
     t instanceof Interface and not t.fromSource()
     or
-    t.hasQualifiedName("java.io", "InputStream")
+    t instanceof TypeInputStream
     or
     t.hasQualifiedName("java.io", "Serializable")
     or
diff --git a/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJBRestrictions.qll b/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJBRestrictions.qll
index 8df603c5d6a..461a7dc8208 100644
--- a/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJBRestrictions.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJBRestrictions.qll
@@ -317,7 +317,7 @@ class SystemSetInputStreamMethod extends Method {
   SystemSetInputStreamMethod() {
     this.hasName("setIn") and
     this.getNumberOfParameters() = 1 and
-    this.getParameter(0).getType().(RefType).hasQualifiedName("java.io", "InputStream") and
+    this.getParameter(0).getType() instanceof TypeInputStream and
     this.getDeclaringType()
         .getAnAncestor()
         .getSourceDeclaration()
diff --git a/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll b/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll
index d40d1608969..f2611d6e2d1 100644
--- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll
@@ -237,7 +237,7 @@ class SpringRequestMappingParameter extends Parameter {
 
   private predicate isExplicitlyTaintedInput() {
     // InputStream or Reader parameters allow access to the body of a request
-    this.getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "InputStream") or
+    this.getType().(RefType).getAnAncestor() instanceof TypeInputStream or
     this.getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "Reader") or
     // The SpringServletInputAnnotations allow access to the URI, request parameters, cookie values and the body of the request
     this.getAnAnnotation() instanceof SpringServletInputAnnotation or

From 3a6665b0ed9af96ad504652dd8f506a50cf63228 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Wed, 19 Jul 2023 12:51:00 +0200
Subject: [PATCH 03/12] Add change note

---
 .../lib/change-notes/2023-07-19-inputstream-wrapper-steps.md  | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 java/ql/lib/change-notes/2023-07-19-inputstream-wrapper-steps.md

diff --git a/java/ql/lib/change-notes/2023-07-19-inputstream-wrapper-steps.md b/java/ql/lib/change-notes/2023-07-19-inputstream-wrapper-steps.md
new file mode 100644
index 00000000000..aaeacf93e34
--- /dev/null
+++ b/java/ql/lib/change-notes/2023-07-19-inputstream-wrapper-steps.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added more dataflow steps for `java.io.InputStream`s that wrap other `java.io.InputStream`s.

From 0156fcc381f37f7773bd576ddf4277398f45f59d Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 11:18:55 +0200
Subject: [PATCH 04/12] Apply suggestions from code review

Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
---
 java/ql/lib/semmle/code/java/JDK.qll           |  6 +++---
 .../library-tests/dataflow/stream-read/A.java  | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/JDK.qll b/java/ql/lib/semmle/code/java/JDK.qll
index 59c132e16c9..c0c388f9308 100644
--- a/java/ql/lib/semmle/code/java/JDK.qll
+++ b/java/ql/lib/semmle/code/java/JDK.qll
@@ -232,10 +232,10 @@ private class InputStreamWrapperAnonymousStep extends AdditionalTaintStep {
  */
 private class InputStreamWrapperConstructorStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
-    exists(ClassInstanceExpr cc, Argument a, AssignExpr ae |
+    exists(ClassInstanceExpr cc, Argument a, AssignExpr ae, int pos |
       cc.getConstructedType().getASourceSupertype+() instanceof TypeInputStream and
-      cc.getAnArgument() = a and
-      cc.getCallee().getParameter(a.getParameterPos()).getAnAccess() = ae.getRhs() and
+      cc.getArgument(pragma[only_bind_into](pos)) = a and
+      cc.getCallee().getParameter(pragma[only_bind_into](pos)).getAnAccess() = ae.getRhs() and
       ae.getDest().(FieldWrite).getField().getType().(RefType).getASourceSupertype*() instanceof
         TypeInputStream
     |
diff --git a/java/ql/test/library-tests/dataflow/stream-read/A.java b/java/ql/test/library-tests/dataflow/stream-read/A.java
index a1b208a898a..e66b99400d2 100644
--- a/java/ql/test/library-tests/dataflow/stream-read/A.java
+++ b/java/ql/test/library-tests/dataflow/stream-read/A.java
@@ -84,4 +84,22 @@ public class A {
         sink(wrapper); // $ hasTaintFlow
     }
 
+    public static InputStream wrapStream(InputStream in) {
+        return new InputStream() {
+            @Override
+            public int read() throws IOException {
+                return 0;
+            }
+
+            @Override
+            public int read(byte[] b) throws IOException {
+                return in.read(b);
+            }
+        };
+    }
+
+    public static void testWrapCall() {
+        sink(wrapStream(null)); // no flow
+        sink(wrapStream(source())); // $ hasTaintFlow
+    }
 }

From 1de68457aebe91818669d27a56353d8024c88a17 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 11:18:17 +0200
Subject: [PATCH 05/12] Move steps to InputStream.qll

---
 java/ql/lib/semmle/code/java/JDK.qll          | 43 -----------------
 .../semmle/code/java/dataflow/FlowSteps.qll   |  2 +-
 .../code/java/frameworks/InputStream.qll      | 47 +++++++++++++++++++
 3 files changed, 48 insertions(+), 44 deletions(-)
 create mode 100644 java/ql/lib/semmle/code/java/frameworks/InputStream.qll

diff --git a/java/ql/lib/semmle/code/java/JDK.qll b/java/ql/lib/semmle/code/java/JDK.qll
index c0c388f9308..6372b22c8f4 100644
--- a/java/ql/lib/semmle/code/java/JDK.qll
+++ b/java/ql/lib/semmle/code/java/JDK.qll
@@ -4,7 +4,6 @@
 
 import Member
 import semmle.code.java.security.ExternalProcess
-private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSteps
 
 // --- Standard types ---
@@ -203,48 +202,6 @@ class TypeFile extends Class {
   TypeFile() { this.hasQualifiedName("java.io", "File") }
 }
 
-/**
- * A taint step from an update of the `bytes[]` parameter in an override of the `InputStream.read` method
- * to a class instance expression of the type extending `InputStream`.
- *
- * This models how a subtype of `InputStream` could be tainted by the definition of its methods, which will
- * normally only happen in anonymous classes.
- */
-private class InputStreamWrapperAnonymousStep extends AdditionalTaintStep {
-  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
-    exists(Method m, AnonymousClass wrapper |
-      m.hasName("read") and
-      m.getDeclaringType() = wrapper and
-      wrapper.getASourceSupertype+() instanceof TypeInputStream
-    |
-      n1.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess() and
-      n2.asExpr() = wrapper.getClassInstanceExpr()
-    )
-  }
-}
-
-/**
- * A taint step from an `InputStream` argument of the constructor of an `InputStream` subtype
- * to the call of the constructor, only if the argument is assigned to a class field.
- *
- * This models how it's assumed that an `InputStream` wrapper is tainted by the wrapped stream,
- * and is a workaround to low `fieldFlowBranchLimit`s in dataflow configurations.
- */
-private class InputStreamWrapperConstructorStep extends AdditionalTaintStep {
-  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
-    exists(ClassInstanceExpr cc, Argument a, AssignExpr ae, int pos |
-      cc.getConstructedType().getASourceSupertype+() instanceof TypeInputStream and
-      cc.getArgument(pragma[only_bind_into](pos)) = a and
-      cc.getCallee().getParameter(pragma[only_bind_into](pos)).getAnAccess() = ae.getRhs() and
-      ae.getDest().(FieldWrite).getField().getType().(RefType).getASourceSupertype*() instanceof
-        TypeInputStream
-    |
-      n1.asExpr() = a and
-      n2.asExpr() = cc
-    )
-  }
-}
-
 // --- Standard methods ---
 /**
  * DEPRECATED: Any constructor of class `java.lang.ProcessBuilder`.
diff --git a/java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll b/java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll
index 1619965f0f0..fef69bec7fd 100644
--- a/java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll
@@ -20,11 +20,11 @@ private module Frameworks {
   private import semmle.code.java.frameworks.Guice
   private import semmle.code.java.frameworks.IoJsonWebToken
   private import semmle.code.java.frameworks.jackson.JacksonSerializability
+  private import semmle.code.java.frameworks.InputStream
   private import semmle.code.java.frameworks.Properties
   private import semmle.code.java.frameworks.Protobuf
   private import semmle.code.java.frameworks.ratpack.RatpackExec
   private import semmle.code.java.frameworks.stapler.Stapler
-  private import semmle.code.java.JDK
 }
 
 /**
diff --git a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
new file mode 100644
index 00000000000..e0c524bd1a4
--- /dev/null
+++ b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
@@ -0,0 +1,47 @@
+/** Provides definitions related to `java.io.InputStream`. */
+
+import java
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.FlowSteps
+
+/**
+ * A taint step from an update of the `bytes[]` parameter in an override of the `InputStream.read` method
+ * to a class instance expression of the type extending `InputStream`.
+ *
+ * This models how a subtype of `InputStream` could be tainted by the definition of its methods, which will
+ * normally only happen in anonymous classes.
+ */
+private class InputStreamWrapperAnonymousStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(Method m, AnonymousClass wrapper |
+      m.hasName("read") and
+      m.getDeclaringType() = wrapper and
+      wrapper.getASourceSupertype+() instanceof TypeInputStream
+    |
+      n1.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess() and
+      n2.asExpr() = wrapper.getClassInstanceExpr()
+    )
+  }
+}
+
+/**
+ * A taint step from an `InputStream` argument of the constructor of an `InputStream` subtype
+ * to the call of the constructor, only if the argument is assigned to a class field.
+ *
+ * This models how it's assumed that an `InputStream` wrapper is tainted by the wrapped stream,
+ * and is a workaround to low `fieldFlowBranchLimit`s in dataflow configurations.
+ */
+private class InputStreamWrapperConstructorStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(ClassInstanceExpr cc, Argument a, AssignExpr ae, int pos |
+      cc.getConstructedType().getASourceSupertype+() instanceof TypeInputStream and
+      cc.getArgument(pragma[only_bind_into](pos)) = a and
+      cc.getCallee().getParameter(pragma[only_bind_into](pos)).getAnAccess() = ae.getRhs() and
+      ae.getDest().(FieldWrite).getField().getType().(RefType).getASourceSupertype*() instanceof
+        TypeInputStream
+    |
+      n1.asExpr() = a and
+      n2.asExpr() = cc
+    )
+  }
+}

From f054f738368b6819c66b41b716dde9ce586f23cb Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 12:00:33 +0200
Subject: [PATCH 06/12] Apply suggestions from code review

Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
---
 java/ql/lib/semmle/code/java/frameworks/InputStream.qll | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
index e0c524bd1a4..2f40220855a 100644
--- a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
@@ -13,13 +13,17 @@ private import semmle.code.java.dataflow.FlowSteps
  */
 private class InputStreamWrapperAnonymousStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
-    exists(Method m, AnonymousClass wrapper |
+    exists(Method m, NestedClass wrapper |
       m.hasName("read") and
       m.getDeclaringType() = wrapper and
       wrapper.getASourceSupertype+() instanceof TypeInputStream
     |
       n1.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess() and
-      n2.asExpr() = wrapper.getClassInstanceExpr()
+      n2.asExpr()
+          .(ClassInstanceExpr)
+          .getConstructedType()
+          .getASourceSupertype*()
+          .getSourceDeclaration() = wrapper
     )
   }
 }

From 226103b246784f739dae09e049844809fe88de41 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 12:01:26 +0200
Subject: [PATCH 07/12] Add local class test

---
 .../library-tests/dataflow/stream-read/A.java   | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/java/ql/test/library-tests/dataflow/stream-read/A.java b/java/ql/test/library-tests/dataflow/stream-read/A.java
index e66b99400d2..2ca284f483d 100644
--- a/java/ql/test/library-tests/dataflow/stream-read/A.java
+++ b/java/ql/test/library-tests/dataflow/stream-read/A.java
@@ -102,4 +102,21 @@ public class A {
         sink(wrapStream(null)); // no flow
         sink(wrapStream(source())); // $ hasTaintFlow
     }
+
+    public static void testLocalClass() {
+        InputStream in = source();
+
+        class LocalInputStream extends InputStream {
+            @Override
+            public int read() throws IOException {
+                return 0;
+            }
+
+            @Override
+            public int read(byte[] b) throws IOException {
+                return in.read(b);
+            }
+        }
+        sink(new LocalInputStream()); // $ hasTaintFlow
+    }
 }

From cc5a404149ed29fe432ec59e9793f73bcf017515 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 13:51:21 +0200
Subject: [PATCH 08/12] Add more test cases

---
 .../library-tests/dataflow/stream-read/A.java | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/java/ql/test/library-tests/dataflow/stream-read/A.java b/java/ql/test/library-tests/dataflow/stream-read/A.java
index 2ca284f483d..705977757a1 100644
--- a/java/ql/test/library-tests/dataflow/stream-read/A.java
+++ b/java/ql/test/library-tests/dataflow/stream-read/A.java
@@ -103,7 +103,24 @@ public class A {
         sink(wrapStream(source())); // $ hasTaintFlow
     }
 
-    public static void testLocalClass() {
+    public static void testLocal() {
+
+        class LocalInputStream extends InputStream {
+            @Override
+            public int read() throws IOException {
+                return 0;
+            }
+
+            @Override
+            public int read(byte[] b) throws IOException {
+                InputStream in = source();
+                return in.read(b);
+            }
+        }
+        sink(new LocalInputStream()); // $ hasTaintFlow
+    }
+
+    public static void testLocalVarCapture() {
         InputStream in = source();
 
         class LocalInputStream extends InputStream {

From 36ff54b48b30076d7cb624549f537d9de36c1a01 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 13:53:12 +0200
Subject: [PATCH 09/12] Convert jump step into local step

Note that this has FNs in the test cases where the source is used locally in the nested classes' methods
---
 .../code/java/frameworks/InputStream.qll      | 34 ++++++++++++++-----
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
index 2f40220855a..27d7da1b03a 100644
--- a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
@@ -3,27 +3,36 @@
 import java
 private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSteps
+private import semmle.code.java.dataflow.SSA
+private import semmle.code.java.dataflow.TaintTracking
 
 /**
- * A taint step from an update of the `bytes[]` parameter in an override of the `InputStream.read` method
+ * A local taint step from the definition of a captured variable, the capturer of which
+ * updates the `bytes[]` parameter in an override of the `InputStream.read` method,
  * to a class instance expression of the type extending `InputStream`.
  *
- * This models how a subtype of `InputStream` could be tainted by the definition of its methods, which will
- * normally only happen in anonymous classes.
+ * This models how a subtype of `InputStream` could be tainted by capturing tainted variables in
+ * the definition of its methods.
  */
-private class InputStreamWrapperAnonymousStep extends AdditionalTaintStep {
+private class InputStreamWrapperCapturedLocalStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
-    exists(Method m, NestedClass wrapper |
-      m.hasName("read") and
+    exists(InputStreamRead m, NestedClass wrapper, SsaVariable captured, SsaImplicitInit capturer |
+      wrapper.getASourceSupertype+() instanceof TypeInputStream and
       m.getDeclaringType() = wrapper and
-      wrapper.getASourceSupertype+() instanceof TypeInputStream
-    |
-      n1.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess() and
+      capturer.captures(captured) and
+      TaintTracking::localTaint(DataFlow::exprNode(capturer.getAFirstUse()),
+        any(DataFlow::PostUpdateNode pun |
+          pun.getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess()
+        )) and
       n2.asExpr()
           .(ClassInstanceExpr)
           .getConstructedType()
           .getASourceSupertype*()
           .getSourceDeclaration() = wrapper
+    |
+      n1.asExpr() = captured.(SsaExplicitUpdate).getDefiningExpr().(VariableAssign).getSource()
+      or
+      captured.(SsaImplicitInit).isParameterDefinition(n1.asParameter())
     )
   }
 }
@@ -49,3 +58,10 @@ private class InputStreamWrapperConstructorStep extends AdditionalTaintStep {
     )
   }
 }
+
+private class InputStreamRead extends Method {
+  InputStreamRead() {
+    this.hasName("read") and
+    this.getDeclaringType().getASourceSupertype*() instanceof TypeInputStream
+  }
+}

From d3b3af8ae666fdc5d235dcc867a77f1fd6fa6328 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 13:54:09 +0200
Subject: [PATCH 10/12] Re-adds jump step

Note that this causes FP flow in the call context test cases
---
 .../code/java/frameworks/InputStream.qll      | 23 +++++++++++++++++++
 .../library-tests/dataflow/stream-read/A.java |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
index 27d7da1b03a..8f37ecc24ea 100644
--- a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
@@ -6,6 +6,29 @@ private import semmle.code.java.dataflow.FlowSteps
 private import semmle.code.java.dataflow.SSA
 private import semmle.code.java.dataflow.TaintTracking
 
+/**
+ * A jump taint step from an update of the `bytes[]` parameter in an override of the `InputStream.read` method
+ * to a class instance expression of the type extending `InputStream`.
+ *
+ * This models how a subtype of `InputStream` could be tainted by the definition of its methods, which will
+ * normally only happen in nested classes.
+ */
+private class InputStreamWrapperCapturedJumpStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(InputStreamRead m, NestedClass wrapper |
+      m.getDeclaringType() = wrapper and
+      wrapper.getASourceSupertype+() instanceof TypeInputStream
+    |
+      n1.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess() and
+      n2.asExpr()
+          .(ClassInstanceExpr)
+          .getConstructedType()
+          .getASourceSupertype*()
+          .getSourceDeclaration() = wrapper
+    )
+  }
+}
+
 /**
  * A local taint step from the definition of a captured variable, the capturer of which
  * updates the `bytes[]` parameter in an override of the `InputStream.read` method,
diff --git a/java/ql/test/library-tests/dataflow/stream-read/A.java b/java/ql/test/library-tests/dataflow/stream-read/A.java
index 705977757a1..779f95bcefa 100644
--- a/java/ql/test/library-tests/dataflow/stream-read/A.java
+++ b/java/ql/test/library-tests/dataflow/stream-read/A.java
@@ -99,7 +99,7 @@ public class A {
     }
 
     public static void testWrapCall() {
-        sink(wrapStream(null)); // no flow
+        sink(wrapStream(null)); // $ SPURIOUS: hasTaintFlow
         sink(wrapStream(source())); // $ hasTaintFlow
     }
 

From 4e7438ac5c8319386ee26ed914d1f4ba0e52fee8 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 14:15:16 +0200
Subject: [PATCH 11/12] Make sure that InputStreamWrapperCapturedLocalStep is
 indeed local

---
 java/ql/lib/semmle/code/java/frameworks/InputStream.qll | 1 +
 1 file changed, 1 insertion(+)

diff --git a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
index 8f37ecc24ea..fe433831386 100644
--- a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
@@ -39,6 +39,7 @@ private class InputStreamWrapperCapturedJumpStep extends AdditionalTaintStep {
  */
 private class InputStreamWrapperCapturedLocalStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    n1.getEnclosingCallable() = n2.getEnclosingCallable() and
     exists(InputStreamRead m, NestedClass wrapper, SsaVariable captured, SsaImplicitInit capturer |
       wrapper.getASourceSupertype+() instanceof TypeInputStream and
       m.getDeclaringType() = wrapper and

From 6c0d47f122dfcad898c5b114b2836fbb01296297 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Fri, 21 Jul 2023 15:42:12 +0200
Subject: [PATCH 12/12] Update
 java/ql/lib/semmle/code/java/frameworks/InputStream.qll

Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
---
 java/ql/lib/semmle/code/java/frameworks/InputStream.qll | 1 -
 1 file changed, 1 deletion(-)

diff --git a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
index fe433831386..8f37ecc24ea 100644
--- a/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll
@@ -39,7 +39,6 @@ private class InputStreamWrapperCapturedJumpStep extends AdditionalTaintStep {
  */
 private class InputStreamWrapperCapturedLocalStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
-    n1.getEnclosingCallable() = n2.getEnclosingCallable() and
     exists(InputStreamRead m, NestedClass wrapper, SsaVariable captured, SsaImplicitInit capturer |
       wrapper.getASourceSupertype+() instanceof TypeInputStream and
       m.getDeclaringType() = wrapper and