Skip to content

Commit 5119de8

Browse files
smowtonsmowton
authored
Merge pull request #9238 from atorralba/atorralba/remove-xxe-sinks
Java: Remove org.dom4j.DocumentHelper:parseText as XXE sink
2 parents 7971b54 + 98f70dc commit 5119de8

File tree

3 files changed

+38
-71
lines changed

3 files changed

+38
-71
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-611/XXELib.qll‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -97,26 +97,6 @@ private class SafeValidatorFlowConfig extends DataFlow3::Configuration {
9797
override int fieldFlowBranchLimit() { result = 0 }
9898
}
9999

100-
/** The class `org.dom4j.DocumentHelper`. */
101-
class DocumentHelper extends RefType {
102-
DocumentHelper() { this.hasQualifiedName("org.dom4j", "DocumentHelper") }
103-
}
104-
105-
/** A call to `DocumentHelper.parseText`. */
106-
class DocumentHelperParseText extends XmlParserCall {
107-
DocumentHelperParseText() {
108-
exists(Method m |
109-
this.getMethod() = m and
110-
m.getDeclaringType() instanceof DocumentHelper and
111-
m.hasName("parseText")
112-
)
113-
}
114-
115-
override Expr getSink() { result = this.getArgument(0) }
116-
117-
override predicate isSafe() { none() }
118-
}
119-
120100
/**
121101
* The classes `org.apache.commons.digester3.Digester`, `org.apache.commons.digester.Digester` or `org.apache.tomcat.util.digester.Digester`.
122102
*/

‎java/ql/test/experimental/query-tests/security/CWE-611/XXE.expected‎

Lines changed: 18 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,26 @@
11
edges
22
| XXE.java:22:43:22:66 | getInputStream(...) : ServletInputStream | XXE.java:24:18:24:35 | servletInputStream |
3-
| XXE.java:29:23:29:41 | getReader(...) : BufferedReader | XXE.java:32:17:32:18 | br : BufferedReader |
4-
| XXE.java:32:17:32:18 | br : BufferedReader | XXE.java:32:17:32:29 | readLine(...) : String |
5-
| XXE.java:32:17:32:29 | readLine(...) : String | XXE.java:33:22:33:24 | str : String |
6-
| XXE.java:33:4:33:13 | listString [post update] : StringBuilder | XXE.java:35:48:35:57 | listString : StringBuilder |
7-
| XXE.java:33:22:33:24 | str : String | XXE.java:33:4:33:13 | listString [post update] : StringBuilder |
8-
| XXE.java:35:48:35:57 | listString : StringBuilder | XXE.java:35:48:35:68 | toString(...) |
9-
| XXE.java:40:43:40:66 | getInputStream(...) : ServletInputStream | XXE.java:44:42:44:59 | servletInputStream : ServletInputStream |
10-
| XXE.java:44:25:44:60 | new StreamSource(...) : StreamSource | XXE.java:45:22:45:27 | source |
11-
| XXE.java:44:42:44:59 | servletInputStream : ServletInputStream | XXE.java:44:25:44:60 | new StreamSource(...) : StreamSource |
12-
| XXE.java:50:43:50:66 | getInputStream(...) : ServletInputStream | XXE.java:51:42:51:59 | servletInputStream : ServletInputStream |
13-
| XXE.java:51:27:51:60 | new XMLDecoder(...) : XMLDecoder | XXE.java:52:3:52:12 | xmlDecoder |
14-
| XXE.java:51:42:51:59 | servletInputStream : ServletInputStream | XXE.java:51:27:51:60 | new XMLDecoder(...) : XMLDecoder |
3+
| XXE.java:29:43:29:66 | getInputStream(...) : ServletInputStream | XXE.java:33:42:33:59 | servletInputStream : ServletInputStream |
4+
| XXE.java:33:25:33:60 | new StreamSource(...) : StreamSource | XXE.java:34:22:34:27 | source |
5+
| XXE.java:33:42:33:59 | servletInputStream : ServletInputStream | XXE.java:33:25:33:60 | new StreamSource(...) : StreamSource |
6+
| XXE.java:39:43:39:66 | getInputStream(...) : ServletInputStream | XXE.java:40:42:40:59 | servletInputStream : ServletInputStream |
7+
| XXE.java:40:27:40:60 | new XMLDecoder(...) : XMLDecoder | XXE.java:41:3:41:12 | xmlDecoder |
8+
| XXE.java:40:42:40:59 | servletInputStream : ServletInputStream | XXE.java:40:27:40:60 | new XMLDecoder(...) : XMLDecoder |
159
nodes
1610
| XXE.java:22:43:22:66 | getInputStream(...) : ServletInputStream | semmle.label | getInputStream(...) : ServletInputStream |
1711
| XXE.java:24:18:24:35 | servletInputStream | semmle.label | servletInputStream |
18-
| XXE.java:29:23:29:41 | getReader(...) : BufferedReader | semmle.label | getReader(...) : BufferedReader |
19-
| XXE.java:32:17:32:18 | br : BufferedReader | semmle.label | br : BufferedReader |
20-
| XXE.java:32:17:32:29 | readLine(...) : String | semmle.label | readLine(...) : String |
21-
| XXE.java:33:4:33:13 | listString [post update] : StringBuilder | semmle.label | listString [post update] : StringBuilder |
22-
| XXE.java:33:22:33:24 | str : String | semmle.label | str : String |
23-
| XXE.java:35:48:35:57 | listString : StringBuilder | semmle.label | listString : StringBuilder |
24-
| XXE.java:35:48:35:68 | toString(...) | semmle.label | toString(...) |
25-
| XXE.java:40:43:40:66 | getInputStream(...) : ServletInputStream | semmle.label | getInputStream(...) : ServletInputStream |
26-
| XXE.java:44:25:44:60 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource |
27-
| XXE.java:44:42:44:59 | servletInputStream : ServletInputStream | semmle.label | servletInputStream : ServletInputStream |
28-
| XXE.java:45:22:45:27 | source | semmle.label | source |
29-
| XXE.java:50:43:50:66 | getInputStream(...) : ServletInputStream | semmle.label | getInputStream(...) : ServletInputStream |
30-
| XXE.java:51:27:51:60 | new XMLDecoder(...) : XMLDecoder | semmle.label | new XMLDecoder(...) : XMLDecoder |
31-
| XXE.java:51:42:51:59 | servletInputStream : ServletInputStream | semmle.label | servletInputStream : ServletInputStream |
32-
| XXE.java:52:3:52:12 | xmlDecoder | semmle.label | xmlDecoder |
33-
| XXE.java:57:49:57:72 | getInputStream(...) | semmle.label | getInputStream(...) |
12+
| XXE.java:29:43:29:66 | getInputStream(...) : ServletInputStream | semmle.label | getInputStream(...) : ServletInputStream |
13+
| XXE.java:33:25:33:60 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource |
14+
| XXE.java:33:42:33:59 | servletInputStream : ServletInputStream | semmle.label | servletInputStream : ServletInputStream |
15+
| XXE.java:34:22:34:27 | source | semmle.label | source |
16+
| XXE.java:39:43:39:66 | getInputStream(...) : ServletInputStream | semmle.label | getInputStream(...) : ServletInputStream |
17+
| XXE.java:40:27:40:60 | new XMLDecoder(...) : XMLDecoder | semmle.label | new XMLDecoder(...) : XMLDecoder |
18+
| XXE.java:40:42:40:59 | servletInputStream : ServletInputStream | semmle.label | servletInputStream : ServletInputStream |
19+
| XXE.java:41:3:41:12 | xmlDecoder | semmle.label | xmlDecoder |
20+
| XXE.java:46:49:46:72 | getInputStream(...) | semmle.label | getInputStream(...) |
3421
subpaths
3522
#select
3623
| XXE.java:24:18:24:35 | servletInputStream | XXE.java:22:43:22:66 | getInputStream(...) : ServletInputStream | XXE.java:24:18:24:35 | servletInputStream | Unsafe parsing of XML file from $@. | XXE.java:22:43:22:66 | getInputStream(...) | user input |
37-
| XXE.java:35:48:35:68 | toString(...) | XXE.java:29:23:29:41 | getReader(...) : BufferedReader | XXE.java:35:48:35:68 | toString(...) | Unsafe parsing of XML file from $@. | XXE.java:29:23:29:41 | getReader(...) | user input |
38-
| XXE.java:45:22:45:27 | source | XXE.java:40:43:40:66 | getInputStream(...) : ServletInputStream | XXE.java:45:22:45:27 | source | Unsafe parsing of XML file from $@. | XXE.java:40:43:40:66 | getInputStream(...) | user input |
39-
| XXE.java:52:3:52:12 | xmlDecoder | XXE.java:50:43:50:66 | getInputStream(...) : ServletInputStream | XXE.java:52:3:52:12 | xmlDecoder | Unsafe parsing of XML file from $@. | XXE.java:50:43:50:66 | getInputStream(...) | user input |
40-
| XXE.java:57:49:57:72 | getInputStream(...) | XXE.java:57:49:57:72 | getInputStream(...) | XXE.java:57:49:57:72 | getInputStream(...) | Unsafe parsing of XML file from $@. | XXE.java:57:49:57:72 | getInputStream(...) | user input |
24+
| XXE.java:34:22:34:27 | source | XXE.java:29:43:29:66 | getInputStream(...) : ServletInputStream | XXE.java:34:22:34:27 | source | Unsafe parsing of XML file from $@. | XXE.java:29:43:29:66 | getInputStream(...) | user input |
25+
| XXE.java:41:3:41:12 | xmlDecoder | XXE.java:39:43:39:66 | getInputStream(...) : ServletInputStream | XXE.java:41:3:41:12 | xmlDecoder | Unsafe parsing of XML file from $@. | XXE.java:39:43:39:66 | getInputStream(...) | user input |
26+
| XXE.java:46:49:46:72 | getInputStream(...) | XXE.java:46:49:46:72 | getInputStream(...) | XXE.java:46:49:46:72 | getInputStream(...) | Unsafe parsing of XML file from $@. | XXE.java:46:49:46:72 | getInputStream(...) | user input |

‎java/ql/test/experimental/query-tests/security/CWE-611/XXE.java‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,40 +21,29 @@ public class XXE {
2121
public void bad1(HttpServletRequest request, HttpServletResponse response) throws Exception {
2222
ServletInputStream servletInputStream = request.getInputStream();
2323
Digester digester = new Digester();
24-
digester.parse(servletInputStream); //bad
24+
digester.parse(servletInputStream); // bad
2525
}
2626

2727
@PostMapping(value = "bad2")
2828
public void bad2(HttpServletRequest request) throws Exception {
29-
BufferedReader br = request.getReader();
30-
String str = "";
31-
StringBuilder listString = new StringBuilder();
32-
while ((str = br.readLine()) != null) {
33-
listString.append(str).append("\n");
34-
}
35-
Document document = DocumentHelper.parseText(listString.toString()); //bad
36-
}
37-
38-
@PostMapping(value = "bad3")
39-
public void bad3(HttpServletRequest request) throws Exception {
4029
ServletInputStream servletInputStream = request.getInputStream();
4130
SchemaFactory factory = SchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema");
4231
Schema schema = factory.newSchema();
4332
Validator validator = schema.newValidator();
4433
StreamSource source = new StreamSource(servletInputStream);
45-
validator.validate(source); //bad
34+
validator.validate(source); // bad
4635
}
4736

48-
@PostMapping(value = "bad4")
49-
public void bad4(HttpServletRequest request) throws Exception {
37+
@PostMapping(value = "bad3")
38+
public void bad3(HttpServletRequest request) throws Exception {
5039
ServletInputStream servletInputStream = request.getInputStream();
5140
XMLDecoder xmlDecoder = new XMLDecoder(servletInputStream);
52-
xmlDecoder.readObject(); //bad
41+
xmlDecoder.readObject(); // bad
5342
}
5443

55-
@PostMapping(value = "bad5")
56-
public void bad5(HttpServletRequest request) throws Exception {
57-
Document document = ParserHelper.loadDocument(request.getInputStream()); //bad
44+
@PostMapping(value = "bad4")
45+
public void bad4(HttpServletRequest request) throws Exception {
46+
Document document = ParserHelper.loadDocument(request.getInputStream()); // bad
5847
}
5948

6049
@PostMapping(value = "good1")
@@ -88,4 +77,16 @@ public void good2(HttpServletRequest request, HttpServletResponse response) thro
8877
StreamSource source = new StreamSource(listString.toString());
8978
validator.validate(source);
9079
}
80+
81+
@PostMapping(value = "good3")
82+
public void good3(HttpServletRequest request) throws Exception {
83+
BufferedReader br = request.getReader();
84+
String str = "";
85+
StringBuilder listString = new StringBuilder();
86+
while ((str = br.readLine()) != null) {
87+
listString.append(str).append("\n");
88+
}
89+
// parseText falls back to a default SAXReader, which is safe
90+
Document document = DocumentHelper.parseText(listString.toString()); // Safe
91+
}
9192
}

0 commit comments

Comments
 (0)