From 00e0e5a61a9570b19ede598668ca6eb074ddedbf Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 19 Jul 2023 11:36:09 +0200 Subject: [PATCH] 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