diff --git a/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll b/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll index 566ec3a45e7d..5a631a2fdff3 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll +++ b/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) } /** 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 bc7b43558620..870c4b320c75 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll +++ b/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 | diff --git a/java/ql/test/library-tests/dataflow/local-additional-taint/CollectionsTest.java b/java/ql/test/library-tests/dataflow/local-additional-taint/CollectionsTest.java new file mode 100644 index 000000000000..02d542fd43a7 --- /dev/null +++ b/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 list, List 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"); + } +} + diff --git a/java/ql/test/library-tests/dataflow/local-additional-taint/localAdditionalTaintStep.expected b/java/ql/test/library-tests/dataflow/local-additional-taint/localAdditionalTaintStep.expected index 66e1184ce87a..43e179695152 100644 --- a/java/ql/test/library-tests/dataflow/local-additional-taint/localAdditionalTaintStep.expected +++ b/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(...) |