Skip to content

Commit 6eafa9d

Browse files
aschackmullaschackmull
authored
Merge pull request github#5133 from pwntester/fix_SnakeYaml
Remove sanitizing condition which does not prevent vulnerability.
2 parents b309b71 + 3d3f4ba commit 6eafa9d

File tree

4 files changed

+28
-19
lines changed

4 files changed

+28
-19
lines changed

‎java/change-notes/2021-02-15-snakeyaml-fn-fix.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
lgtm,codescanning
2+
* The query "Unsafe Deserialization" (`java/unsafe-deserialization`) has been
3+
improved to report those cases where SnakeYaml `Constructor` is used to fix
4+
the unmarshaled object graph root's type but injection is still possible in
5+
nested nodes of the object graph.

‎java/ql/src/semmle/code/java/frameworks/SnakeYaml.qll

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,6 @@ import semmle.code.java.dataflow.DataFlow
77
import semmle.code.java.dataflow.DataFlow2
88
import semmle.code.java.dataflow.DataFlow3
99

10-
/**
11-
* The class `org.yaml.snakeyaml.constructor.Constructor`.
12-
*/
13-
class SnakeYamlConstructor extends RefType {
14-
SnakeYamlConstructor() { this.hasQualifiedName("org.yaml.snakeyaml.constructor", "Constructor") }
15-
}
16-
1710
/**
1811
* The class `org.yaml.snakeyaml.constructor.SafeConstructor`.
1912
*/
@@ -24,15 +17,10 @@ class SnakeYamlSafeConstructor extends RefType {
2417
}
2518

2619
/**
27-
* An instance of `SafeConstructor` or a `Constructor` that only allows the type that is passed into its argument.
20+
* An instance of `SafeConstructor`.
2821
*/
2922
class SafeSnakeYamlConstruction extends ClassInstanceExpr {
30-
SafeSnakeYamlConstruction() {
31-
this.getConstructedType() instanceof SnakeYamlSafeConstructor
32-
or
33-
this.getConstructedType() instanceof SnakeYamlConstructor and
34-
this.getNumArgument() > 0
35-
}
23+
SafeSnakeYamlConstruction() { this.getConstructedType() instanceof SnakeYamlSafeConstructor }
3624
}
3725

3826
/**

‎java/ql/test/query-tests/security/CWE-502/A.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,10 @@ public void deserializeSnakeYaml3(Socket sock) throws java.io.IOException {
8888
public void deserializeSnakeYaml4(Socket sock) throws java.io.IOException {
8989
Yaml yaml = new Yaml(new Constructor(A.class));
9090
InputStream input = sock.getInputStream();
91-
Object o = yaml.load(input); //OK
92-
Object o2 = yaml.loadAll(input); //OK
93-
Object o3 = yaml.parse(new InputStreamReader(input)); //OK
94-
A o4 = yaml.loadAs(input, A.class); //OK
95-
A o5 = yaml.loadAs(new InputStreamReader(input), A.class); //OK
91+
Object o = yaml.load(input); //unsafe
92+
Object o2 = yaml.loadAll(input); //unsafe
93+
Object o3 = yaml.parse(new InputStreamReader(input)); //unsafe
94+
A o4 = yaml.loadAs(input, A.class); //unsafe
95+
A o5 = yaml.loadAs(new InputStreamReader(input), A.class); //unsafe
9696
}
9797
}

‎java/ql/test/query-tests/security/CWE-502/UnsafeDeserialization.expected

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@ edges
1616
| A.java:70:25:70:45 | getInputStream(...) : InputStream | A.java:73:28:73:55 | new InputStreamReader(...) |
1717
| A.java:70:25:70:45 | getInputStream(...) : InputStream | A.java:74:24:74:28 | input |
1818
| A.java:70:25:70:45 | getInputStream(...) : InputStream | A.java:75:24:75:51 | new InputStreamReader(...) |
19+
| A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:91:26:91:30 | input |
20+
| A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:92:30:92:34 | input |
21+
| A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:93:28:93:55 | new InputStreamReader(...) |
22+
| A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:94:24:94:28 | input |
23+
| A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:95:24:95:51 | new InputStreamReader(...) |
1924
| B.java:7:31:7:51 | getInputStream(...) : InputStream | B.java:8:29:8:39 | inputStream |
2025
| B.java:12:31:12:51 | getInputStream(...) : InputStream | B.java:15:23:15:27 | bytes |
2126
| B.java:19:31:19:51 | getInputStream(...) : InputStream | B.java:23:29:23:29 | s |
@@ -46,6 +51,12 @@ nodes
4651
| A.java:73:28:73:55 | new InputStreamReader(...) | semmle.label | new InputStreamReader(...) |
4752
| A.java:74:24:74:28 | input | semmle.label | input |
4853
| A.java:75:24:75:51 | new InputStreamReader(...) | semmle.label | new InputStreamReader(...) |
54+
| A.java:90:25:90:45 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
55+
| A.java:91:26:91:30 | input | semmle.label | input |
56+
| A.java:92:30:92:34 | input | semmle.label | input |
57+
| A.java:93:28:93:55 | new InputStreamReader(...) | semmle.label | new InputStreamReader(...) |
58+
| A.java:94:24:94:28 | input | semmle.label | input |
59+
| A.java:95:24:95:51 | new InputStreamReader(...) | semmle.label | new InputStreamReader(...) |
4960
| B.java:7:31:7:51 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
5061
| B.java:8:29:8:39 | inputStream | semmle.label | inputStream |
5162
| B.java:12:31:12:51 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
@@ -74,6 +85,11 @@ nodes
7485
| A.java:73:17:73:56 | parse(...) | A.java:70:25:70:45 | getInputStream(...) : InputStream | A.java:73:28:73:55 | new InputStreamReader(...) | Unsafe deserialization of $@. | A.java:70:25:70:45 | getInputStream(...) | user input |
7586
| A.java:74:12:74:38 | loadAs(...) | A.java:70:25:70:45 | getInputStream(...) : InputStream | A.java:74:24:74:28 | input | Unsafe deserialization of $@. | A.java:70:25:70:45 | getInputStream(...) | user input |
7687
| A.java:75:12:75:61 | loadAs(...) | A.java:70:25:70:45 | getInputStream(...) : InputStream | A.java:75:24:75:51 | new InputStreamReader(...) | Unsafe deserialization of $@. | A.java:70:25:70:45 | getInputStream(...) | user input |
88+
| A.java:91:16:91:31 | load(...) | A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:91:26:91:30 | input | Unsafe deserialization of $@. | A.java:90:25:90:45 | getInputStream(...) | user input |
89+
| A.java:92:17:92:35 | loadAll(...) | A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:92:30:92:34 | input | Unsafe deserialization of $@. | A.java:90:25:90:45 | getInputStream(...) | user input |
90+
| A.java:93:17:93:56 | parse(...) | A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:93:28:93:55 | new InputStreamReader(...) | Unsafe deserialization of $@. | A.java:90:25:90:45 | getInputStream(...) | user input |
91+
| A.java:94:12:94:38 | loadAs(...) | A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:94:24:94:28 | input | Unsafe deserialization of $@. | A.java:90:25:90:45 | getInputStream(...) | user input |
92+
| A.java:95:12:95:61 | loadAs(...) | A.java:90:25:90:45 | getInputStream(...) : InputStream | A.java:95:24:95:51 | new InputStreamReader(...) | Unsafe deserialization of $@. | A.java:90:25:90:45 | getInputStream(...) | user input |
7793
| B.java:8:12:8:46 | parseObject(...) | B.java:7:31:7:51 | getInputStream(...) : InputStream | B.java:8:29:8:39 | inputStream | Unsafe deserialization of $@. | B.java:7:31:7:51 | getInputStream(...) | user input |
7894
| B.java:15:12:15:28 | parse(...) | B.java:12:31:12:51 | getInputStream(...) : InputStream | B.java:15:23:15:27 | bytes | Unsafe deserialization of $@. | B.java:12:31:12:51 | getInputStream(...) | user input |
7995
| B.java:23:12:23:30 | parseObject(...) | B.java:19:31:19:51 | getInputStream(...) : InputStream | B.java:23:29:23:29 | s | Unsafe deserialization of $@. | B.java:19:31:19:51 | getInputStream(...) | user input |

0 commit comments

Comments
 (0)