Skip to content

Commit 9c3fed7

Browse files
yo-hyo-h
authored
Merge pull request #2734 from aschackmull/java/taint-postupdate
Java: Improve taint step modeling to use postupdate nodes.
2 parents dac4f0f + 75f7671 commit 9c3fed7

File tree

6 files changed

+108
-24
lines changed

6 files changed

+108
-24
lines changed

‎java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,27 @@ private predicate argToQualifierStep(Expr tracked, Expr sink) {
161161
/**
162162
* Holds if the step from `n1` to `n2` is either extracting a value from a
163163
* container, inserting a value into a container, or transforming one container
164-
* to another.
164+
* to another. This is restricted to cases where `n2` is the returned value of
165+
* a call.
165166
*/
166-
predicate containerStep(Expr n1, Expr n2) {
167-
qualifierToMethodStep(n1, n2) or
167+
predicate containerReturnValueStep(Expr n1, Expr n2) { qualifierToMethodStep(n1, n2) }
168+
169+
/**
170+
* Holds if the step from `n1` to `n2` is either extracting a value from a
171+
* container, inserting a value into a container, or transforming one container
172+
* to another. This is restricted to cases where the value of `n2` is being modified.
173+
*/
174+
predicate containerUpdateStep(Expr n1, Expr n2) {
168175
qualifierToArgumentStep(n1, n2) or
169176
argToQualifierStep(n1, n2)
170177
}
178+
179+
/**
180+
* Holds if the step from `n1` to `n2` is either extracting a value from a
181+
* container, inserting a value into a container, or transforming one container
182+
* to another.
183+
*/
184+
predicate containerStep(Expr n1, Expr n2) {
185+
containerReturnValueStep(n1, n2) or
186+
containerUpdateStep(n1, n2)
187+
}

‎java/ql/src/semmle/code/java/dataflow/internal/DataFlowUtil.qll‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ private newtype TNode =
2626
e instanceof Argument and not e.getType() instanceof ImmutableType
2727
or
2828
exists(FieldAccess fa | fa.getField() instanceof InstanceField and e = fa.getQualifier())
29+
or
30+
exists(ArrayAccess aa | e = aa.getArray())
2931
} or
3032
TImplicitExprPostUpdate(InstanceAccessExt ia) {
3133
implicitInstanceArgument(_, ia)

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

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ predicate localTaintStep(DataFlow::Node src, DataFlow::Node sink) {
4141
predicate localAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) {
4242
localAdditionalTaintExprStep(src.asExpr(), sink.asExpr())
4343
or
44+
localAdditionalTaintUpdateStep(src.asExpr(),
45+
sink.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr())
46+
or
4447
exists(Argument arg |
4548
src.asExpr() = arg and
4649
arg.isVararg() and
@@ -105,37 +108,47 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) {
105108
or
106109
sink.(LogicExpr).getAnOperand() = src
107110
or
108-
exists(Assignment assign | assign.getSource() = src |
109-
sink = assign.getDest().(ArrayAccess).getArray()
110-
)
111-
or
112111
exists(EnhancedForStmt for, SsaExplicitUpdate v |
113112
for.getExpr() = src and
114113
v.getDefiningExpr() = for.getVariable() and
115114
v.getAFirstUse() = sink
116115
)
117116
or
118-
containerStep(src, sink)
117+
containerReturnValueStep(src, sink)
119118
or
120119
constructorStep(src, sink)
121120
or
122121
qualifierToMethodStep(src, sink)
123122
or
124-
qualifierToArgumentStep(src, sink)
125-
or
126123
argToMethodStep(src, sink)
127124
or
128-
argToArgStep(src, sink)
129-
or
130-
argToQualifierStep(src, sink)
131-
or
132125
comparisonStep(src, sink)
133126
or
134127
stringBuilderStep(src, sink)
135128
or
136129
serializationStep(src, sink)
137130
}
138131

132+
/**
133+
* Holds if taint can flow in one local step from `src` to `sink` excluding
134+
* local data flow steps. That is, `src` and `sink` are likely to represent
135+
* different objects.
136+
* This is restricted to cases where the step updates the value of `sink`.
137+
*/
138+
private predicate localAdditionalTaintUpdateStep(Expr src, Expr sink) {
139+
exists(Assignment assign | assign.getSource() = src |
140+
sink = assign.getDest().(ArrayAccess).getArray()
141+
)
142+
or
143+
containerUpdateStep(src, sink)
144+
or
145+
qualifierToArgumentStep(src, sink)
146+
or
147+
argToArgStep(src, sink)
148+
or
149+
argToQualifierStep(src, sink)
150+
}
151+
139152
private class BulkData extends RefType {
140153
BulkData() {
141154
this.(Array).getElementType().(PrimitiveType).getName().regexpMatch("byte|char")
@@ -242,7 +255,7 @@ private predicate constructorStep(Expr tracked, ConstructorCall sink) {
242255
}
243256

244257
/** Access to a method that passes taint from qualifier to argument. */
245-
private predicate qualifierToArgumentStep(Expr tracked, RValue sink) {
258+
private predicate qualifierToArgumentStep(Expr tracked, Expr sink) {
246259
exists(MethodAccess ma, int arg |
247260
taintPreservingQualifierToArgument(ma.getMethod(), arg) and
248261
tracked = ma.getQualifier() and
@@ -458,7 +471,7 @@ private predicate taintPreservingArgumentToMethod(Method method, int arg) {
458471
* Holds if `tracked` and `sink` are arguments to a method that transfers taint
459472
* between arguments.
460473
*/
461-
private predicate argToArgStep(Expr tracked, RValue sink) {
474+
private predicate argToArgStep(Expr tracked, Expr sink) {
462475
exists(MethodAccess ma, Method method, int input, int output |
463476
taintPreservingArgToArg(method, input, output) and
464477
ma.getMethod() = method and

‎java/ql/test/library-tests/dataflow/taint-ioutils/dataFlow.expected‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,36 +19,36 @@
1919
| Test.java:22:20:22:52 | toInputStream(...) |
2020
| Test.java:22:42:22:42 | s |
2121
| Test.java:26:16:26:18 | inp |
22-
| Test.java:26:21:26:26 | writer |
22+
| Test.java:26:21:26:26 | writer [post update] |
2323
| Test.java:27:3:27:8 | writer |
2424
| Test.java:27:3:27:19 | toString(...) |
2525
| Test.java:31:21:31:27 | bufread |
26-
| Test.java:31:30:31:35 | writer |
26+
| Test.java:31:30:31:35 | writer [post update] |
2727
| Test.java:32:3:32:8 | writer |
2828
| Test.java:32:3:32:19 | toString(...) |
2929
| Test.java:37:16:37:18 | inp |
30-
| Test.java:37:21:37:23 | tgt |
30+
| Test.java:37:21:37:23 | tgt [post update] |
3131
| Test.java:38:3:38:12 | ...=... |
3232
| Test.java:38:7:38:9 | tgt |
3333
| Test.java:38:7:38:12 | ...[...] |
3434
| Test.java:42:21:42:23 | inp |
35-
| Test.java:42:26:42:28 | tgt |
35+
| Test.java:42:26:42:28 | tgt [post update] |
3636
| Test.java:43:3:43:12 | ...=... |
3737
| Test.java:43:7:43:9 | tgt |
3838
| Test.java:43:7:43:12 | ...[...] |
3939
| Test.java:47:17:47:21 | chars |
40-
| Test.java:47:24:47:29 | writer |
40+
| Test.java:47:24:47:29 | writer [post update] |
4141
| Test.java:48:3:48:8 | writer |
4242
| Test.java:48:3:48:19 | toString(...) |
4343
| Test.java:52:24:52:28 | chars |
44-
| Test.java:52:31:52:36 | writer |
44+
| Test.java:52:31:52:36 | writer [post update] |
4545
| Test.java:53:3:53:8 | writer |
4646
| Test.java:53:3:53:19 | toString(...) |
4747
| Test.java:57:22:57:26 | lines |
48-
| Test.java:57:35:57:40 | writer |
48+
| Test.java:57:35:57:40 | writer [post update] |
4949
| Test.java:58:3:58:8 | writer |
5050
| Test.java:58:3:58:19 | toString(...) |
5151
| Test.java:62:47:62:47 | s |
52-
| Test.java:62:50:62:55 | writer |
52+
| Test.java:62:50:62:55 | writer [post update] |
5353
| Test.java:63:3:63:8 | writer |
5454
| Test.java:63:3:63:19 | toString(...) |

‎java/ql/test/library-tests/dataflow/taint/A.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,52 @@ void test2() {
2424
bInput.read(b2);
2525
sink(b2);
2626
}
27+
28+
void streamWrite(ByteArrayOutputStream baos, byte[] data) {
29+
baos.write(data);
30+
}
31+
32+
void test3(ByteArrayOutputStream baos) {
33+
streamWrite(baos, taint());
34+
sink(baos.toByteArray());
35+
}
36+
37+
static class BaosHolder {
38+
ByteArrayOutputStream baos = new ByteArrayOutputStream();
39+
}
40+
41+
void streamWriteHolder(BaosHolder bh, byte[] data) {
42+
bh.baos.write(data);
43+
}
44+
45+
void test4(BaosHolder bh) {
46+
streamWriteHolder(bh, taint());
47+
sink(bh.baos.toByteArray());
48+
}
49+
50+
static class DataHolder {
51+
byte[] data = new byte[10];
52+
}
53+
54+
void test5_a(DataHolder dh) {
55+
ByteArrayInputStream bais = new ByteArrayInputStream(taint());
56+
bais.read(dh.data);
57+
test5_b(dh);
58+
}
59+
60+
void test5_b(DataHolder dh) {
61+
sink(dh.data);
62+
}
63+
64+
void arrayWrite(byte[] from, byte[] to) {
65+
for (int i = 0; i < 10; i++) {
66+
to[i] = from[i];
67+
}
68+
}
69+
70+
void test6() {
71+
byte[] b = new byte[10];
72+
arrayWrite(taint(), b);
73+
sink(b);
74+
}
2775
}

‎java/ql/test/library-tests/dataflow/taint/test.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
| A.java:10:19:10:25 | taint(...) | A.java:15:10:15:11 | b2 |
22
| A.java:20:19:20:25 | taint(...) | A.java:25:10:25:11 | b2 |
3+
| A.java:33:23:33:29 | taint(...) | A.java:34:10:34:27 | toByteArray(...) |
4+
| A.java:46:27:46:33 | taint(...) | A.java:47:10:47:30 | toByteArray(...) |
5+
| A.java:55:58:55:64 | taint(...) | A.java:61:10:61:16 | dh.data |
6+
| A.java:72:16:72:22 | taint(...) | A.java:73:10:73:10 | b |
37
| B.java:15:21:15:27 | taint(...) | B.java:18:10:18:16 | aaaargs |
48
| B.java:15:21:15:27 | taint(...) | B.java:21:10:21:10 | s |
59
| B.java:15:21:15:27 | taint(...) | B.java:24:10:24:15 | concat |

0 commit comments

Comments
 (0)