Skip to content

Commit eafde05

Browse files
joefarebrotherjoefarebrother
committed
Java: Expand flow step refactoring to Callables
Also add some missing flow steps for StringBuilder
1 parent 7e2c49f commit eafde05

File tree

9 files changed

+65
-45
lines changed

9 files changed

+65
-45
lines changed

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,39 +32,39 @@ class AdditionalTaintStep extends Unit {
3232
}
3333

3434
/**
35-
* A method that preserves taint.
35+
* A method or constructor that preserves taint.
3636
*
3737
* Extend this class and override at least one of `returnsTaintFrom` or `transfersTaint`
3838
* to add additional taint steps through a method that should apply to all taint configurations.
3939
*/
40-
abstract class TaintPreservingMethod extends Method {
40+
abstract class TaintPreservingCallable extends Callable {
4141
/**
42-
* Holds if this method returns tainted data when `arg` tainted.
42+
* Holds if this callable returns tainted data when `arg` tainted.
4343
* `arg` is a parameter index, or is -1 to indicate the qualifier.
4444
*/
4545
predicate returnsTaintFrom(int arg) { none() }
4646

4747
/**
48-
* Holds if this method writes tainted data to `sink` when `src` is tainted.
48+
* Holds if this callable writes tainted data to `sink` when `src` is tainted.
4949
* `src` and `sink` are parameter indices, or -1 to indicate the qualifier.
5050
*/
5151
predicate transfersTaint(int src, int sink) { none() }
5252
}
5353

54-
private class StringTaintPreservingMethod extends TaintPreservingMethod {
55-
StringTaintPreservingMethod() {
54+
private class StringTaintPreservingCallable extends TaintPreservingCallable {
55+
StringTaintPreservingCallable() {
5656
this.getDeclaringType() instanceof TypeString and
5757
this
5858
.hasName(["concat", "copyValueOf", "endsWith", "format", "formatted", "getBytes", "indent",
5959
"intern", "join", "repeat", "split", "strip", "stripIndent", "stripLeading",
6060
"stripTrailing", "substring", "toCharArray", "toLowerCase", "toString", "toUpperCase",
61-
"trim"])
61+
"trim", "String"])
6262
}
6363

6464
override predicate returnsTaintFrom(int arg) {
6565
arg = -1 and not this.isStatic()
6666
or
67-
this.hasName(["concat", "copyValueOf"]) and arg = 0
67+
this.hasName(["concat", "copyValueOf", "String"]) and arg = 0
6868
or
6969
this.hasName(["format", "formatted", "join"]) and arg = [0 .. getNumberOfParameters()]
7070
}

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

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,6 @@ private predicate inputStreamWrapper(Constructor c, int argi) {
162162
private predicate constructorStep(Expr tracked, ConstructorCall sink) {
163163
exists(int argi | sink.getArgument(argi) = tracked |
164164
exists(string s | sink.getConstructedType().getQualifiedName() = s |
165-
// String constructor does nothing to data
166-
s = "java.lang.String" and argi = 0
167-
or
168165
// some readers preserve the content of streams
169166
s = "java.io.InputStreamReader" and argi = 0
170167
or
@@ -254,18 +251,20 @@ private predicate constructorStep(Expr tracked, ConstructorCall sink) {
254251
argi = 0 and
255252
tracked.getType() instanceof TypeString
256253
)
254+
or
255+
sink.getConstructor().(TaintPreservingCallable).returnsTaintFrom(argToParam(sink, argi))
257256
)
258257
}
259258

260259
/**
261260
* Converts an argument index to a formal parameter index.
262261
* This is relevant for varadic methods.
263262
*/
264-
private int argToParam(MethodAccess ma, int arg) {
265-
exists(ma.getArgument(arg)) and
266-
exists(Method m | m = ma.getMethod() |
267-
if m.isVarargs() and arg >= m.getNumberOfParameters()
268-
then result = m.getNumberOfParameters() - 1
263+
private int argToParam(Call call, int arg) {
264+
exists(call.getArgument(arg)) and
265+
exists(Callable c | c = call.getCallee() |
266+
if c.isVarargs() and arg >= c.getNumberOfParameters()
267+
then result = c.getNumberOfParameters() - 1
269268
else result = arg
270269
)
271270
}
@@ -296,7 +295,7 @@ private predicate taintPreservingQualifierToArgument(Method m, int arg) {
296295
m.hasName("read") and
297296
arg = 0
298297
or
299-
m.(TaintPreservingMethod).transfersTaint(-1, arg)
298+
m.(TaintPreservingCallable).transfersTaint(-1, arg)
300299
}
301300

302301
/** Access to a method that passes taint from the qualifier. */
@@ -378,10 +377,10 @@ private predicate taintPreservingQualifierToMethod(Method m) {
378377
)
379378
)
380379
or
381-
m.(TaintPreservingMethod).returnsTaintFrom(-1)
380+
m.(TaintPreservingCallable).returnsTaintFrom(-1)
382381
}
383382

384-
private class StringReplaceMethod extends TaintPreservingMethod {
383+
private class StringReplaceMethod extends TaintPreservingCallable {
385384
StringReplaceMethod() {
386385
getDeclaringType() instanceof TypeString and
387386
(
@@ -523,7 +522,7 @@ private predicate taintPreservingArgumentToMethod(Method method, int arg) {
523522
method.hasName("append") and
524523
arg = 0
525524
or
526-
method.(TaintPreservingMethod).returnsTaintFrom(arg)
525+
method.(TaintPreservingCallable).returnsTaintFrom(arg)
527526
}
528527

529528
/**
@@ -571,7 +570,7 @@ private predicate taintPreservingArgToArg(Method method, int input, int output)
571570
input = 0 and
572571
output = 2
573572
or
574-
method.(TaintPreservingMethod).transfersTaint(input, output)
573+
method.(TaintPreservingCallable).transfersTaint(input, output)
575574
}
576575

577576
/**
@@ -607,10 +606,14 @@ private predicate taintPreservingArgumentToQualifier(Method method, int arg) {
607606
method.overrides*(append) and
608607
append.hasName("append") and
609608
arg = 0 and
610-
append.getDeclaringType().hasQualifiedName("java.io", "StringWriter")
609+
(
610+
append.getDeclaringType().hasQualifiedName("java.lang", "StringBuilder") or
611+
append.getDeclaringType().hasQualifiedName("java.lang", "StringBuffer") or
612+
append.getDeclaringType().hasQualifiedName("java.io", "StringWriter")
613+
)
611614
)
612615
or
613-
method.(TaintPreservingMethod).transfersTaint(arg, -1)
616+
method.(TaintPreservingCallable).transfersTaint(arg, -1)
614617
}
615618

616619
/** A comparison or equality test with a constant. */
@@ -734,15 +737,27 @@ private class TypeFormatter extends Class {
734737
TypeFormatter() { this.hasQualifiedName("java.util", "Formatter") }
735738
}
736739

737-
private class FormatterMethod extends TaintPreservingMethod {
738-
FormatterMethod() {
739-
getDeclaringType() instanceof TypeFormatter and
740-
hasName(["format", "out", "toString"])
740+
private class FormatterCallable extends TaintPreservingCallable {
741+
FormatterCallable() {
742+
this.getDeclaringType() instanceof TypeFormatter and
743+
(
744+
this.hasName(["format", "out", "toString"])
745+
or
746+
this
747+
.(Constructor)
748+
.getParameterType(0)
749+
.(RefType)
750+
.getASourceSupertype*()
751+
.hasQualifiedName("java.lang", "Appendable")
752+
)
741753
}
742754

743-
override predicate returnsTaintFrom(int arg) { arg = [-1 .. getNumberOfParameters()] }
755+
override predicate returnsTaintFrom(int arg) {
756+
if this instanceof Constructor then arg = 0 else arg = [-1 .. getNumberOfParameters()]
757+
}
744758

745759
override predicate transfersTaint(int src, int sink) {
760+
this.hasName("format") and
746761
sink = -1 and
747762
src = [0 .. getNumberOfParameters()]
748763
}

‎java/ql/src/semmle/code/java/frameworks/Guice.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class GuiceProvider extends Interface {
3535
}
3636
}
3737

38-
private class OverridingGetMethod extends TaintPreservingMethod {
38+
private class OverridingGetMethod extends TaintPreservingCallable {
3939
OverridingGetMethod() { this = any(GuiceProvider gp).getAnOverridingGetMethod() }
4040

4141
override predicate returnsTaintFrom(int arg) { arg = -1 }

‎java/ql/src/semmle/code/java/frameworks/Protobuf.qll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,13 @@ class ProtobufMessageLite extends Interface {
5656
}
5757
}
5858

59-
private class TaintPreservingGetterMethod extends TaintPreservingMethod {
59+
private class TaintPreservingGetterMethod extends TaintPreservingCallable {
6060
TaintPreservingGetterMethod() { this = any(ProtobufMessageLite p).getAGetterMethod() }
6161

6262
override predicate returnsTaintFrom(int arg) { arg = -1 }
6363
}
6464

65-
private class TaintPreservingParseFromMethod extends TaintPreservingMethod {
65+
private class TaintPreservingParseFromMethod extends TaintPreservingCallable {
6666
TaintPreservingParseFromMethod() {
6767
exists(ProtobufParser p | this = p.getAParseFromMethod())
6868
or

‎java/ql/src/semmle/code/java/frameworks/android/Intent.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ class ContextStartActivityMethod extends Method {
3434
}
3535
}
3636

37-
class IntentGetExtraMethod extends Method, TaintPreservingMethod {
37+
class IntentGetExtraMethod extends Method, TaintPreservingCallable {
3838
IntentGetExtraMethod() {
3939
(getName().regexpMatch("get\\w+Extra") or hasName("getExtras")) and
4040
getDeclaringType() instanceof TypeIntent

‎java/ql/src/semmle/code/java/frameworks/android/SQLite.qll‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ private class ContentProviderUpdateMethod extends SQLiteRunner {
228228
override int sqlIndex() { result = 2 }
229229
}
230230

231-
private class QueryBuilderBuildMethod extends TaintPreservingMethod {
231+
private class QueryBuilderBuildMethod extends TaintPreservingCallable {
232232
int argument;
233233

234234
QueryBuilderBuildMethod() {
@@ -255,7 +255,7 @@ private class QueryBuilderBuildMethod extends TaintPreservingMethod {
255255
override predicate returnsTaintFrom(int arg) { argument = arg }
256256
}
257257

258-
private class QueryBuilderAppendMethod extends TaintPreservingMethod {
258+
private class QueryBuilderAppendMethod extends TaintPreservingCallable {
259259
QueryBuilderAppendMethod() {
260260
this.getDeclaringType().getASourceSupertype*() instanceof TypeSQLiteQueryBuilder and
261261
// setProjectionMap(Map<String, String> columnMap)
@@ -273,7 +273,7 @@ private class QueryBuilderAppendMethod extends TaintPreservingMethod {
273273
}
274274
}
275275

276-
private class UnsafeAppendUtilMethod extends TaintPreservingMethod {
276+
private class UnsafeAppendUtilMethod extends TaintPreservingCallable {
277277
UnsafeAppendUtilMethod() {
278278
this.getDeclaringType() instanceof TypeDatabaseUtils and
279279
// String[] appendSelectionArgs(String[] originalValues, String[] newValues)
@@ -284,7 +284,7 @@ private class UnsafeAppendUtilMethod extends TaintPreservingMethod {
284284
override predicate returnsTaintFrom(int arg) { arg = [0 .. getNumberOfParameters()] }
285285
}
286286

287-
private class TaintPreservingQueryMethod extends TaintPreservingMethod {
287+
private class TaintPreservingQueryMethod extends TaintPreservingCallable {
288288
TaintPreservingQueryMethod() {
289289
(
290290
this.getDeclaringType() instanceof AndroidContentProvider or

‎java/ql/src/semmle/code/java/frameworks/jackson/JacksonSerializability.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ abstract class JacksonSerializableType extends Type { }
2828
* A method used for serializing objects using Jackson. The final parameter is the object to be
2929
* serialized.
3030
*/
31-
library class JacksonWriteValueMethod extends TaintPreservingMethod {
31+
library class JacksonWriteValueMethod extends Method, TaintPreservingCallable {
3232
JacksonWriteValueMethod() {
3333
(
3434
getDeclaringType().hasQualifiedName("com.fasterxml.jackson.databind", "ObjectWriter") or

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import java.util.Formatter;
22
import java.lang.StringBuilder;
3-
import java.lang.System;
4-
import java.io.Console;
3+
4+
55

66
class A {
77
public static String taint() { return "tainted"; }
@@ -38,9 +38,10 @@ public static void test3() {
3838

3939
public static void test4() {
4040
String bad = taint();
41-
Console c = System.console();
41+
StringBuilder sb = new StringBuilder();
42+
43+
sb.append(bad);
4244

43-
c.format(bad);
44-
c.readLine("Enter something: %s", bad);
45+
new Formatter(sb).format("ok").toString();
4546
}
4647
}

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@
2727
| A.java:30:22:30:28 | taint(...) | A.java:36:9:36:10 | sb |
2828
| A.java:30:22:30:28 | taint(...) | A.java:36:9:36:21 | toString(...) |
2929
| A.java:40:22:40:28 | taint(...) | A.java:40:22:40:28 | taint(...) |
30-
| A.java:40:22:40:28 | taint(...) | A.java:43:18:43:20 | bad |
31-
| A.java:40:22:40:28 | taint(...) | A.java:44:9:44:46 | new ..[] { .. } |
32-
| A.java:40:22:40:28 | taint(...) | A.java:44:43:44:45 | bad |
30+
| A.java:40:22:40:28 | taint(...) | A.java:43:9:43:10 | sb [post update] |
31+
| A.java:40:22:40:28 | taint(...) | A.java:43:9:43:22 | append(...) |
32+
| A.java:40:22:40:28 | taint(...) | A.java:43:19:43:21 | bad |
33+
| A.java:40:22:40:28 | taint(...) | A.java:45:9:45:25 | new Formatter(...) |
34+
| A.java:40:22:40:28 | taint(...) | A.java:45:9:45:38 | format(...) |
35+
| A.java:40:22:40:28 | taint(...) | A.java:45:9:45:49 | toString(...) |
36+
| A.java:40:22:40:28 | taint(...) | A.java:45:23:45:24 | sb |

0 commit comments

Comments
 (0)