Skip to content

Commit 8783746

Browse files
authored
Merge pull request github#5774 from atorralba/promote-xpath-injection
Java: Promote XPath Injection query from experimental
2 parents 7a75864 + 2a50195 commit 8783746

30 files changed

+1928
-25
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "XPath injection" (`java/xml/xpath-injection`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @SpaceWhite](https://github.com/github/codeql/pull/2800)

java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.java renamed to java/ql/src/Security/CWE/CWE-643/XPathInjection.java

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,19 @@
5858
// Bad Dom4j
5959
org.dom4j.io.SAXReader reader = new org.dom4j.io.SAXReader();
6060
org.dom4j.Document document = reader.read(new InputSource(new StringReader(xmlStr)));
61-
isExist = document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']").hasContent();
61+
isExist = document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']") != null;
6262
// or document.selectNodes
6363
System.out.println(isExist);
64+
65+
// Good Dom4j
66+
org.jaxen.SimpleVariableContext svc = new org.jaxen.SimpleVariableContext();
67+
svc.setVariableValue("user", user);
68+
svc.setVariableValue("pass", pass);
69+
String xpathString = "/users/user[@name=$user and @pass=$pass]";
70+
org.dom4j.XPath safeXPath = document.createXPath(xpathString);
71+
safeXPath.setVariableContext(svc);
72+
isExist = safeXPath.selectSingleNode(document) != null;
73+
System.out.println(isExist);
6474
}
6575
} catch (ParserConfigurationException e) {
6676

java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.qhelp renamed to java/ql/src/Security/CWE/CWE-643/XPathInjection.qhelp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@
55
<overview>
66
<p>
77
If an XPath expression is built using string concatenation, and the components of the concatenation
8-
include user input, a user is likely to be able to create a malicious XPath expression.
8+
include user input, it makes it very easy for a user to create a malicious XPath expression.
99
</p>
1010
</overview>
1111

1212
<recommendation>
1313
<p>
14-
If user input must be included in an XPath expression, pre-compile the query and use variable
15-
references to include the user input.
14+
If user input must be included in an XPath expression, either sanitize the data or pre-compile the query
15+
and use variable references to include the user input.
1616
</p>
1717
<p>
1818
XPath injection can also be prevented by using XQuery.
@@ -22,23 +22,23 @@ XPath injection can also be prevented by using XQuery.
2222

2323
<example>
2424
<p>
25-
In the first, second, and third example, the code accepts a name and password specified by the user, and uses this
25+
In the first three examples, the code accepts a name and password specified by the user, and uses this
2626
unvalidated and unsanitized value in an XPath expression. This is vulnerable to the user providing
2727
special characters or string sequences that change the meaning of the XPath expression to search
2828
for different values.
2929
</p>
3030

3131
<p>
32-
In the fourth example, the code utilizes setXPathVariableResolver which prevents XPath Injection.
32+
In the fourth example, the code uses <code>setXPathVariableResolver</code> which prevents XPath injection.
3333
</p>
3434
<p>
35-
The fifth example is a dom4j XPath injection example.
35+
The final two examples are for dom4j. They show an example of XPath injection and one method of preventing it.
3636
</p>
3737
<sample src="XPathInjection.java" />
3838
</example>
3939

4040
<references>
4141
<li>OWASP: <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/07-Input_Validation_Testing/09-Testing_for_XPath_Injection">Testing for XPath Injection</a>.</li>
42-
<li>OWASP: <a href="https://www.owasp.org/index.php/XPATH_Injection">XPath Injection</a>.</li>
42+
<li>OWASP: <a href="https://owasp.org/www-community/attacks/XPATH_Injection">XPath Injection</a>.</li>
4343
</references>
4444
</qhelp>

java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql renamed to java/ql/src/Security/CWE/CWE-643/XPathInjection.ql

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import java
1414
import semmle.code.java.dataflow.FlowSources
1515
import semmle.code.java.dataflow.TaintTracking
16-
import semmle.code.java.security.XmlParsers
16+
import semmle.code.java.security.XPath
1717
import DataFlow::PathGraph
1818

1919
class XPathInjectionConfiguration extends TaintTracking::Configuration {
@@ -24,20 +24,6 @@ class XPathInjectionConfiguration extends TaintTracking::Configuration {
2424
override predicate isSink(DataFlow::Node sink) { sink instanceof XPathInjectionSink }
2525
}
2626

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-
}
40-
4127
from DataFlow::PathNode source, DataFlow::PathNode sink, XPathInjectionConfiguration c
4228
where c.hasFlowPath(source, sink)
4329
select sink.getNode(), source, sink, "$@ flows to here and is used in an XPath expression.",

java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ private module Frameworks {
8080
private import semmle.code.java.security.ResponseSplitting
8181
private import semmle.code.java.security.XSS
8282
private import semmle.code.java.security.LdapInjection
83+
private import semmle.code.java.security.XPath
8384
}
8485

8586
private predicate sourceModelCsv(string row) {
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/** Provides classes to reason about XPath vulnerabilities. */
2+
3+
import java
4+
import semmle.code.java.dataflow.DataFlow
5+
import semmle.code.java.dataflow.ExternalFlow
6+
7+
/**
8+
* A sink that represents a method that interprets XPath expressions.
9+
* Extend this class to add your own XPath Injection sinks.
10+
*/
11+
abstract class XPathInjectionSink extends DataFlow::Node { }
12+
13+
/** CSV sink models representing methods susceptible to XPath Injection attacks. */
14+
private class DefaultXPathInjectionSinkModel extends SinkModelCsv {
15+
override predicate row(string row) {
16+
row =
17+
[
18+
"javax.xml.xpath;XPath;true;evaluate;;;Argument[0];xpath",
19+
"javax.xml.xpath;XPath;true;evaluateExpression;;;Argument[0];xpath",
20+
"javax.xml.xpath;XPath;true;compile;;;Argument[0];xpath",
21+
"org.dom4j;Node;true;selectObject;;;Argument[0];xpath",
22+
"org.dom4j;Node;true;selectNodes;;;Argument[0..1];xpath",
23+
"org.dom4j;Node;true;selectSingleNode;;;Argument[0];xpath",
24+
"org.dom4j;Node;true;numberValueOf;;;Argument[0];xpath",
25+
"org.dom4j;Node;true;valueOf;;;Argument[0];xpath",
26+
"org.dom4j;Node;true;matches;;;Argument[0];xpath",
27+
"org.dom4j;Node;true;createXPath;;;Argument[0];xpath",
28+
"org.dom4j;DocumentFactory;true;createPattern;;;Argument[0];xpath",
29+
"org.dom4j;DocumentFactory;true;createXPath;;;Argument[0];xpath",
30+
"org.dom4j;DocumentFactory;true;createXPathFilter;;;Argument[0];xpath",
31+
"org.dom4j;DocumentHelper;false;createPattern;;;Argument[0];xpath",
32+
"org.dom4j;DocumentHelper;false;createXPath;;;Argument[0];xpath",
33+
"org.dom4j;DocumentHelper;false;createXPathFilter;;;Argument[0];xpath",
34+
"org.dom4j;DocumentHelper;false;selectNodes;;;Argument[0];xpath",
35+
"org.dom4j;DocumentHelper;false;sort;;;Argument[1];xpath",
36+
"org.dom4j.tree;AbstractNode;true;createXPathFilter;;;Argument[0];xpath",
37+
"org.dom4j.tree;AbstractNode;true;createPattern;;;Argument[0];xpath",
38+
"org.dom4j.util;ProxyDocumentFactory;true;createPattern;;;Argument[0];xpath",
39+
"org.dom4j.util;ProxyDocumentFactory;true;createXPath;;;Argument[0];xpath",
40+
"org.dom4j.util;ProxyDocumentFactory;true;createXPathFilter;;;Argument[0];xpath"
41+
]
42+
}
43+
}
44+
45+
/** A default sink representing methods susceptible to XPath Injection attacks. */
46+
private class DefaultXPathInjectionSink extends XPathInjectionSink {
47+
DefaultXPathInjectionSink() {
48+
sinkNode(this, "xpath")
49+
or
50+
exists(ClassInstanceExpr constructor |
51+
constructor.getConstructedType().getASourceSupertype*().hasQualifiedName("org.dom4j", "XPath")
52+
or
53+
constructor.getConstructedType().hasQualifiedName("org.dom4j.xpath", "XPathPattern")
54+
|
55+
this.asExpr() = constructor.getArgument(0)
56+
)
57+
}
58+
}
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/jdom-1.1.3:${testdir}/../../../stubs/dom4j-2.1.1:${testdir}/../../../stubs/simple-xml-2.7.1:${testdir}/../../../stubs/jaxb-api-2.3.1
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/jdom-1.1.3:${testdir}/../../../stubs/dom4j-2.1.1:${testdir}/../../../stubs/simple-xml-2.7.1:${testdir}/../../../stubs/jaxb-api-2.3.1:${testdir}/../../../stubs/jaxen-1.2.0

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

Whitespace-only changes.
Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
import java.io.ByteArrayInputStream;
2+
import java.io.StringReader;
3+
import java.util.ArrayList;
4+
5+
import javax.servlet.http.HttpServletRequest;
6+
import javax.xml.namespace.QName;
7+
import javax.xml.parsers.DocumentBuilder;
8+
import javax.xml.parsers.DocumentBuilderFactory;
9+
import javax.xml.xpath.XPath;
10+
import javax.xml.xpath.XPathConstants;
11+
import javax.xml.xpath.XPathExpression;
12+
import javax.xml.xpath.XPathExpressionException;
13+
import javax.xml.xpath.XPathFactory;
14+
15+
import org.jaxen.pattern.Pattern;
16+
import org.dom4j.DocumentFactory;
17+
import org.dom4j.DocumentHelper;
18+
import org.dom4j.Namespace;
19+
import org.dom4j.Node;
20+
import org.dom4j.io.SAXReader;
21+
import org.dom4j.util.ProxyDocumentFactory;
22+
import org.dom4j.xpath.DefaultXPath;
23+
import org.dom4j.xpath.XPathPattern;
24+
import org.w3c.dom.Document;
25+
import org.xml.sax.InputSource;
26+
27+
public class XPathInjectionTest {
28+
private static abstract class XPathImplStub implements XPath {
29+
public static XPathImplStub getInstance() {
30+
return null;
31+
}
32+
33+
@Override
34+
public XPathExpression compile(String expression) throws XPathExpressionException {
35+
return null;
36+
}
37+
38+
@Override
39+
public Object evaluate(String expression, Object item, QName returnType) throws XPathExpressionException {
40+
return null;
41+
}
42+
43+
@Override
44+
public String evaluate(String expression, Object item) throws XPathExpressionException {
45+
return null;
46+
}
47+
48+
@Override
49+
public Object evaluate(String expression, InputSource source, QName returnType)
50+
throws XPathExpressionException {
51+
return null;
52+
}
53+
54+
@Override
55+
public String evaluate(String expression, InputSource source) throws XPathExpressionException {
56+
return null;
57+
}
58+
59+
}
60+
61+
private static class ProxyDocumentFactoryStub extends ProxyDocumentFactory {
62+
}
63+
64+
private static class PatternStub extends Pattern {
65+
private String text;
66+
67+
PatternStub(String text) {
68+
this.text = text;
69+
}
70+
71+
public String getText() {
72+
return text;
73+
}
74+
}
75+
76+
public void handle(HttpServletRequest request) throws Exception {
77+
String user = request.getParameter("user");
78+
String pass = request.getParameter("pass");
79+
String expression = "/users/user[@name='" + user + "' and @pass='" + pass + "']";
80+
81+
final String xmlStr = "<users>" + " <user name=\"aaa\" pass=\"pass1\"></user>"
82+
+ " <user name=\"bbb\" pass=\"pass2\"></user>" + "</users>";
83+
DocumentBuilderFactory domFactory = DocumentBuilderFactory.newInstance();
84+
DocumentBuilder builder = domFactory.newDocumentBuilder();
85+
InputSource xmlSource = new InputSource(new StringReader(xmlStr));
86+
Document doc = builder.parse(xmlSource);
87+
88+
XPathFactory factory = XPathFactory.newInstance();
89+
XPath xpath = factory.newXPath();
90+
91+
xpath.evaluate(expression, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
92+
xpath.evaluateExpression(expression, xmlSource); // $hasXPathInjection
93+
xpath.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
94+
95+
XPathImplStub xpathStub = XPathImplStub.getInstance();
96+
xpathStub.evaluate(expression, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
97+
xpathStub.evaluateExpression(expression, xmlSource); // $hasXPathInjection
98+
xpathStub.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
99+
100+
StringBuffer sb = new StringBuffer("/users/user[@name=");
101+
sb.append(user);
102+
sb.append("' and @pass='");
103+
sb.append(pass);
104+
sb.append("']");
105+
String query = sb.toString();
106+
107+
xpath.compile(query); // $hasXPathInjection
108+
xpathStub.compile(query); // $hasXPathInjection
109+
110+
String expression4 = "/users/user[@name=$user and @pass=$pass]";
111+
xpath.setXPathVariableResolver(v -> {
112+
switch (v.getLocalPart()) {
113+
case "user":
114+
return user;
115+
case "pass":
116+
return pass;
117+
default:
118+
throw new IllegalArgumentException();
119+
}
120+
});
121+
xpath.evaluate(expression4, doc, XPathConstants.BOOLEAN); // Safe
122+
123+
SAXReader reader = new SAXReader();
124+
org.dom4j.Document document = reader.read(new ByteArrayInputStream(xmlStr.getBytes()));
125+
document.selectObject("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
126+
document.selectNodes("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
127+
document.selectNodes("/users/user[@name='test']", "/users/user[@pass='" + pass + "']"); // $hasXPathInjection
128+
document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
129+
document.valueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
130+
document.numberValueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
131+
document.matches("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
132+
document.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
133+
134+
new DefaultXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
135+
new XPathPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
136+
new XPathPattern(new PatternStub(user)); // $ MISSING: hasXPathInjection // Jaxen is not modeled yet
137+
138+
DocumentFactory docFactory = DocumentFactory.getInstance();
139+
docFactory.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
140+
docFactory.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
141+
docFactory.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
142+
143+
DocumentHelper.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
144+
DocumentHelper.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
145+
DocumentHelper.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
146+
DocumentHelper.selectNodes("/users/user[@name='" + user + "' and @pass='" + pass + "']", new ArrayList<Node>()); // $hasXPathInjection
147+
DocumentHelper.sort(new ArrayList<Node>(), "/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
148+
149+
ProxyDocumentFactoryStub proxyDocFactory = new ProxyDocumentFactoryStub();
150+
proxyDocFactory.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
151+
proxyDocFactory.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
152+
proxyDocFactory.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
153+
154+
Namespace namespace = new Namespace("prefix", "http://some.uri.io");
155+
namespace.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
156+
namespace.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
157+
158+
org.jaxen.SimpleVariableContext svc = new org.jaxen.SimpleVariableContext();
159+
svc.setVariableValue("user", user);
160+
svc.setVariableValue("pass", pass);
161+
String xpathString = "/users/user[@name=$user and @pass=$pass]";
162+
org.dom4j.XPath safeXPath = document.createXPath(xpathString); // Safe
163+
safeXPath.setVariableContext(svc);
164+
safeXPath.selectSingleNode(document); // Safe
165+
}
166+
}
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
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 Conf extends TaintTracking::Configuration {
8+
Conf() { this = "test:xml:xpathinjection" }
9+
10+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
11+
12+
override predicate isSink(DataFlow::Node sink) { sink instanceof XPathInjectionSink }
13+
}
14+
15+
class HasXPathInjectionTest extends InlineExpectationsTest {
16+
HasXPathInjectionTest() { this = "HasXPathInjectionTest" }
17+
18+
override string getARelevantTag() { result = "hasXPathInjection" }
19+
20+
override predicate hasActualResult(Location location, string element, string tag, string value) {
21+
tag = "hasXPathInjection" and
22+
exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) |
23+
sink.getLocation() = location and
24+
element = sink.toString() and
25+
value = ""
26+
)
27+
}
28+
}

0 commit comments

Comments
 (0)