Skip to content

Commit 41f1315

Browse files
atorralbaatorralba
authored
Merge pull request github#13772 from atorralba/atorralba/java/inputstream-wrapper-read-step
Java: Add taint steps for InputStream wrappers
2 parents e534afe + 6c0d47f commit 41f1315

File tree

12 files changed

+249
-7
lines changed

12 files changed

+249
-7
lines changed

‎java/ql/lib/change-notes/2023-07-19-inputstream-wrapper-steps.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added more dataflow steps for `java.io.InputStream`s that wrap other `java.io.InputStream`s.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,11 @@ class TypeObjectInputStream extends RefType {
177177
TypeObjectInputStream() { this.hasQualifiedName("java.io", "ObjectInputStream") }
178178
}
179179

180+
/** The class `java.io.InputStream`. */
181+
class TypeInputStream extends RefType {
182+
TypeInputStream() { this.hasQualifiedName("java.io", "InputStream") }
183+
}
184+
180185
/** The class `java.nio.file.Paths`. */
181186
class TypePaths extends Class {
182187
TypePaths() { this.hasQualifiedName("java.nio.file", "Paths") }

‎java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ private module Frameworks {
2020
private import semmle.code.java.frameworks.Guice
2121
private import semmle.code.java.frameworks.IoJsonWebToken
2222
private import semmle.code.java.frameworks.jackson.JacksonSerializability
23+
private import semmle.code.java.frameworks.InputStream
2324
private import semmle.code.java.frameworks.Properties
2425
private import semmle.code.java.frameworks.Protobuf
2526
private import semmle.code.java.frameworks.ratpack.RatpackExec
2627
private import semmle.code.java.frameworks.stapler.Stapler
27-
private import semmle.code.java.JDK
2828
}
2929

3030
/**

‎java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -757,7 +757,7 @@ private predicate baseBound(Expr e, int b, boolean upper) {
757757
or
758758
exists(Method read |
759759
e.(MethodAccess).getMethod().overrides*(read) and
760-
read.getDeclaringType().hasQualifiedName("java.io", "InputStream") and
760+
read.getDeclaringType() instanceof TypeInputStream and
761761
read.hasName("read") and
762762
read.getNumberOfParameters() = 0
763763
|

‎java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ private class BulkData extends RefType {
239239
this.(Array).getElementType().(PrimitiveType).hasName(["byte", "char"])
240240
or
241241
exists(RefType t | this.getASourceSupertype*() = t |
242-
t.hasQualifiedName("java.io", "InputStream") or
242+
t instanceof TypeInputStream or
243243
t.hasQualifiedName("java.nio", "ByteBuffer") or
244244
t.hasQualifiedName("java.lang", "Readable") or
245245
t.hasQualifiedName("java.io", "DataInput") or
@@ -259,7 +259,7 @@ private class BulkData extends RefType {
259259
private predicate inputStreamWrapper(Constructor c, int argi) {
260260
not c.fromSource() and
261261
c.getParameterType(argi) instanceof BulkData and
262-
c.getDeclaringType().getASourceSupertype+().hasQualifiedName("java.io", "InputStream")
262+
c.getDeclaringType().getASourceSupertype+() instanceof TypeInputStream
263263
}
264264

265265
/** An object construction that preserves the data flow status of any of its arguments. */

‎java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ private module Dispatch {
102102
or
103103
t instanceof Interface and not t.fromSource()
104104
or
105-
t.hasQualifiedName("java.io", "InputStream")
105+
t instanceof TypeInputStream
106106
or
107107
t.hasQualifiedName("java.io", "Serializable")
108108
or

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

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/** Provides definitions related to `java.io.InputStream`. */
2+
3+
import java
4+
private import semmle.code.java.dataflow.DataFlow
5+
private import semmle.code.java.dataflow.FlowSteps
6+
private import semmle.code.java.dataflow.SSA
7+
private import semmle.code.java.dataflow.TaintTracking
8+
9+
/**
10+
* A jump taint step from an update of the `bytes[]` parameter in an override of the `InputStream.read` method
11+
* to a class instance expression of the type extending `InputStream`.
12+
*
13+
* This models how a subtype of `InputStream` could be tainted by the definition of its methods, which will
14+
* normally only happen in nested classes.
15+
*/
16+
private class InputStreamWrapperCapturedJumpStep extends AdditionalTaintStep {
17+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
18+
exists(InputStreamRead m, NestedClass wrapper |
19+
m.getDeclaringType() = wrapper and
20+
wrapper.getASourceSupertype+() instanceof TypeInputStream
21+
|
22+
n1.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess() and
23+
n2.asExpr()
24+
.(ClassInstanceExpr)
25+
.getConstructedType()
26+
.getASourceSupertype*()
27+
.getSourceDeclaration() = wrapper
28+
)
29+
}
30+
}
31+
32+
/**
33+
* A local taint step from the definition of a captured variable, the capturer of which
34+
* updates the `bytes[]` parameter in an override of the `InputStream.read` method,
35+
* to a class instance expression of the type extending `InputStream`.
36+
*
37+
* This models how a subtype of `InputStream` could be tainted by capturing tainted variables in
38+
* the definition of its methods.
39+
*/
40+
private class InputStreamWrapperCapturedLocalStep extends AdditionalTaintStep {
41+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
42+
exists(InputStreamRead m, NestedClass wrapper, SsaVariable captured, SsaImplicitInit capturer |
43+
wrapper.getASourceSupertype+() instanceof TypeInputStream and
44+
m.getDeclaringType() = wrapper and
45+
capturer.captures(captured) and
46+
TaintTracking::localTaint(DataFlow::exprNode(capturer.getAFirstUse()),
47+
any(DataFlow::PostUpdateNode pun |
48+
pun.getPreUpdateNode().asExpr() = m.getParameter(0).getAnAccess()
49+
)) and
50+
n2.asExpr()
51+
.(ClassInstanceExpr)
52+
.getConstructedType()
53+
.getASourceSupertype*()
54+
.getSourceDeclaration() = wrapper
55+
|
56+
n1.asExpr() = captured.(SsaExplicitUpdate).getDefiningExpr().(VariableAssign).getSource()
57+
or
58+
captured.(SsaImplicitInit).isParameterDefinition(n1.asParameter())
59+
)
60+
}
61+
}
62+
63+
/**
64+
* A taint step from an `InputStream` argument of the constructor of an `InputStream` subtype
65+
* to the call of the constructor, only if the argument is assigned to a class field.
66+
*
67+
* This models how it's assumed that an `InputStream` wrapper is tainted by the wrapped stream,
68+
* and is a workaround to low `fieldFlowBranchLimit`s in dataflow configurations.
69+
*/
70+
private class InputStreamWrapperConstructorStep extends AdditionalTaintStep {
71+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
72+
exists(ClassInstanceExpr cc, Argument a, AssignExpr ae, int pos |
73+
cc.getConstructedType().getASourceSupertype+() instanceof TypeInputStream and
74+
cc.getArgument(pragma[only_bind_into](pos)) = a and
75+
cc.getCallee().getParameter(pragma[only_bind_into](pos)).getAnAccess() = ae.getRhs() and
76+
ae.getDest().(FieldWrite).getField().getType().(RefType).getASourceSupertype*() instanceof
77+
TypeInputStream
78+
|
79+
n1.asExpr() = a and
80+
n2.asExpr() = cc
81+
)
82+
}
83+
}
84+
85+
private class InputStreamRead extends Method {
86+
InputStreamRead() {
87+
this.hasName("read") and
88+
this.getDeclaringType().getASourceSupertype*() instanceof TypeInputStream
89+
}
90+
}

‎java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJBRestrictions.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ class SystemSetInputStreamMethod extends Method {
317317
SystemSetInputStreamMethod() {
318318
this.hasName("setIn") and
319319
this.getNumberOfParameters() = 1 and
320-
this.getParameter(0).getType().(RefType).hasQualifiedName("java.io", "InputStream") and
320+
this.getParameter(0).getType() instanceof TypeInputStream and
321321
this.getDeclaringType()
322322
.getAnAncestor()
323323
.getSourceDeclaration()

‎java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ class SpringRequestMappingParameter extends Parameter {
237237

238238
private predicate isExplicitlyTaintedInput() {
239239
// InputStream or Reader parameters allow access to the body of a request
240-
this.getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "InputStream") or
240+
this.getType().(RefType).getAnAncestor() instanceof TypeInputStream or
241241
this.getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "Reader") or
242242
// The SpringServletInputAnnotations allow access to the URI, request parameters, cookie values and the body of the request
243243
this.getAnAnnotation() instanceof SpringServletInputAnnotation or

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

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
import java.io.InputStream;
2+
import java.io.IOException;
3+
4+
public class A {
5+
6+
private static InputStream source() {
7+
return null;
8+
}
9+
10+
private static void sink(Object s) {}
11+
12+
static class MyStream extends InputStream {
13+
private InputStream wrapped;
14+
15+
MyStream(InputStream wrapped) {
16+
this.wrapped = wrapped;
17+
}
18+
19+
@Override
20+
public int read() throws IOException {
21+
return 0;
22+
}
23+
24+
@Override
25+
public int read(byte[] b) throws IOException {
26+
return wrapped.read(b);
27+
}
28+
}
29+
30+
public static void testSeveralWrappers() {
31+
InputStream src = source();
32+
InputStream wrapper1 = new MyStream(src);
33+
sink(wrapper1); // $ hasTaintFlow
34+
InputStream wrapper2 = new MyStream(wrapper1);
35+
sink(wrapper2); // $ hasTaintFlow
36+
InputStream wrapper3 = new MyStream(wrapper2);
37+
sink(wrapper3); // $ hasTaintFlow
38+
39+
InputStream wrapper4 = new InputStream() {
40+
@Override
41+
public int read() throws IOException {
42+
return 0;
43+
}
44+
45+
@Override
46+
public int read(byte[] b) throws IOException {
47+
return wrapper3.read(b);
48+
49+
}
50+
};
51+
sink(wrapper4); // $ hasTaintFlow
52+
}
53+
54+
public static void testAnonymous() throws Exception {
55+
InputStream wrapper = new InputStream() {
56+
@Override
57+
public int read() throws IOException {
58+
return 0;
59+
}
60+
61+
@Override
62+
public int read(byte[] b) throws IOException {
63+
InputStream in = source();
64+
return in.read(b);
65+
}
66+
};
67+
sink(wrapper); // $ hasTaintFlow
68+
}
69+
70+
public static void testAnonymousVarCapture() throws Exception {
71+
InputStream in = source();
72+
InputStream wrapper = new InputStream() {
73+
@Override
74+
public int read() throws IOException {
75+
return 0;
76+
}
77+
78+
@Override
79+
public int read(byte[] b) throws IOException {
80+
return in.read(b);
81+
82+
}
83+
};
84+
sink(wrapper); // $ hasTaintFlow
85+
}
86+
87+
public static InputStream wrapStream(InputStream in) {
88+
return new InputStream() {
89+
@Override
90+
public int read() throws IOException {
91+
return 0;
92+
}
93+
94+
@Override
95+
public int read(byte[] b) throws IOException {
96+
return in.read(b);
97+
}
98+
};
99+
}
100+
101+
public static void testWrapCall() {
102+
sink(wrapStream(null)); // $ SPURIOUS: hasTaintFlow
103+
sink(wrapStream(source())); // $ hasTaintFlow
104+
}
105+
106+
public static void testLocal() {
107+
108+
class LocalInputStream extends InputStream {
109+
@Override
110+
public int read() throws IOException {
111+
return 0;
112+
}
113+
114+
@Override
115+
public int read(byte[] b) throws IOException {
116+
InputStream in = source();
117+
return in.read(b);
118+
}
119+
}
120+
sink(new LocalInputStream()); // $ hasTaintFlow
121+
}
122+
123+
public static void testLocalVarCapture() {
124+
InputStream in = source();
125+
126+
class LocalInputStream extends InputStream {
127+
@Override
128+
public int read() throws IOException {
129+
return 0;
130+
}
131+
132+
@Override
133+
public int read(byte[] b) throws IOException {
134+
return in.read(b);
135+
}
136+
}
137+
sink(new LocalInputStream()); // $ hasTaintFlow
138+
}
139+
}

0 commit comments

Comments
 (0)