Skip to content

Commit ed56194

Browse files
atorralbaatorralba
committed
WIP: XPath Injection promotion
1 parent 8b2009c commit ed56194

File tree

8 files changed

+202
-24
lines changed

8 files changed

+202
-24
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -11,32 +11,9 @@
1111
*/
1212

1313
import java
14-
import semmle.code.java.dataflow.FlowSources
1514
import semmle.code.java.dataflow.TaintTracking
16-
import semmle.code.java.security.XmlParsers
1715
import DataFlow::PathGraph
18-
19-
class XPathInjectionConfiguration extends TaintTracking::Configuration {
20-
XPathInjectionConfiguration() { this = "XPathInjection" }
21-
22-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
23-
24-
override predicate isSink(DataFlow::Node sink) { sink instanceof XPathInjectionSink }
25-
}
26-
27-
class XPathInjectionSink extends DataFlow::ExprNode {
28-
XPathInjectionSink() {
29-
exists(Method m, MethodAccess ma | ma.getMethod() = m |
30-
m.getDeclaringType().hasQualifiedName("javax.xml.xpath", "XPath") and
31-
(m.hasName("evaluate") or m.hasName("compile")) and
32-
ma.getArgument(0) = this.getExpr()
33-
or
34-
m.getDeclaringType().hasQualifiedName("org.dom4j", "Node") and
35-
(m.hasName("selectNodes") or m.hasName("selectSingleNode")) and
36-
ma.getArgument(0) = this.getExpr()
37-
)
38-
}
39-
}
16+
import semmle.code.java.security.XPath
4017

4118
from DataFlow::PathNode source, DataFlow::PathNode sink, XPathInjectionConfiguration c
4219
where c.hasFlowPath(source, sink)

‎java/ql/src/semmle/code/java/security/XPath.qll

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import semmle.code.java.dataflow.FlowSources
2+
import semmle.code.java.dataflow.TaintTracking
3+
import semmle.code.java.security.XmlParsers
4+
5+
/**
6+
* An abstract type representing a call to interpret XPath expressions.
7+
*/
8+
class XPathSink extends MethodAccess {
9+
abstract Expr getSink();
10+
}
11+
12+
/** The class `javax.xml.xpath.XPath` */
13+
class XPath extends RefType {
14+
XPath() { this.hasQualifiedName("javax.xml.xpath", "XPath") }
15+
}
16+
17+
/** A call to `XPath.Evaluate` or `XPath.compile` */
18+
class XPathEvaluateOrCompile extends XPathSink {
19+
XPathEvaluateOrCompile() {
20+
exists(Method m | this.getMethod() = m and m.getDeclaringType() instanceof XPath |
21+
m.hasName("evaluate")
22+
or
23+
m.hasName("compile")
24+
)
25+
}
26+
27+
override Expr getSink() { result = this.getArgument(0) }
28+
}
29+
30+
/** The class `org.dom4j.Node` */
31+
class Dom4JNode extends RefType {
32+
Dom4JNode() { this.hasQualifiedName("org.dom4j", "Node") }
33+
}
34+
35+
/** A call to `Node.selectNodes` or `Node.selectSingleNode` */
36+
class NodeSelectNodes extends XPathSink {
37+
NodeSelectNodes() {
38+
exists(Method m | this.getMethod() = m and m.getDeclaringType() instanceof Dom4JNode |
39+
m.hasName("selectNodes") or m.hasName("selectSingleNode")
40+
)
41+
}
42+
43+
override Expr getSink() { result = this.getArgument(0) }
44+
}
45+
46+
class XPathInjectionSink extends DataFlow::ExprNode {
47+
XPathInjectionSink() { exists(XPathSink sink | this.getExpr() = sink.getSink()) }
48+
}
49+
50+
class XPathInjectionConfiguration extends TaintTracking::Configuration {
51+
XPathInjectionConfiguration() { this = "XPathInjection" }
52+
53+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
54+
55+
override predicate isSink(DataFlow::Node sink) { sink instanceof XPathInjectionSink }
56+
}

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import javax.xml.*;
2+
3+
import org.w3c.dom.Document;
4+
import org.xml.sax.InputSource;
5+
import org.xml.sax.SAXException;
6+
7+
import javax.xml.parsers.DocumentBuilder;
8+
import javax.xml.parsers.DocumentBuilderFactory;
9+
import javax.xml.parsers.ParserConfigurationException;
10+
import javax.xml.xpath.XPath;
11+
import javax.xml.xpath.XPathConstants;
12+
import javax.xml.xpath.XPathExpression;
13+
import javax.xml.xpath.XPathExpressionException;
14+
import javax.xml.xpath.XPathFactory;
15+
16+
import java.io.BufferedInputStream;
17+
import java.io.ByteArrayInputStream;
18+
import java.io.InputStream;
19+
import java.io.StringReader;
20+
21+
import javax.servlet.http.HttpServletRequest;
22+
23+
public class A {
24+
public void handle(HttpServletRequest request) throws Exception {
25+
final String xmlStr = "<users>" + " <user name=\"aaa\" pass=\"pass1\"></user>"
26+
+ " <user name=\"bbb\" pass=\"pass2\"></user>" + "</users>";
27+
DocumentBuilderFactory domFactory = DocumentBuilderFactory.newInstance();
28+
domFactory.setNamespaceAware(true);
29+
DocumentBuilder builder = domFactory.newDocumentBuilder();
30+
Document doc = builder.parse(new InputSource(new StringReader(xmlStr)));
31+
32+
XPathFactory factory = XPathFactory.newInstance();
33+
XPath xpath = factory.newXPath();
34+
35+
// Injectable data
36+
String user = request.getParameter("user");
37+
String pass = request.getParameter("pass");
38+
if (user != null && pass != null) {
39+
boolean isExist = false;
40+
41+
// Bad expression
42+
String expression1 = "/users/user[@name='" + user + "' and @pass='" + pass + "']";
43+
isExist = (boolean) xpath.evaluate(expression1, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
44+
System.out.println(isExist);
45+
46+
// Bad expression
47+
XPathExpression expression2 = xpath.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
48+
isExist = (boolean) expression2.evaluate(doc, XPathConstants.BOOLEAN);
49+
System.out.println(isExist);
50+
51+
// Bad expression
52+
StringBuffer sb = new StringBuffer("/users/user[@name=");
53+
sb.append(user);
54+
sb.append("' and @pass='");
55+
sb.append(pass);
56+
sb.append("']");
57+
String query = sb.toString();
58+
XPathExpression expression3 = xpath.compile(query); // $hasXPathInjection
59+
isExist = (boolean) expression3.evaluate(doc, XPathConstants.BOOLEAN);
60+
System.out.println(isExist);
61+
62+
// Good expression
63+
String expression4 = "/users/user[@name=$user and @pass=$pass]";
64+
xpath.setXPathVariableResolver(v -> {
65+
switch (v.getLocalPart()) {
66+
case "user":
67+
return user;
68+
case "pass":
69+
return pass;
70+
default:
71+
throw new IllegalArgumentException();
72+
}
73+
});
74+
isExist = (boolean) xpath.evaluate(expression4, doc, XPathConstants.BOOLEAN);
75+
System.out.println(isExist);
76+
77+
// Bad Dom4j
78+
org.dom4j.io.SAXReader reader = new org.dom4j.io.SAXReader();
79+
org.dom4j.Document document = reader.read(new ByteArrayInputStream(xmlStr.getBytes()));
80+
isExist = document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']") // $hasXPathInjection
81+
.hasContent();
82+
document.selectNodes("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
83+
}
84+
}
85+
}

‎java/ql/test/experimental/query-tests/security/CWE-643/XPathInjectionTest.expected

Whitespace-only changes.

‎java/ql/test/experimental/query-tests/security/CWE-643/XPathInjectionTest.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import java
2+
import semmle.code.java.dataflow.TaintTracking
3+
import semmle.code.java.dataflow.FlowSources
4+
import semmle.code.java.security.XPath
5+
import TestUtilities.InlineExpectationsTest
6+
7+
class HasXPathInjectionTest extends InlineExpectationsTest {
8+
HasXPathInjectionTest() { this = "HasXPathInjectionTest" }
9+
10+
override string getARelevantTag() { result = "hasXPathInjection" }
11+
12+
override predicate hasActualResult(Location location, string element, string tag, string value) {
13+
tag = "hasXPathInjection" and
14+
exists(DataFlow::Node src, DataFlow::Node sink, XPathInjectionConfiguration conf |
15+
conf.hasFlow(src, sink)
16+
|
17+
sink.getLocation() = location and
18+
element = sink.toString() and
19+
value = ""
20+
)
21+
}
22+
}

‎java/ql/test/experimental/query-tests/security/CWE-643/options

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/dom4j-2.1.1:${testdir}/../../../../stubs/servlet-api-2.4

‎java/ql/test/stubs/dom4j-2.1.1/org/dom4j/Document.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,13 @@
1313

1414
package org.dom4j;
1515

16+
import java.util.List;
17+
1618
public interface Document {
19+
20+
public Node selectSingleNode(String xpathExpression);
21+
22+
public List selectNodes(String xpathExpression);
1723
}
1824

1925
/*

‎java/ql/test/stubs/dom4j-2.1.1/org/dom4j/Node.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/*
2+
* Copyright 2001-2005 (C) MetaStuff, Ltd. All Rights Reserved.
3+
*
4+
* This software is open source.
5+
* See the bottom of this file for the licence.
6+
*/
7+
8+
/*
9+
* Adapted from DOM4J version 2.1.1 as available at
10+
* https://search.maven.org/remotecontent?filepath=org/dom4j/dom4j/2.1.1/dom4j-2.1.1-sources.jar
11+
* Only relevant stubs of this file have been retained for test purposes.
12+
*/
13+
package org.dom4j;
14+
15+
public interface Node {
16+
17+
public boolean hasContent();
18+
}
19+
20+
/*
21+
* Copyright 2001-2005 (C) MetaStuff, Ltd. All Rights Reserved.
22+
*
23+
* This software is open source. See the bottom of this file for the licence.
24+
*/
25+
26+
/*
27+
* Adapted from DOM4J version 2.1.1 as available at
28+
* https://search.maven.org/remotecontent?filepath=org/dom4j/dom4j/2.1.1/dom4j-2
29+
* .1.1-sources.jar Only relevant stubs of this file have been retained for test
30+
* purposes.
31+
*/

0 commit comments

Comments
 (0)