Skip to content

Commit bea38fc

Browse files
joefarebrotherjoefarebrother
committed
Java: Add taint modelling for string format methods
1 parent 274147c commit bea38fc

File tree

5 files changed

+151
-1
lines changed

5 files changed

+151
-1
lines changed

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

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ private import semmle.code.java.frameworks.spring.SpringHttp
1313
private import semmle.code.java.Maps
1414
private import semmle.code.java.dataflow.internal.ContainerFlow
1515
private import semmle.code.java.frameworks.jackson.JacksonSerializability
16+
private import semmle.code.java.StringFormat
1617

1718
/**
1819
* Holds if taint can flow from `src` to `sink` in zero or more
@@ -124,6 +125,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) {
124125
stringBuilderStep(src, sink)
125126
or
126127
serializationStep(src, sink)
128+
or
129+
formatStep(src, sink)
127130
}
128131

129132
/**
@@ -387,6 +390,11 @@ private predicate taintPreservingQualifierToMethod(Method m) {
387390
stringlist.getTypeArgument(0) instanceof TypeString
388391
)
389392
)
393+
or
394+
m instanceof StringFormatMethod
395+
or
396+
m.getDeclaringType() instanceof TypeFormatter and
397+
m.hasName("out")
390398
}
391399

392400
private class StringReplaceMethod extends Method {
@@ -446,7 +454,9 @@ private predicate argToMethodStep(Expr tracked, MethodAccess sink) {
446454
*/
447455
private predicate taintPreservingArgumentToMethod(Method method) {
448456
method.getDeclaringType() instanceof TypeString and
449-
(method.hasName("format") or method.hasName("formatted") or method.hasName("join"))
457+
method.hasName("join")
458+
or
459+
method instanceof StringFormatMethod
450460
}
451461

452462
/**
@@ -625,6 +635,21 @@ private predicate argToQualifierStep(Expr tracked, Expr sink) {
625635
tracked = ma.getArgument(i) and
626636
sink = ma.getQualifier()
627637
)
638+
or
639+
exists(Method m, MethodAccess ma |
640+
taintPreservingArgumentToQualifier(m) and
641+
ma.getMethod() = m and
642+
tracked = ma.getAnArgument() and
643+
sink = ma.getQualifier()
644+
)
645+
}
646+
647+
/**
648+
* Holds if `method` is a method that transfers taint from any of its arguments to its qualifier.
649+
*/
650+
private predicate taintPreservingArgumentToQualifier(Method method) {
651+
method instanceof StringFormatMethod and
652+
not method.getDeclaringType() instanceof TypeString
628653
}
629654

630655
/**
@@ -722,6 +747,56 @@ class ObjectOutputStreamVar extends LocalVariableDecl {
722747
}
723748
}
724749

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

727802
module StringBuilderVarModule {

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

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

‎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: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
| A.java:8:22:8:28 | taint(...) | A.java:8:22:8:28 | taint(...) |
2+
| A.java:8:22:8:28 | taint(...) | A.java:11:9:11:11 | bad |
3+
| A.java:8:22:8:28 | taint(...) | A.java:11:9:11:27 | formatted(...) |
4+
| A.java:8:22:8:28 | taint(...) | A.java:12:9:12:43 | formatted(...) |
5+
| A.java:8:22:8:28 | taint(...) | A.java:12:9:12:43 | new ..[] { .. } |
6+
| A.java:8:22:8:28 | taint(...) | A.java:12:29:12:31 | bad |
7+
| A.java:8:22:8:28 | taint(...) | A.java:13:9:13:40 | format(...) |
8+
| A.java:8:22:8:28 | taint(...) | A.java:13:9:13:40 | new ..[] { .. } |
9+
| A.java:8:22:8:28 | taint(...) | A.java:13:31:13:33 | bad |
10+
| A.java:17:22:17:28 | taint(...) | A.java:17:22:17:28 | taint(...) |
11+
| A.java:17:22:17:28 | taint(...) | A.java:21:9:21:9 | f [post update] |
12+
| A.java:17:22:17:28 | taint(...) | A.java:21:9:21:27 | format(...) |
13+
| A.java:17:22:17:28 | taint(...) | A.java:21:9:21:27 | new ..[] { .. } |
14+
| A.java:17:22:17:28 | taint(...) | A.java:21:24:21:26 | bad |
15+
| A.java:17:22:17:28 | taint(...) | A.java:22:9:22:9 | f |
16+
| A.java:26:22:26:28 | taint(...) | A.java:26:22:26:28 | taint(...) |
17+
| A.java:26:22:26:28 | taint(...) | A.java:30:9:30:10 | sb |
18+
| A.java:26:22:26:28 | taint(...) | A.java:30:9:30:21 | toString(...) |
19+
| A.java:26:22:26:28 | taint(...) | A.java:31:9:31:9 | f [post update] |
20+
| A.java:26:22:26:28 | taint(...) | A.java:31:9:31:27 | format(...) |
21+
| A.java:26:22:26:28 | taint(...) | A.java:31:9:31:27 | new ..[] { .. } |
22+
| A.java:26:22:26:28 | taint(...) | A.java:31:24:31:26 | bad |
23+
| A.java:26:22:26:28 | taint(...) | A.java:32:9:32:10 | sb |
24+
| A.java:26:22:26:28 | taint(...) | A.java:32:9:32:21 | toString(...) |

‎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)