Skip to content

Commit 7d79d87

Browse files
atorralbaatorralba
committed
Add XPath.evaluate as XXE sink
1 parent 9dede31 commit 7d79d87

File tree

4 files changed

+59
-19
lines changed

4 files changed

+59
-19
lines changed

‎java/ql/lib/semmle/code/java/security/XmlParsers.qll

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -655,6 +655,11 @@ class XmlReader extends RefType {
655655
XmlReader() { this.hasQualifiedName("org.xml.sax", "XMLReader") }
656656
}
657657

658+
/** The class `org.xml.sax.InputSource`. */
659+
class InputSource extends Class {
660+
InputSource() { this.hasQualifiedName("org.xml.sax", "InputSource") }
661+
}
662+
658663
/** DEPRECATED: Alias for XmlReader */
659664
deprecated class XMLReader = XmlReader;
660665

@@ -1164,22 +1169,34 @@ class XmlUnmarshal extends XmlParserCall {
11641169
}
11651170

11661171
/* XPathExpression: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#xpathexpression */
1167-
/** The class `javax.xml.xpath.XPathExpression`. */
1168-
class XPathExpression extends RefType {
1172+
/** The interface `javax.xml.xpath.XPathExpression`. */
1173+
class XPathExpression extends Interface {
11691174
XPathExpression() { this.hasQualifiedName("javax.xml.xpath", "XPathExpression") }
11701175
}
11711176

1172-
/** A call to `XPathExpression.evaluate`. */
1177+
/** The interface `java.xml.xpath.XPath`. */
1178+
class XPath extends Interface {
1179+
XPath() { this.hasQualifiedName("javax.xml.xpath", "XPath") }
1180+
}
1181+
1182+
/** A call to the method `evaluate` of the classes `XPathExpression` or `XPath`. */
11731183
class XPathEvaluate extends XmlParserCall {
1184+
Argument sink;
1185+
11741186
XPathEvaluate() {
11751187
exists(Method m |
11761188
this.getMethod() = m and
1177-
m.getDeclaringType() instanceof XPathExpression and
11781189
m.hasName("evaluate")
1190+
|
1191+
m.getDeclaringType().getASourceSupertype*() instanceof XPathExpression and
1192+
sink = this.getArgument(0)
1193+
or
1194+
m.getDeclaringType().getASourceSupertype*() instanceof XPath and
1195+
sink = this.getArgument(1)
11791196
)
11801197
}
11811198

1182-
override Expr getSink() { result = this.getArgument(0) }
1199+
override Expr getSink() { result = sink }
11831200

11841201
override predicate isSafe() { none() }
11851202
}

‎java/ql/src/change-notes/2023-05-15-xpath-xxe-sink.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The queries `java/xxe` and `java/xxe-local` now recognize the second argument of calls to `XPath.evaluate` as a sink.

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

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,33 @@ public class XPathExpressionTests {
1212

1313
public void safeXPathExpression(Socket sock) throws Exception {
1414
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
15-
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
16-
DocumentBuilder builder = factory.newDocumentBuilder();
17-
XPathFactory xFactory = XPathFactory.newInstance();
18-
XPath path = xFactory.newXPath();
19-
XPathExpression expr = path.compile("");
20-
expr.evaluate(builder.parse(sock.getInputStream())); //safe
15+
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
16+
DocumentBuilder builder = factory.newDocumentBuilder();
17+
XPathFactory xFactory = XPathFactory.newInstance();
18+
XPath path = xFactory.newXPath();
19+
XPathExpression expr = path.compile("");
20+
expr.evaluate(builder.parse(sock.getInputStream())); // safe
2121
}
2222

2323
public void unsafeExpressionTests(Socket sock) throws Exception {
24-
XPathFactory xFactory = XPathFactory.newInstance();
25-
XPath path = xFactory.newXPath();
26-
XPathExpression expr = path.compile("");
27-
expr.evaluate(new InputSource(sock.getInputStream())); //unsafe
24+
XPathFactory xFactory = XPathFactory.newInstance();
25+
XPath path = xFactory.newXPath();
26+
XPathExpression expr = path.compile("");
27+
expr.evaluate(new InputSource(sock.getInputStream())); // unsafe
28+
}
29+
30+
public void safeXPathEvaluateTest(Socket sock) throws Exception {
31+
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
32+
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
33+
DocumentBuilder builder = factory.newDocumentBuilder();
34+
XPathFactory xFactory = XPathFactory.newInstance();
35+
XPath path = xFactory.newXPath();
36+
path.evaluate("", builder.parse(sock.getInputStream()));
37+
}
38+
39+
public void unsafeXPathEvaluateTest(Socket sock) throws Exception {
40+
XPathFactory xFactory = XPathFactory.newInstance();
41+
XPath path = xFactory.newXPath();
42+
path.evaluate("", new InputSource(sock.getInputStream())); // unsafe
2843
}
2944
}

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ edges
7474
| XMLReaderTests.java:86:34:86:54 | getInputStream(...) : InputStream | XMLReaderTests.java:86:18:86:55 | new InputSource(...) |
7575
| XMLReaderTests.java:94:34:94:54 | getInputStream(...) : InputStream | XMLReaderTests.java:94:18:94:55 | new InputSource(...) |
7676
| XMLReaderTests.java:100:34:100:54 | getInputStream(...) : InputStream | XMLReaderTests.java:100:18:100:55 | new InputSource(...) |
77-
| XPathExpressionTests.java:27:37:27:57 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:21:27:58 | new InputSource(...) |
77+
| XPathExpressionTests.java:27:35:27:55 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:19:27:56 | new InputSource(...) |
78+
| XPathExpressionTests.java:42:39:42:59 | getInputStream(...) : InputStream | XPathExpressionTests.java:42:23:42:60 | new InputSource(...) |
7879
nodes
7980
| DocumentBuilderTests.java:14:19:14:39 | getInputStream(...) | semmle.label | getInputStream(...) |
8081
| DocumentBuilderTests.java:28:19:28:39 | getInputStream(...) | semmle.label | getInputStream(...) |
@@ -235,8 +236,10 @@ nodes
235236
| XMLReaderTests.java:94:34:94:54 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
236237
| XMLReaderTests.java:100:18:100:55 | new InputSource(...) | semmle.label | new InputSource(...) |
237238
| XMLReaderTests.java:100:34:100:54 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
238-
| XPathExpressionTests.java:27:21:27:58 | new InputSource(...) | semmle.label | new InputSource(...) |
239-
| XPathExpressionTests.java:27:37:27:57 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
239+
| XPathExpressionTests.java:27:19:27:56 | new InputSource(...) | semmle.label | new InputSource(...) |
240+
| XPathExpressionTests.java:27:35:27:55 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
241+
| XPathExpressionTests.java:42:23:42:60 | new InputSource(...) | semmle.label | new InputSource(...) |
242+
| XPathExpressionTests.java:42:39:42:59 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
240243
| XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | semmle.label | getInputStream(...) |
241244
| XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | semmle.label | getInputStream(...) |
242245
| XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | semmle.label | getInputStream(...) |
@@ -336,7 +339,8 @@ subpaths
336339
| XMLReaderTests.java:86:18:86:55 | new InputSource(...) | XMLReaderTests.java:86:34:86:54 | getInputStream(...) : InputStream | XMLReaderTests.java:86:18:86:55 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XMLReaderTests.java:86:34:86:54 | getInputStream(...) | user-provided value |
337340
| XMLReaderTests.java:94:18:94:55 | new InputSource(...) | XMLReaderTests.java:94:34:94:54 | getInputStream(...) : InputStream | XMLReaderTests.java:94:18:94:55 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XMLReaderTests.java:94:34:94:54 | getInputStream(...) | user-provided value |
338341
| XMLReaderTests.java:100:18:100:55 | new InputSource(...) | XMLReaderTests.java:100:34:100:54 | getInputStream(...) : InputStream | XMLReaderTests.java:100:18:100:55 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XMLReaderTests.java:100:34:100:54 | getInputStream(...) | user-provided value |
339-
| XPathExpressionTests.java:27:21:27:58 | new InputSource(...) | XPathExpressionTests.java:27:37:27:57 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:21:27:58 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XPathExpressionTests.java:27:37:27:57 | getInputStream(...) | user-provided value |
342+
| XPathExpressionTests.java:27:19:27:56 | new InputSource(...) | XPathExpressionTests.java:27:35:27:55 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:19:27:56 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XPathExpressionTests.java:27:35:27:55 | getInputStream(...) | user-provided value |
343+
| XPathExpressionTests.java:42:23:42:60 | new InputSource(...) | XPathExpressionTests.java:42:39:42:59 | getInputStream(...) : InputStream | XPathExpressionTests.java:42:23:42:60 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XPathExpressionTests.java:42:39:42:59 | getInputStream(...) | user-provided value |
340344
| XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | user-provided value |
341345
| XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | user-provided value |
342346
| XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | user-provided value |

0 commit comments

Comments
 (0)