Skip to content

Commit e660ac5

Browse files
aschackmullaschackmull
authored
Merge pull request #4358 from joefarebrother/format-taint
Java: Add taint steps through string formatting methods
2 parents 43b2c90 + ca4781e commit e660ac5

File tree

5 files changed

+162
-0
lines changed

5 files changed

+162
-0
lines changed

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) {
124124
stringBuilderStep(src, sink)
125125
or
126126
serializationStep(src, sink)
127+
or
128+
formatStep(src, sink)
127129
}
128130

129131
/**
@@ -387,6 +389,9 @@ private predicate taintPreservingQualifierToMethod(Method m) {
387389
stringlist.getTypeArgument(0) instanceof TypeString
388390
)
389391
)
392+
or
393+
m.getDeclaringType() instanceof TypeFormatter and
394+
m.hasName(["format", "out"])
390395
}
391396

392397
private class StringReplaceMethod extends Method {
@@ -447,6 +452,9 @@ private predicate argToMethodStep(Expr tracked, MethodAccess sink) {
447452
private predicate taintPreservingArgumentToMethod(Method method) {
448453
method.getDeclaringType() instanceof TypeString and
449454
(method.hasName("format") or method.hasName("formatted") or method.hasName("join"))
455+
or
456+
method.getDeclaringType() instanceof TypeFormatter and
457+
method.hasName("format")
450458
}
451459

452460
/**
@@ -625,6 +633,20 @@ private predicate argToQualifierStep(Expr tracked, Expr sink) {
625633
tracked = ma.getArgument(i) and
626634
sink = ma.getQualifier()
627635
)
636+
or
637+
exists(MethodAccess ma |
638+
taintPreservingArgumentToQualifier(ma.getMethod()) and
639+
tracked = ma.getAnArgument() and
640+
sink = ma.getQualifier()
641+
)
642+
}
643+
644+
/**
645+
* Holds if `method` is a method that transfers taint from any of its arguments to its qualifier.
646+
*/
647+
private predicate taintPreservingArgumentToQualifier(Method method) {
648+
method.getDeclaringType() instanceof TypeFormatter and
649+
method.hasName("format")
628650
}
629651

630652
/**
@@ -722,6 +744,56 @@ class ObjectOutputStreamVar extends LocalVariableDecl {
722744
}
723745
}
724746

747+
/** Flow through string formatting. */
748+
private predicate formatStep(Expr tracked, Expr sink) {
749+
exists(FormatterVar v, VariableAssign def |
750+
def = v.getADef() and
751+
exists(MethodAccess ma, RValue use |
752+
ma.getAnArgument() = tracked and
753+
ma = v.getAFormatMethodAccess() and
754+
use = ma.getQualifier() and
755+
defUsePair(def, use)
756+
) and
757+
exists(RValue output, ClassInstanceExpr cie |
758+
cie = def.getSource() and
759+
output = cie.getArgument(0) and
760+
adjacentUseUse(output, sink) and
761+
exists(RefType t | output.getType().(RefType).getASourceSupertype*() = t |
762+
t.hasQualifiedName("java.io", "OutputStream") or
763+
t.hasQualifiedName("java.lang", "Appendable")
764+
)
765+
)
766+
)
767+
}
768+
769+
/**
770+
* A local variable that is assigned a `Formatter`.
771+
* Writing tainted data to such a formatter causes the underlying
772+
* `OutputStream` or `Appendable` to be tainted.
773+
*/
774+
private class FormatterVar extends LocalVariableDecl {
775+
FormatterVar() {
776+
exists(ClassInstanceExpr cie | cie = this.getAnAssignedValue() |
777+
cie.getType() instanceof TypeFormatter
778+
)
779+
}
780+
781+
VariableAssign getADef() {
782+
result.getSource().(ClassInstanceExpr).getType() instanceof TypeFormatter and
783+
result.getDestVar() = this
784+
}
785+
786+
MethodAccess getAFormatMethodAccess() {
787+
result.getQualifier() = getAnAccess() and
788+
result.getMethod().hasName("format")
789+
}
790+
}
791+
792+
/** The class `java.util.Formatter`. */
793+
private class TypeFormatter extends Class {
794+
TypeFormatter() { this.hasQualifiedName("java.util", "Formatter") }
795+
}
796+
725797
private import StringBuilderVarModule
726798

727799
module StringBuilderVarModule {

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import java.util.Formatter;
2+
import java.lang.StringBuilder;
3+
import java.lang.System;
4+
import java.io.Console;
5+
6+
class A {
7+
public static String taint() { return "tainted"; }
8+
9+
public static void test1() {
10+
String bad = taint();
11+
String good = "hi";
12+
13+
bad.formatted(good);
14+
good.formatted("a", bad, "b", good);
15+
String.format("%s%s", bad, good);
16+
String.format("%s", good);
17+
}
18+
19+
public static void test2() {
20+
String bad = taint();
21+
Formatter f = new Formatter();
22+
23+
f.toString();
24+
f.format("%s", bad);
25+
f.toString();
26+
}
27+
28+
public static void test3() {
29+
String bad = taint();
30+
StringBuilder sb = new StringBuilder();
31+
Formatter f = new Formatter(sb);
32+
33+
sb.toString(); // false positive
34+
f.format("%s", bad);
35+
sb.toString();
36+
}
37+
38+
public static void test4() {
39+
String bad = taint();
40+
Console c = System.console();
41+
42+
c.format(bad);
43+
c.readLine("Enter something: %s", bad);
44+
}
45+
}

‎java/ql/test/library-tests/dataflow/taint-format/options‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
//semmle-extractor-options: --javac-args --enable-preview -source 14 -target 14

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
| A.java:10:22:10:28 | taint(...) | A.java:10:22:10:28 | taint(...) |
2+
| A.java:10:22:10:28 | taint(...) | A.java:13:9:13:11 | bad |
3+
| A.java:10:22:10:28 | taint(...) | A.java:13:9:13:27 | formatted(...) |
4+
| A.java:10:22:10:28 | taint(...) | A.java:14:9:14:43 | formatted(...) |
5+
| A.java:10:22:10:28 | taint(...) | A.java:14:9:14:43 | new ..[] { .. } |
6+
| A.java:10:22:10:28 | taint(...) | A.java:14:29:14:31 | bad |
7+
| A.java:10:22:10:28 | taint(...) | A.java:15:9:15:40 | format(...) |
8+
| A.java:10:22:10:28 | taint(...) | A.java:15:9:15:40 | new ..[] { .. } |
9+
| A.java:10:22:10:28 | taint(...) | A.java:15:31:15:33 | bad |
10+
| A.java:20:22:20:28 | taint(...) | A.java:20:22:20:28 | taint(...) |
11+
| A.java:20:22:20:28 | taint(...) | A.java:24:9:24:9 | f [post update] |
12+
| A.java:20:22:20:28 | taint(...) | A.java:24:9:24:27 | format(...) |
13+
| A.java:20:22:20:28 | taint(...) | A.java:24:9:24:27 | new ..[] { .. } |
14+
| A.java:20:22:20:28 | taint(...) | A.java:24:24:24:26 | bad |
15+
| A.java:20:22:20:28 | taint(...) | A.java:25:9:25:9 | f |
16+
| A.java:29:22:29:28 | taint(...) | A.java:29:22:29:28 | taint(...) |
17+
| A.java:29:22:29:28 | taint(...) | A.java:33:9:33:10 | sb |
18+
| A.java:29:22:29:28 | taint(...) | A.java:33:9:33:21 | toString(...) |
19+
| A.java:29:22:29:28 | taint(...) | A.java:34:9:34:9 | f [post update] |
20+
| A.java:29:22:29:28 | taint(...) | A.java:34:9:34:27 | format(...) |
21+
| A.java:29:22:29:28 | taint(...) | A.java:34:9:34:27 | new ..[] { .. } |
22+
| A.java:29:22:29:28 | taint(...) | A.java:34:24:34:26 | bad |
23+
| A.java:29:22:29:28 | taint(...) | A.java:35:9:35:10 | sb |
24+
| A.java:29:22:29:28 | taint(...) | A.java:35:9:35:21 | toString(...) |
25+
| A.java:39:22:39:28 | taint(...) | A.java:39:22:39:28 | taint(...) |
26+
| A.java:39:22:39:28 | taint(...) | A.java:42:18:42:20 | bad |
27+
| A.java:39:22:39:28 | taint(...) | A.java:43:9:43:46 | new ..[] { .. } |
28+
| A.java:39:22:39:28 | taint(...) | A.java:43:43:43:45 | bad |

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import java
2+
import semmle.code.java.dataflow.TaintTracking
3+
4+
class Conf extends TaintTracking::Configuration {
5+
Conf() { this = "qltest:dataflow:format" }
6+
7+
override predicate isSource(DataFlow::Node n) {
8+
n.asExpr().(MethodAccess).getMethod().hasName("taint")
9+
}
10+
11+
override predicate isSink(DataFlow::Node n) { any() }
12+
}
13+
14+
from DataFlow::Node src, DataFlow::Node sink, Conf conf
15+
where conf.hasFlow(src, sink)
16+
select src, sink

0 commit comments

Comments
 (0)