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. diff --git a/java/ql/lib/semmle/code/java/JDK.qll b/java/ql/lib/semmle/code/java/JDK.qll index 156cbbc0f93..6372b22c8f4 100644 --- a/java/ql/lib/semmle/code/java/JDK.qll +++ b/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") } 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/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/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/InputStream.qll b/java/ql/lib/semmle/code/java/frameworks/InputStream.qll new file mode 100644 index 00000000000..8f37ecc24ea --- /dev/null +++ b/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 + } +} 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 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..779f95bcefa --- /dev/null +++ b/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 + } +} 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