Merge pull request github#3869 from aibaars/util-collections

Java: model java.util.Collections

Author: aibaars

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

@@ -159,6 +159,44 @@ private predicate taintPreservingArgumentToQualifier(Method method, int arg) {
   method.(CollectionMethod).hasName("offer") and arg = 0
 }
 
+/**
+ * Holds if `method` is a library method that returns tainted data if its
+ * `arg`th argument is tainted.
+ */
+private predicate taintPreservingArgumentToMethod(Method method, int arg) {
+  method.getDeclaringType().hasQualifiedName("java.util", "Collections") and
+  (
+    method
+        .hasName(["checkedCollection", "checkedList", "checkedMap", "checkedNavigableMap",
+              "checkedNavigableSet", "checkedSet", "checkedSortedMap", "checkedSortedSet",
+              "enumeration", "list", "max", "min", "singleton", "singletonList",
+              "synchronizedCollection", "synchronizedList", "synchronizedMap",
+              "synchronizedNavigableMap", "synchronizedNavigableSet", "synchronizedSet",
+              "synchronizedSortedMap", "synchronizedSortedSet", "unmodifiableCollection",
+              "unmodifiableList", "unmodifiableMap", "unmodifiableNavigableMap",
+              "unmodifiableNavigableSet", "unmodifiableSet", "unmodifiableSortedMap",
+              "unmodifiableSortedSet"]) and
+    arg = 0
+    or
+    method.hasName(["nCopies", "singletonMap"]) and arg = 1
+  )
+}
+
+/**
+ * Holds if `method` is a library method that writes tainted data to the
+ * `output`th argument if the `input`th argument is tainted.
+ */
+private predicate taintPreservingArgToArg(Method method, int input, int output) {
+  method.getDeclaringType().hasQualifiedName("java.util", "Collections") and
+  (
+    method.hasName(["copy", "fill"]) and
+    input = 1 and
+    output = 0
+    or
+    method.hasName("replaceAll") and input = 2 and output = 0
+  )
+}
+
 private predicate argToQualifierStep(Expr tracked, Expr sink) {
   exists(Method m, int i, MethodAccess ma |
     taintPreservingArgumentToQualifier(m, i) and
@@ -168,13 +206,44 @@ private predicate argToQualifierStep(Expr tracked, Expr sink) {
   )
 }
 
+/** Access to a method that passes taint from an argument. */
+private predicate argToMethodStep(Expr tracked, MethodAccess sink) {
+  exists(Method m, int i |
+    m = sink.getMethod() and
+    taintPreservingArgumentToMethod(m, i) and
+    tracked = sink.getArgument(i)
+  )
+}
+
+/**
+ * Holds if `tracked` and `sink` are arguments to a method that transfers taint
+ * between arguments.
+ */
+private predicate argToArgStep(Expr tracked, Expr sink) {
+  exists(MethodAccess ma, Method method, int input, int output |
+    ma.getMethod() = method and
+    ma.getArgument(input) = tracked and
+    ma.getArgument(output) = sink and
+    (
+      taintPreservingArgToArg(method, input, output)
+      or
+      method.getDeclaringType().hasQualifiedName("java.util", "Collections") and
+      method.hasName("addAll") and
+      input >= 1 and
+      output = 0
+    )
+  )
+}
+
 /**
  * Holds if the step from `n1` to `n2` is either extracting a value from a
  * container, inserting a value into a container, or transforming one container
  * to another. This is restricted to cases where `n2` is the returned value of
  * a call.
  */
-predicate containerReturnValueStep(Expr n1, Expr n2) { qualifierToMethodStep(n1, n2) }
+predicate containerReturnValueStep(Expr n1, Expr n2) {
+  qualifierToMethodStep(n1, n2) or argToMethodStep(n1, n2)
+}
 
 /**
  * Holds if the step from `n1` to `n2` is either extracting a value from a
@@ -183,7 +252,8 @@ predicate containerReturnValueStep(Expr n1, Expr n2) { qualifierToMethodStep(n1,
  */
 predicate containerUpdateStep(Expr n1, Expr n2) {
   qualifierToArgumentStep(n1, n2) or
-  argToQualifierStep(n1, n2)
+  argToQualifierStep(n1, n2) or
+  argToArgStep(n1, n2)
 }
 
 /**

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

@@ -383,9 +383,9 @@ private predicate unsafeEscape(MethodAccess ma) {
 /** Access to a method that passes taint from an argument. */
 private predicate argToMethodStep(Expr tracked, MethodAccess sink) {
   exists(Method m, int i |
-    m = sink.(MethodAccess).getMethod() and
+    m = sink.getMethod() and
     taintPreservingArgumentToMethod(m, i) and
-    tracked = sink.(MethodAccess).getArgument(i)
+    tracked = sink.getArgument(i)
   )
   or
   exists(MethodAccess ma |

java/ql/test/library-tests/dataflow/local-additional-taint/CollectionsTest.java

@@ -0,0 +1,22 @@
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.List;
+
+class CollectionsTest {
+	public static void taintSteps(List<String> list, List<String> other, Enumeration enumeration) {
+		Collections.addAll(list);
+		Collections.addAll(list, "one");
+		Collections.addAll(list, "two", "three");
+		Collections.addAll(list, new String[]{ "four" });
+
+		Collections.checkedList(list, String.class);
+		Collections.min(list);
+		Collections.enumeration(list);
+		Collections.list(enumeration);
+		Collections.singletonMap("key", "value");
+		Collections.copy(list, other);
+		Collections.nCopies(10, "item");
+		Collections.replaceAll(list, "search", "replace");
+	}
+}
+

java/ql/test/library-tests/dataflow/local-additional-taint/localAdditionalTaintStep.expected

@@ -1,3 +1,20 @@
+| CollectionsTest.java:8:28:8:32 | "one" | CollectionsTest.java:8:3:8:33 | new ..[] { .. } |
+| CollectionsTest.java:8:28:8:32 | "one" | CollectionsTest.java:8:22:8:25 | list [post update] |
+| CollectionsTest.java:9:28:9:32 | "two" | CollectionsTest.java:9:3:9:42 | new ..[] { .. } |
+| CollectionsTest.java:9:28:9:32 | "two" | CollectionsTest.java:9:22:9:25 | list [post update] |
+| CollectionsTest.java:9:35:9:41 | "three" | CollectionsTest.java:9:3:9:42 | new ..[] { .. } |
+| CollectionsTest.java:9:35:9:41 | "three" | CollectionsTest.java:9:22:9:25 | list [post update] |
+| CollectionsTest.java:10:28:10:49 | new String[] | CollectionsTest.java:10:22:10:25 | list [post update] |
+| CollectionsTest.java:10:28:10:49 | {...} | CollectionsTest.java:10:28:10:49 | new String[] |
+| CollectionsTest.java:10:42:10:47 | "four" | CollectionsTest.java:10:28:10:49 | {...} |
+| CollectionsTest.java:12:27:12:30 | list | CollectionsTest.java:12:3:12:45 | checkedList(...) |
+| CollectionsTest.java:13:19:13:22 | list | CollectionsTest.java:13:3:13:23 | min(...) |
+| CollectionsTest.java:14:27:14:30 | list | CollectionsTest.java:14:3:14:31 | enumeration(...) |
+| CollectionsTest.java:15:20:15:30 | enumeration | CollectionsTest.java:15:3:15:31 | list(...) |
+| CollectionsTest.java:16:35:16:41 | "value" | CollectionsTest.java:16:3:16:42 | singletonMap(...) |
+| CollectionsTest.java:17:26:17:30 | other | CollectionsTest.java:17:20:17:23 | list [post update] |
+| CollectionsTest.java:18:27:18:32 | "item" | CollectionsTest.java:18:3:18:33 | nCopies(...) |
+| CollectionsTest.java:19:42:19:50 | "replace" | CollectionsTest.java:19:26:19:29 | list [post update] |
 | Test.java:24:32:24:38 | string2 | Test.java:24:17:24:39 | decode(...) |
 | Test.java:25:46:25:51 | bytes2 | Test.java:25:31:25:52 | encode(...) |
 | Test.java:27:34:27:40 | string2 | Test.java:27:13:27:41 | decode(...) |