diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSteps.qll b/java/ql/src/semmle/code/java/dataflow/FlowSteps.qll index 1468363c33c..802e287bf43 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSteps.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSteps.qll @@ -32,39 +32,39 @@ class AdditionalTaintStep extends Unit { } /** - * A method that preserves taint. + * A method or constructor that preserves taint. * * Extend this class and override at least one of `returnsTaintFrom` or `transfersTaint` * to add additional taint steps through a method that should apply to all taint configurations. */ -abstract class TaintPreservingMethod extends Method { +abstract class TaintPreservingCallable extends Callable { /** - * Holds if this method returns tainted data when `arg` tainted. + * Holds if this callable returns tainted data when `arg` tainted. * `arg` is a parameter index, or is -1 to indicate the qualifier. */ predicate returnsTaintFrom(int arg) { none() } /** - * Holds if this method writes tainted data to `sink` when `src` is tainted. + * Holds if this callable writes tainted data to `sink` when `src` is tainted. * `src` and `sink` are parameter indices, or -1 to indicate the qualifier. */ predicate transfersTaint(int src, int sink) { none() } } -private class StringTaintPreservingMethod extends TaintPreservingMethod { - StringTaintPreservingMethod() { +private class StringTaintPreservingCallable extends TaintPreservingCallable { + StringTaintPreservingCallable() { this.getDeclaringType() instanceof TypeString and this .hasName(["concat", "copyValueOf", "endsWith", "format", "formatted", "getBytes", "indent", "intern", "join", "repeat", "split", "strip", "stripIndent", "stripLeading", "stripTrailing", "substring", "toCharArray", "toLowerCase", "toString", "toUpperCase", - "trim"]) + "trim", "String"]) } override predicate returnsTaintFrom(int arg) { arg = -1 and not this.isStatic() or - this.hasName(["concat", "copyValueOf"]) and arg = 0 + this.hasName(["concat", "copyValueOf", "String"]) and arg = 0 or this.hasName(["format", "formatted", "join"]) and arg = [0 .. getNumberOfParameters()] } diff --git a/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll b/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll index 8c76d621c21..8643b8e3c19 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll @@ -162,9 +162,6 @@ private predicate inputStreamWrapper(Constructor c, int argi) { private predicate constructorStep(Expr tracked, ConstructorCall sink) { exists(int argi | sink.getArgument(argi) = tracked | exists(string s | sink.getConstructedType().getQualifiedName() = s | - // String constructor does nothing to data - s = "java.lang.String" and argi = 0 - or // some readers preserve the content of streams s = "java.io.InputStreamReader" and argi = 0 or @@ -254,6 +251,8 @@ private predicate constructorStep(Expr tracked, ConstructorCall sink) { argi = 0 and tracked.getType() instanceof TypeString ) + or + sink.getConstructor().(TaintPreservingCallable).returnsTaintFrom(argToParam(sink, argi)) ) } @@ -261,11 +260,11 @@ private predicate constructorStep(Expr tracked, ConstructorCall sink) { * Converts an argument index to a formal parameter index. * This is relevant for varadic methods. */ -private int argToParam(MethodAccess ma, int arg) { - exists(ma.getArgument(arg)) and - exists(Method m | m = ma.getMethod() | - if m.isVarargs() and arg >= m.getNumberOfParameters() - then result = m.getNumberOfParameters() - 1 +private int argToParam(Call call, int arg) { + exists(call.getArgument(arg)) and + exists(Callable c | c = call.getCallee() | + if c.isVarargs() and arg >= c.getNumberOfParameters() + then result = c.getNumberOfParameters() - 1 else result = arg ) } @@ -296,7 +295,7 @@ private predicate taintPreservingQualifierToArgument(Method m, int arg) { m.hasName("read") and arg = 0 or - m.(TaintPreservingMethod).transfersTaint(-1, arg) + m.(TaintPreservingCallable).transfersTaint(-1, arg) } /** Access to a method that passes taint from the qualifier. */ @@ -378,10 +377,10 @@ private predicate taintPreservingQualifierToMethod(Method m) { ) ) or - m.(TaintPreservingMethod).returnsTaintFrom(-1) + m.(TaintPreservingCallable).returnsTaintFrom(-1) } -private class StringReplaceMethod extends TaintPreservingMethod { +private class StringReplaceMethod extends TaintPreservingCallable { StringReplaceMethod() { getDeclaringType() instanceof TypeString and ( @@ -523,7 +522,7 @@ private predicate taintPreservingArgumentToMethod(Method method, int arg) { method.hasName("append") and arg = 0 or - method.(TaintPreservingMethod).returnsTaintFrom(arg) + method.(TaintPreservingCallable).returnsTaintFrom(arg) } /** @@ -571,7 +570,7 @@ private predicate taintPreservingArgToArg(Method method, int input, int output) input = 0 and output = 2 or - method.(TaintPreservingMethod).transfersTaint(input, output) + method.(TaintPreservingCallable).transfersTaint(input, output) } /** @@ -607,10 +606,14 @@ private predicate taintPreservingArgumentToQualifier(Method method, int arg) { method.overrides*(append) and append.hasName("append") and arg = 0 and - append.getDeclaringType().hasQualifiedName("java.io", "StringWriter") + ( + append.getDeclaringType().hasQualifiedName("java.lang", "StringBuilder") or + append.getDeclaringType().hasQualifiedName("java.lang", "StringBuffer") or + append.getDeclaringType().hasQualifiedName("java.io", "StringWriter") + ) ) or - method.(TaintPreservingMethod).transfersTaint(arg, -1) + method.(TaintPreservingCallable).transfersTaint(arg, -1) } /** A comparison or equality test with a constant. */ @@ -734,15 +737,27 @@ private class TypeFormatter extends Class { TypeFormatter() { this.hasQualifiedName("java.util", "Formatter") } } -private class FormatterMethod extends TaintPreservingMethod { - FormatterMethod() { - getDeclaringType() instanceof TypeFormatter and - hasName(["format", "out", "toString"]) +private class FormatterCallable extends TaintPreservingCallable { + FormatterCallable() { + this.getDeclaringType() instanceof TypeFormatter and + ( + this.hasName(["format", "out", "toString"]) + or + this + .(Constructor) + .getParameterType(0) + .(RefType) + .getASourceSupertype*() + .hasQualifiedName("java.lang", "Appendable") + ) } - override predicate returnsTaintFrom(int arg) { arg = [-1 .. getNumberOfParameters()] } + override predicate returnsTaintFrom(int arg) { + if this instanceof Constructor then arg = 0 else arg = [-1 .. getNumberOfParameters()] + } override predicate transfersTaint(int src, int sink) { + this.hasName("format") and sink = -1 and src = [0 .. getNumberOfParameters()] } diff --git a/java/ql/src/semmle/code/java/frameworks/Guice.qll b/java/ql/src/semmle/code/java/frameworks/Guice.qll index e79fb494b4d..f7154df4bd2 100644 --- a/java/ql/src/semmle/code/java/frameworks/Guice.qll +++ b/java/ql/src/semmle/code/java/frameworks/Guice.qll @@ -35,7 +35,7 @@ class GuiceProvider extends Interface { } } -private class OverridingGetMethod extends TaintPreservingMethod { +private class OverridingGetMethod extends TaintPreservingCallable { OverridingGetMethod() { this = any(GuiceProvider gp).getAnOverridingGetMethod() } override predicate returnsTaintFrom(int arg) { arg = -1 } diff --git a/java/ql/src/semmle/code/java/frameworks/Protobuf.qll b/java/ql/src/semmle/code/java/frameworks/Protobuf.qll index bb7d0e815c1..7382294f6f9 100644 --- a/java/ql/src/semmle/code/java/frameworks/Protobuf.qll +++ b/java/ql/src/semmle/code/java/frameworks/Protobuf.qll @@ -56,13 +56,13 @@ class ProtobufMessageLite extends Interface { } } -private class TaintPreservingGetterMethod extends TaintPreservingMethod { +private class TaintPreservingGetterMethod extends TaintPreservingCallable { TaintPreservingGetterMethod() { this = any(ProtobufMessageLite p).getAGetterMethod() } override predicate returnsTaintFrom(int arg) { arg = -1 } } -private class TaintPreservingParseFromMethod extends TaintPreservingMethod { +private class TaintPreservingParseFromMethod extends TaintPreservingCallable { TaintPreservingParseFromMethod() { exists(ProtobufParser p | this = p.getAParseFromMethod()) or diff --git a/java/ql/src/semmle/code/java/frameworks/android/Intent.qll b/java/ql/src/semmle/code/java/frameworks/android/Intent.qll index 6bd184a0bb3..c4894a5976c 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/Intent.qll +++ b/java/ql/src/semmle/code/java/frameworks/android/Intent.qll @@ -34,7 +34,7 @@ class ContextStartActivityMethod extends Method { } } -class IntentGetExtraMethod extends Method, TaintPreservingMethod { +class IntentGetExtraMethod extends Method, TaintPreservingCallable { IntentGetExtraMethod() { (getName().regexpMatch("get\\w+Extra") or hasName("getExtras")) and getDeclaringType() instanceof TypeIntent diff --git a/java/ql/src/semmle/code/java/frameworks/android/SQLite.qll b/java/ql/src/semmle/code/java/frameworks/android/SQLite.qll index cd2232a8683..7a8defc84d1 100644 --- a/java/ql/src/semmle/code/java/frameworks/android/SQLite.qll +++ b/java/ql/src/semmle/code/java/frameworks/android/SQLite.qll @@ -228,7 +228,7 @@ private class ContentProviderUpdateMethod extends SQLiteRunner { override int sqlIndex() { result = 2 } } -private class QueryBuilderBuildMethod extends TaintPreservingMethod { +private class QueryBuilderBuildMethod extends TaintPreservingCallable { int argument; QueryBuilderBuildMethod() { @@ -255,7 +255,7 @@ private class QueryBuilderBuildMethod extends TaintPreservingMethod { override predicate returnsTaintFrom(int arg) { argument = arg } } -private class QueryBuilderAppendMethod extends TaintPreservingMethod { +private class QueryBuilderAppendMethod extends TaintPreservingCallable { QueryBuilderAppendMethod() { this.getDeclaringType().getASourceSupertype*() instanceof TypeSQLiteQueryBuilder and // setProjectionMap(Map columnMap) @@ -273,7 +273,7 @@ private class QueryBuilderAppendMethod extends TaintPreservingMethod { } } -private class UnsafeAppendUtilMethod extends TaintPreservingMethod { +private class UnsafeAppendUtilMethod extends TaintPreservingCallable { UnsafeAppendUtilMethod() { this.getDeclaringType() instanceof TypeDatabaseUtils and // String[] appendSelectionArgs(String[] originalValues, String[] newValues) @@ -284,7 +284,7 @@ private class UnsafeAppendUtilMethod extends TaintPreservingMethod { override predicate returnsTaintFrom(int arg) { arg = [0 .. getNumberOfParameters()] } } -private class TaintPreservingQueryMethod extends TaintPreservingMethod { +private class TaintPreservingQueryMethod extends TaintPreservingCallable { TaintPreservingQueryMethod() { ( this.getDeclaringType() instanceof AndroidContentProvider or diff --git a/java/ql/src/semmle/code/java/frameworks/jackson/JacksonSerializability.qll b/java/ql/src/semmle/code/java/frameworks/jackson/JacksonSerializability.qll index ba51ff4c6d7..3356e31d965 100644 --- a/java/ql/src/semmle/code/java/frameworks/jackson/JacksonSerializability.qll +++ b/java/ql/src/semmle/code/java/frameworks/jackson/JacksonSerializability.qll @@ -28,7 +28,7 @@ abstract class JacksonSerializableType extends Type { } * A method used for serializing objects using Jackson. The final parameter is the object to be * serialized. */ -library class JacksonWriteValueMethod extends TaintPreservingMethod { +library class JacksonWriteValueMethod extends Method, TaintPreservingCallable { JacksonWriteValueMethod() { ( getDeclaringType().hasQualifiedName("com.fasterxml.jackson.databind", "ObjectWriter") or diff --git a/java/ql/test/library-tests/dataflow/taint-format/A.java b/java/ql/test/library-tests/dataflow/taint-format/A.java index cb4a3395ae4..57825e24e43 100644 --- a/java/ql/test/library-tests/dataflow/taint-format/A.java +++ b/java/ql/test/library-tests/dataflow/taint-format/A.java @@ -1,7 +1,7 @@ import java.util.Formatter; import java.lang.StringBuilder; -import java.lang.System; -import java.io.Console; + + class A { public static String taint() { return "tainted"; } @@ -38,9 +38,10 @@ class A { public static void test4() { String bad = taint(); - Console c = System.console(); + StringBuilder sb = new StringBuilder(); - c.format(bad); - c.readLine("Enter something: %s", bad); + sb.append(bad); + + new Formatter(sb).format("ok").toString(); } } \ No newline at end of file diff --git a/java/ql/test/library-tests/dataflow/taint-format/test.expected b/java/ql/test/library-tests/dataflow/taint-format/test.expected index 22f8a10ad1a..ddc0f36d753 100644 --- a/java/ql/test/library-tests/dataflow/taint-format/test.expected +++ b/java/ql/test/library-tests/dataflow/taint-format/test.expected @@ -27,6 +27,10 @@ | A.java:30:22:30:28 | taint(...) | A.java:36:9:36:10 | sb | | A.java:30:22:30:28 | taint(...) | A.java:36:9:36:21 | toString(...) | | A.java:40:22:40:28 | taint(...) | A.java:40:22:40:28 | taint(...) | -| A.java:40:22:40:28 | taint(...) | A.java:43:18:43:20 | bad | -| A.java:40:22:40:28 | taint(...) | A.java:44:9:44:46 | new ..[] { .. } | -| A.java:40:22:40:28 | taint(...) | A.java:44:43:44:45 | bad | +| A.java:40:22:40:28 | taint(...) | A.java:43:9:43:10 | sb [post update] | +| A.java:40:22:40:28 | taint(...) | A.java:43:9:43:22 | append(...) | +| A.java:40:22:40:28 | taint(...) | A.java:43:19:43:21 | bad | +| A.java:40:22:40:28 | taint(...) | A.java:45:9:45:25 | new Formatter(...) | +| A.java:40:22:40:28 | taint(...) | A.java:45:9:45:38 | format(...) | +| A.java:40:22:40:28 | taint(...) | A.java:45:9:45:49 | toString(...) | +| A.java:40:22:40:28 | taint(...) | A.java:45:23:45:24 | sb |