Merge pull request #13772 from atorralba/atorralba/java/inputstream-wrapper-read-step

Java: Add taint steps for InputStream wrappers

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.

java/ql/lib/semmle/code/java/JDK.qll

@@ -177,6 +177,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") }

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
 }
 
 /**

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
   |

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. */

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

java/ql/lib/semmle/code/java/frameworks/InputStream.qll

@@ -0,0 +1,90 @@
+/** Provides definitions related to `java.io.InputStream`. */
+
+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 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,
+ * to a class instance expression of the type extending `InputStream`.
+ *
+ * This models how a subtype of `InputStream` could be tainted by capturing tainted variables in
+ * the definition of its methods.
+ */
+private class InputStreamWrapperCapturedLocalStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(InputStreamRead m, NestedClass wrapper, SsaVariable captured, SsaImplicitInit capturer |
+      wrapper.getASourceSupertype+() instanceof TypeInputStream and
+      m.getDeclaringType() = wrapper 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())
+    )
+  }
+}
+
+/**
+ * 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
+    )
+  }
+}
+
+private class InputStreamRead extends Method {
+  InputStreamRead() {
+    this.hasName("read") and
+    this.getDeclaringType().getASourceSupertype*() instanceof TypeInputStream
+  }
+}

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()

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

java/ql/test/library-tests/dataflow/stream-read/A.java

@@ -0,0 +1,139 @@
+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
+    }
+
+    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)); // $ SPURIOUS: hasTaintFlow
+        sink(wrapStream(source())); // $ hasTaintFlow
+    }
+
+    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 {
+            @Override
+            public int read() throws IOException {
+                return 0;
+            }
+
+            @Override
+            public int read(byte[] b) throws IOException {
+                return in.read(b);
+            }
+        }
+        sink(new LocalInputStream()); // $ hasTaintFlow
+    }
+}

java/ql/test/library-tests/dataflow/stream-read/test.expected

@@ -0,0 +1,2 @@
+failures
+testFailures

java/ql/test/library-tests/dataflow/stream-read/test.ql

@@ -0,0 +1,2 @@
+import TestUtilities.InlineFlowTest
+import DefaultFlowTest