Skip to content

Commit 0d33a77

Browse files
committed
Fix modelling of Stack.push
Stack.push(E) returns its argument, it does not propagate taint from the stack to the return value.
1 parent d3d5879 commit 0d33a77

File tree

3 files changed

+7
-4
lines changed

3 files changed

+7
-4
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ private predicate taintPreservingQualifierToMethod(Method m) {
127127
m.(CollectionMethod).hasName(["elementAt", "elements", "firstElement", "lastElement"])
128128
or
129129
// java.util.Stack
130-
m.(CollectionMethod).hasName(["peek", "pop", "push"])
130+
m.(CollectionMethod).hasName(["peek", "pop"])
131131
or
132132
// java.util.Queue
133133
m.(CollectionMethod).hasName(["element", "poll"])
@@ -269,6 +269,9 @@ private predicate taintPreservingArgumentToQualifier(Method method, int arg) {
269269
* `arg`th argument is tainted.
270270
*/
271271
private predicate taintPreservingArgumentToMethod(Method method, int arg) {
272+
// java.util.Stack
273+
method.(CollectionMethod).hasName("push") and arg = 0
274+
or
272275
method.getDeclaringType().hasQualifiedName("java.util", "Collections") and
273276
(
274277
method

java/ql/test/library-tests/dataflow/collections/ContainterTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ public static void taintSteps(
8888
// java.util.Stack
8989
sink(stack.peek());
9090
sink(stack.pop());
91-
stack.push("value"); // not tainted
92-
sink(stack.push(source("value")));
91+
sink(stack.push("value")); // not tainted
92+
sink(new Stack().push(source("value")));
9393
mkSink(Stack.class).push(source("value"));
9494

9595
// java.util.Queue

java/ql/test/library-tests/dataflow/collections/flow.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
| ContainterTest.java:34:4:34:24 | vector | ContainterTest.java:86:19:86:40 | mkSink(...) [post update] |
2222
| ContainterTest.java:35:4:35:22 | stack | ContainterTest.java:89:8:89:19 | peek(...) |
2323
| ContainterTest.java:35:4:35:22 | stack | ContainterTest.java:90:8:90:18 | pop(...) |
24-
| ContainterTest.java:35:4:35:22 | stack | ContainterTest.java:92:8:92:34 | push(...) |
2524
| ContainterTest.java:36:4:36:22 | queue | ContainterTest.java:96:8:96:22 | element(...) |
2625
| ContainterTest.java:36:4:36:22 | queue | ContainterTest.java:97:8:97:19 | peek(...) |
2726
| ContainterTest.java:36:4:36:22 | queue | ContainterTest.java:98:8:98:19 | poll(...) |
@@ -104,6 +103,7 @@
104103
| ContainterTest.java:83:42:83:50 | "element" | ContainterTest.java:83:3:83:22 | mkSink(...) [post update] |
105104
| ContainterTest.java:84:47:84:55 | "element" | ContainterTest.java:84:3:84:22 | mkSink(...) [post update] |
106105
| ContainterTest.java:85:44:85:52 | "element" | ContainterTest.java:85:3:85:22 | mkSink(...) [post update] |
106+
| ContainterTest.java:92:32:92:38 | "value" | ContainterTest.java:92:8:92:40 | push(...) |
107107
| ContainterTest.java:93:35:93:41 | "value" | ContainterTest.java:93:3:93:21 | mkSink(...) [post update] |
108108
| ContainterTest.java:100:36:100:44 | "element" | ContainterTest.java:100:3:100:21 | mkSink(...) [post update] |
109109
| ContainterTest.java:111:39:111:45 | "value" | ContainterTest.java:111:3:111:21 | mkSink(...) [post update] |

0 commit comments

Comments
 (0)