From a29f615da037ffb602f0a89cc066896710ccab8d Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 28 Jan 2019 14:30:08 +0100 Subject: [PATCH] Java: Add additional taint steps through collections. --- change-notes/1.20/analysis-java.md | 3 + .../code/java/dataflow/TaintTracking.qll | 64 ++++++++++++++++++- .../dataflow/collections/Test.java | 29 +++++++++ .../dataflow/collections/flow.expected | 4 ++ .../dataflow/collections/flow.ql | 21 ++++++ 5 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 java/ql/test/library-tests/dataflow/collections/Test.java create mode 100644 java/ql/test/library-tests/dataflow/collections/flow.expected create mode 100644 java/ql/test/library-tests/dataflow/collections/flow.ql diff --git a/change-notes/1.20/analysis-java.md b/change-notes/1.20/analysis-java.md index 36ce1c094af..8c59d936ef1 100644 --- a/change-notes/1.20/analysis-java.md +++ b/change-notes/1.20/analysis-java.md @@ -24,5 +24,8 @@ `semmle.code.java.dataflow.DataFlow`, `semmle.code.java.dataflow.TaintTracking`, and `semmle.code.java.dataflow.FlowSources` since 1.16. +* Taint tracking now includes additional default data-flow steps through + collections, maps, and iterators. This affects all security queries, which + can report more results based on such paths. diff --git a/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll b/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll index 78b0f382f8c..cf2c9792921 100644 --- a/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll +++ b/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll @@ -12,6 +12,7 @@ private import DefUse private import semmle.code.java.security.SecurityTests private import semmle.code.java.security.Validation private import semmle.code.java.frameworks.android.Intent +private import semmle.code.java.Maps module TaintTracking { /** @@ -209,6 +210,12 @@ module TaintTracking { sink = assign.getDest().(ArrayAccess).getArray() ) or + exists(EnhancedForStmt for, SsaExplicitUpdate v | + for.getExpr() = src and + v.getDefiningExpr() = for.getVariable() and + v.getAFirstUse() = sink + ) + or constructorStep(src, sink) or qualifierToMethodStep(src, sink) @@ -418,8 +425,49 @@ module TaintTracking { or m instanceof IntentGetExtraMethod or - m instanceof CollectionMethod and - m.hasName("toArray") + m + .getDeclaringType() + .getSourceDeclaration() + .getASourceSupertype*() + .hasQualifiedName("java.util", "Map<>$Entry") and + m.hasName("getValue") + or + m + .getDeclaringType() + .getSourceDeclaration() + .getASourceSupertype*() + .hasQualifiedName("java.lang", "Iterable") and + m.hasName("iterator") + or + m + .getDeclaringType() + .getSourceDeclaration() + .getASourceSupertype*() + .hasQualifiedName("java.util", "Iterator") and + m.hasName("next") + or + m.getDeclaringType().getSourceDeclaration().hasQualifiedName("java.util", "Enumeration") and + m.hasName("nextElement") + or + m.(MapMethod).hasName("entrySet") + or + m.(MapMethod).hasName("get") + or + m.(MapMethod).hasName("remove") + or + m.(MapMethod).hasName("values") + or + m.(CollectionMethod).hasName("toArray") + or + m.(CollectionMethod).hasName("get") + or + m.(CollectionMethod).hasName("remove") and m.getParameterType(0).(PrimitiveType).hasName("int") + or + m.(CollectionMethod).hasName("subList") + or + m.(CollectionMethod).hasName("firstElement") + or + m.(CollectionMethod).hasName("lastElement") or m.getDeclaringType().hasQualifiedName("java.nio", "ByteBuffer") and m.hasName("get") @@ -596,6 +644,18 @@ module TaintTracking { method.getDeclaringType().hasQualifiedName("java.io", "ByteArrayOutputStream") and method.hasName("write") and arg = 0 + or + method.(MapMethod).hasName("put") and arg = 1 + or + method.(MapMethod).hasName("putAll") and arg = 0 + or + method.(CollectionMethod).hasName("add") and arg = method.getNumberOfParameters() - 1 + or + method.(CollectionMethod).hasName("addAll") and arg = method.getNumberOfParameters() - 1 + or + method.(CollectionMethod).hasName("addElement") and arg = 0 + or + method.(CollectionMethod).hasName("set") and arg = 1 } /** A comparison or equality test with a constant. */ diff --git a/java/ql/test/library-tests/dataflow/collections/Test.java b/java/ql/test/library-tests/dataflow/collections/Test.java new file mode 100644 index 00000000000..8557c13b1ec --- /dev/null +++ b/java/ql/test/library-tests/dataflow/collections/Test.java @@ -0,0 +1,29 @@ +import java.util.*; + +public class Test { + static String tainted; + + void sink(Object o) { } + + public void run() { + HashMap m = new HashMap<>(); + String x1 = m.get("key"); + sink(x1); // No flow + + m.put("key", tainted); + String x2 = m.get("key"); + sink(x2); // Flow + + String x3 = m.values().toArray(new String[1])[0]; + sink(x3); // Flow + + for(Map.Entry e : m.entrySet()) { + String x4 = e.getValue(); + sink(x4); // Flow + } + + Iterator it = m.values().iterator(); + String x5 = it.next(); + sink(x5); // Flow + } +} diff --git a/java/ql/test/library-tests/dataflow/collections/flow.expected b/java/ql/test/library-tests/dataflow/collections/flow.expected new file mode 100644 index 00000000000..e2ccd7702b3 --- /dev/null +++ b/java/ql/test/library-tests/dataflow/collections/flow.expected @@ -0,0 +1,4 @@ +| Test.java:13:18:13:24 | tainted | Test.java:15:10:15:11 | x2 | +| Test.java:13:18:13:24 | tainted | Test.java:18:10:18:11 | x3 | +| Test.java:13:18:13:24 | tainted | Test.java:22:12:22:13 | x4 | +| Test.java:13:18:13:24 | tainted | Test.java:27:10:27:11 | x5 | diff --git a/java/ql/test/library-tests/dataflow/collections/flow.ql b/java/ql/test/library-tests/dataflow/collections/flow.ql new file mode 100644 index 00000000000..dab06a3d966 --- /dev/null +++ b/java/ql/test/library-tests/dataflow/collections/flow.ql @@ -0,0 +1,21 @@ +import java +import semmle.code.java.dataflow.TaintTracking + +class Conf extends TaintTracking::Configuration { + Conf() { this = "conf" } + + override predicate isSource(DataFlow::Node src) { + src.asExpr().(VarAccess).getVariable().hasName("tainted") + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + sink.asExpr() = ma.getAnArgument() and + ma.getMethod().hasName("sink") + ) + } +} + +from Conf c, DataFlow::Node src, DataFlow::Node sink +where c.hasFlow(src, sink) +select src, sink