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 eaf7c9d26dc..8d551ecdaff 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll @@ -13,6 +13,7 @@ private import semmle.code.java.frameworks.spring.SpringHttp private import semmle.code.java.Maps private import semmle.code.java.dataflow.internal.ContainerFlow private import semmle.code.java.frameworks.jackson.JacksonSerializability +private import semmle.code.java.StringFormat /** * Holds if taint can flow from `src` to `sink` in zero or more @@ -124,6 +125,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) { stringBuilderStep(src, sink) or serializationStep(src, sink) + or + formatStep(src, sink) } /** @@ -387,6 +390,11 @@ private predicate taintPreservingQualifierToMethod(Method m) { stringlist.getTypeArgument(0) instanceof TypeString ) ) + or + m instanceof StringFormatMethod + or + m.getDeclaringType() instanceof TypeFormatter and + m.hasName("out") } private class StringReplaceMethod extends Method { @@ -446,7 +454,9 @@ private predicate argToMethodStep(Expr tracked, MethodAccess sink) { */ private predicate taintPreservingArgumentToMethod(Method method) { method.getDeclaringType() instanceof TypeString and - (method.hasName("format") or method.hasName("formatted") or method.hasName("join")) + method.hasName("join") + or + method instanceof StringFormatMethod } /** @@ -625,6 +635,21 @@ private predicate argToQualifierStep(Expr tracked, Expr sink) { tracked = ma.getArgument(i) and sink = ma.getQualifier() ) + or + exists(Method m, MethodAccess ma | + taintPreservingArgumentToQualifier(m) and + ma.getMethod() = m and + tracked = ma.getAnArgument() and + sink = ma.getQualifier() + ) +} + +/** + * Holds if `method` is a method that transfers taint from any of its arguments to its qualifier. + */ +private predicate taintPreservingArgumentToQualifier(Method method) { + method instanceof StringFormatMethod and + not method.getDeclaringType() instanceof TypeString } /** @@ -722,6 +747,56 @@ class ObjectOutputStreamVar extends LocalVariableDecl { } } +/** Flow through string formatting. */ +private predicate formatStep(Expr tracked, Expr sink) { + exists(FormatterVar v, VariableAssign def | + def = v.getADef() and + exists(MethodAccess ma, RValue use | + ma.getAnArgument() = tracked and + ma = v.getAFormatMethodAccess() and + use = ma.getQualifier() and + defUsePair(def, use) + ) and + exists(RValue output, ClassInstanceExpr cie | + cie = def.getSource() and + output = cie.getArgument(0) and + adjacentUseUse(output, sink) and + exists(RefType t | output.getType().(RefType).getASourceSupertype*() = t | + t.hasQualifiedName("java.io", "OutputStream") or + t.hasQualifiedName("java.lang", "Appendable") + ) + ) + ) +} + +/** + * A local variable that is assigned a `Formatter`. + * Writing tainted data to such a formatter causes the underlying + * `OutputStream` or `Appenable` to be tainted. + */ +private class FormatterVar extends LocalVariableDecl { + FormatterVar() { + exists(ClassInstanceExpr cie | cie = this.getAnAssignedValue() | + cie.getType() instanceof TypeFormatter + ) + } + + VariableAssign getADef() { + result.getSource().(ClassInstanceExpr).getType() instanceof TypeFormatter and + result.getDestVar() = this + } + + MethodAccess getAFormatMethodAccess() { + result.getQualifier() = getAnAccess() and + result.getMethod().hasName("format") + } +} + +/** The class `java.util.Formatter`. */ +private class TypeFormatter extends Class { + TypeFormatter() { this.hasQualifiedName("java.util", "Formatter") } +} + private import StringBuilderVarModule module StringBuilderVarModule { diff --git a/java/ql/test/library-tests/dataflow/taint-format/A.java b/java/ql/test/library-tests/dataflow/taint-format/A.java new file mode 100644 index 00000000000..c9bb27c8e99 --- /dev/null +++ b/java/ql/test/library-tests/dataflow/taint-format/A.java @@ -0,0 +1,34 @@ +import java.util.Formatter; +import java.lang.StringBuilder; + +class A { + public static String taint() { return "tainted"; } + + public static void test1() { + String bad = taint(); + String good = "hi"; + + bad.formatted(good); + good.formatted("a", bad, "b", good); + String.format("%s%s", bad, good); + } + + public static void test2() { + String bad = taint(); + Formatter f = new Formatter(); + + f.toString(); + f.format("%s", bad); + f.toString(); + } + + public static void test3() { + String bad = taint(); + StringBuilder sb = new StringBuilder(); + Formatter f = new Formatter(sb); + + sb.toString(); // false positive + f.format("%s", bad); + sb.toString(); + } +} \ No newline at end of file diff --git a/java/ql/test/library-tests/dataflow/taint-format/options b/java/ql/test/library-tests/dataflow/taint-format/options new file mode 100644 index 00000000000..814f667417c --- /dev/null +++ b/java/ql/test/library-tests/dataflow/taint-format/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args --enable-preview -source 14 -target 14 \ 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 new file mode 100644 index 00000000000..9e0e64f695c --- /dev/null +++ b/java/ql/test/library-tests/dataflow/taint-format/test.expected @@ -0,0 +1,24 @@ +| A.java:8:22:8:28 | taint(...) | A.java:8:22:8:28 | taint(...) | +| A.java:8:22:8:28 | taint(...) | A.java:11:9:11:11 | bad | +| A.java:8:22:8:28 | taint(...) | A.java:11:9:11:27 | formatted(...) | +| A.java:8:22:8:28 | taint(...) | A.java:12:9:12:43 | formatted(...) | +| A.java:8:22:8:28 | taint(...) | A.java:12:9:12:43 | new ..[] { .. } | +| A.java:8:22:8:28 | taint(...) | A.java:12:29:12:31 | bad | +| A.java:8:22:8:28 | taint(...) | A.java:13:9:13:40 | format(...) | +| A.java:8:22:8:28 | taint(...) | A.java:13:9:13:40 | new ..[] { .. } | +| A.java:8:22:8:28 | taint(...) | A.java:13:31:13:33 | bad | +| A.java:17:22:17:28 | taint(...) | A.java:17:22:17:28 | taint(...) | +| A.java:17:22:17:28 | taint(...) | A.java:21:9:21:9 | f [post update] | +| A.java:17:22:17:28 | taint(...) | A.java:21:9:21:27 | format(...) | +| A.java:17:22:17:28 | taint(...) | A.java:21:9:21:27 | new ..[] { .. } | +| A.java:17:22:17:28 | taint(...) | A.java:21:24:21:26 | bad | +| A.java:17:22:17:28 | taint(...) | A.java:22:9:22:9 | f | +| A.java:26:22:26:28 | taint(...) | A.java:26:22:26:28 | taint(...) | +| A.java:26:22:26:28 | taint(...) | A.java:30:9:30:10 | sb | +| A.java:26:22:26:28 | taint(...) | A.java:30:9:30:21 | toString(...) | +| A.java:26:22:26:28 | taint(...) | A.java:31:9:31:9 | f [post update] | +| A.java:26:22:26:28 | taint(...) | A.java:31:9:31:27 | format(...) | +| A.java:26:22:26:28 | taint(...) | A.java:31:9:31:27 | new ..[] { .. } | +| A.java:26:22:26:28 | taint(...) | A.java:31:24:31:26 | bad | +| A.java:26:22:26:28 | taint(...) | A.java:32:9:32:10 | sb | +| A.java:26:22:26:28 | taint(...) | A.java:32:9:32:21 | toString(...) | diff --git a/java/ql/test/library-tests/dataflow/taint-format/test.ql b/java/ql/test/library-tests/dataflow/taint-format/test.ql new file mode 100644 index 00000000000..0a877c0c985 --- /dev/null +++ b/java/ql/test/library-tests/dataflow/taint-format/test.ql @@ -0,0 +1,16 @@ +import java +import semmle.code.java.dataflow.TaintTracking + +class Conf extends TaintTracking::Configuration { + Conf() { this = "qltest:dataflow:format" } + + override predicate isSource(DataFlow::Node n) { + n.asExpr().(MethodAccess).getMethod().hasName("taint") + } + + override predicate isSink(DataFlow::Node n) { any() } +} + +from DataFlow::Node src, DataFlow::Node sink, Conf conf +where conf.hasFlow(src, sink) +select src, sink