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..8ce4ca66ec6 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll @@ -124,6 +124,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) { stringBuilderStep(src, sink) or serializationStep(src, sink) + or + formatStep(src, sink) } /** @@ -387,6 +389,9 @@ private predicate taintPreservingQualifierToMethod(Method m) { stringlist.getTypeArgument(0) instanceof TypeString ) ) + or + m.getDeclaringType() instanceof TypeFormatter and + m.hasName(["format", "out"]) } private class StringReplaceMethod extends Method { @@ -447,6 +452,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")) + or + method.getDeclaringType() instanceof TypeFormatter and + method.hasName("format") } /** @@ -625,6 +633,20 @@ private predicate argToQualifierStep(Expr tracked, Expr sink) { tracked = ma.getArgument(i) and sink = ma.getQualifier() ) + or + exists(MethodAccess ma | + taintPreservingArgumentToQualifier(ma.getMethod()) 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.getDeclaringType() instanceof TypeFormatter and + method.hasName("format") } /** @@ -722,6 +744,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 `Appendable` 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..3612c23f950 --- /dev/null +++ b/java/ql/test/library-tests/dataflow/taint-format/A.java @@ -0,0 +1,45 @@ +import java.util.Formatter; +import java.lang.StringBuilder; +import java.lang.System; +import java.io.Console; + +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); + String.format("%s", 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(); + } + + public static void test4() { + String bad = taint(); + Console c = System.console(); + + c.format(bad); + c.readLine("Enter something: %s", bad); + } +} \ 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..c52918fadc5 --- /dev/null +++ b/java/ql/test/library-tests/dataflow/taint-format/test.expected @@ -0,0 +1,28 @@ +| A.java:10:22:10:28 | taint(...) | A.java:10:22:10:28 | taint(...) | +| A.java:10:22:10:28 | taint(...) | A.java:13:9:13:11 | bad | +| A.java:10:22:10:28 | taint(...) | A.java:13:9:13:27 | formatted(...) | +| A.java:10:22:10:28 | taint(...) | A.java:14:9:14:43 | formatted(...) | +| A.java:10:22:10:28 | taint(...) | A.java:14:9:14:43 | new ..[] { .. } | +| A.java:10:22:10:28 | taint(...) | A.java:14:29:14:31 | bad | +| A.java:10:22:10:28 | taint(...) | A.java:15:9:15:40 | format(...) | +| A.java:10:22:10:28 | taint(...) | A.java:15:9:15:40 | new ..[] { .. } | +| A.java:10:22:10:28 | taint(...) | A.java:15:31:15:33 | bad | +| A.java:20:22:20:28 | taint(...) | A.java:20:22:20:28 | taint(...) | +| A.java:20:22:20:28 | taint(...) | A.java:24:9:24:9 | f [post update] | +| A.java:20:22:20:28 | taint(...) | A.java:24:9:24:27 | format(...) | +| A.java:20:22:20:28 | taint(...) | A.java:24:9:24:27 | new ..[] { .. } | +| A.java:20:22:20:28 | taint(...) | A.java:24:24:24:26 | bad | +| A.java:20:22:20:28 | taint(...) | A.java:25:9:25:9 | f | +| A.java:29:22:29:28 | taint(...) | A.java:29:22:29:28 | taint(...) | +| A.java:29:22:29:28 | taint(...) | A.java:33:9:33:10 | sb | +| A.java:29:22:29:28 | taint(...) | A.java:33:9:33:21 | toString(...) | +| A.java:29:22:29:28 | taint(...) | A.java:34:9:34:9 | f [post update] | +| A.java:29:22:29:28 | taint(...) | A.java:34:9:34:27 | format(...) | +| A.java:29:22:29:28 | taint(...) | A.java:34:9:34:27 | new ..[] { .. } | +| A.java:29:22:29:28 | taint(...) | A.java:34:24:34:26 | bad | +| A.java:29:22:29:28 | taint(...) | A.java:35:9:35:10 | sb | +| A.java:29:22:29:28 | taint(...) | A.java:35:9:35:21 | toString(...) | +| A.java:39:22:39:28 | taint(...) | A.java:39:22:39:28 | taint(...) | +| A.java:39:22:39:28 | taint(...) | A.java:42:18:42:20 | bad | +| A.java:39:22:39:28 | taint(...) | A.java:43:9:43:46 | new ..[] { .. } | +| A.java:39:22:39:28 | taint(...) | A.java:43:43:43:45 | bad | 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