From 5cf5c77b09ebb00009057a0c101f8405befb43ed Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Thu, 2 Jul 2020 12:01:17 +0200 Subject: [PATCH 1/6] Java: model java.util.Collections --- .../java/dataflow/internal/ContainerFlow.qll | 64 ++++++++++++++++++- 1 file changed, 62 insertions(+), 2 deletions(-) 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..7454ec7f7c05 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,41 @@ 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(["singleton", "singletonList", "singletonMap", "enumeration", "list", "max", "min", + "asLifoQueue", "checkedCollection", "checkedList", "checkedMap", "checkedSet", + "checkedSortedMap", "checkedSortedSet", "synchronizedCollection", "synchronizedList", + "synchronizedMap", "synchronizedSet", "synchronizedSortedMap", + "synchronizedSortedSet", "unmodifiableCollection", "unmodifiableList", + "unmodifiableMap", "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 +203,37 @@ 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.(MethodAccess).getMethod() and + taintPreservingArgumentToMethod(m, i) and + tracked = sink.(MethodAccess).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 | + taintPreservingArgToArg(method, input, output) and + ma.getMethod() = method and + ma.getArgument(input) = tracked and + ma.getArgument(output) = sink + ) +} + /** * 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 +242,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) } /** From e7b495e7d3a57fbf39105e3ce82d201c20df85e2 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Thu, 2 Jul 2020 12:38:22 +0200 Subject: [PATCH 2/6] Java: model Collections::addAll --- .../code/java/dataflow/internal/ContainerFlow.qll | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) 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 7454ec7f7c05..ab9dd560d493 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll @@ -218,10 +218,17 @@ private predicate argToMethodStep(Expr tracked, MethodAccess sink) { */ private predicate argToArgStep(Expr tracked, Expr sink) { exists(MethodAccess ma, Method method, int input, int output | - taintPreservingArgToArg(method, input, output) and ma.getMethod() = method and ma.getArgument(input) = tracked and - ma.getArgument(output) = sink + 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 + ) ) } From d80bf3395f0a07cd39b7fc0217b7c5228df937db Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Thu, 2 Jul 2020 13:02:38 +0200 Subject: [PATCH 3/6] Add Navigable variants and sort method names --- .../code/java/dataflow/internal/ContainerFlow.qll | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) 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 ab9dd560d493..8f59cc348202 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll @@ -167,12 +167,15 @@ private predicate taintPreservingArgumentToMethod(Method method, int arg) { method.getDeclaringType().hasQualifiedName("java.util", "Collections") and ( method - .hasName(["singleton", "singletonList", "singletonMap", "enumeration", "list", "max", "min", - "asLifoQueue", "checkedCollection", "checkedList", "checkedMap", "checkedSet", - "checkedSortedMap", "checkedSortedSet", "synchronizedCollection", "synchronizedList", - "synchronizedMap", "synchronizedSet", "synchronizedSortedMap", - "synchronizedSortedSet", "unmodifiableCollection", "unmodifiableList", - "unmodifiableMap", "unmodifiableSet", "unmodifiableSortedMap", "unmodifiableSortedSet"]) and + .hasName(["checkedCollection", "checkedList", "checkedMap", "checkedNavigableMap", + "checkedNavigableSet", "checkedSet", "checkedSortedMap", "checkedSortedSet", + "enumeration", "list", "max", "min", "singleton", "singletonList", "singletonMap", + "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 From 21a4b8d6c0a279064053bdd620453051c009f75e Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Thu, 2 Jul 2020 13:03:15 +0200 Subject: [PATCH 4/6] Java: remove useless casts --- .../src/semmle/code/java/dataflow/internal/ContainerFlow.qll | 4 ++-- .../semmle/code/java/dataflow/internal/TaintTrackingUtil.qll | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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 8f59cc348202..3467af8c5842 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll @@ -209,9 +209,9 @@ 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.(MethodAccess).getMethod() and + m = sink.getMethod() and taintPreservingArgumentToMethod(m, i) and - tracked = sink.(MethodAccess).getArgument(i) + tracked = sink.getArgument(i) ) } 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 | From 5f2a5f1b554503985ab1392e2658ee2bb3e962b6 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Thu, 2 Jul 2020 18:19:07 +0200 Subject: [PATCH 5/6] Java: Collections: add tests --- .../CollectionsTest.java | 22 +++++++++++++++++++ .../localAdditionalTaintStep.expected | 18 +++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 java/ql/test/library-tests/dataflow/local-additional-taint/CollectionsTest.java 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..07c218d02c36 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,21 @@ +| 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:28:16:32 | "key" | CollectionsTest.java:16:3:16:42 | singletonMap(...) | +| 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(...) | From 5fff41f35b7dc6e51477833ab644979e600e26ff Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Fri, 3 Jul 2020 13:36:56 +0200 Subject: [PATCH 6/6] Don't track taint on Map keys --- .../ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll | 2 +- .../local-additional-taint/localAdditionalTaintStep.expected | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) 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 3467af8c5842..5a631a2fdff3 100644 --- a/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll @@ -169,7 +169,7 @@ private predicate taintPreservingArgumentToMethod(Method method, int arg) { method .hasName(["checkedCollection", "checkedList", "checkedMap", "checkedNavigableMap", "checkedNavigableSet", "checkedSet", "checkedSortedMap", "checkedSortedSet", - "enumeration", "list", "max", "min", "singleton", "singletonList", "singletonMap", + "enumeration", "list", "max", "min", "singleton", "singletonList", "synchronizedCollection", "synchronizedList", "synchronizedMap", "synchronizedNavigableMap", "synchronizedNavigableSet", "synchronizedSet", "synchronizedSortedMap", "synchronizedSortedSet", "unmodifiableCollection", 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 07c218d02c36..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 @@ -11,7 +11,6 @@ | 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:28:16:32 | "key" | CollectionsTest.java:16:3:16:42 | singletonMap(...) | | 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(...) |