Skip to content

Commit 720b5d6

Browse files
atorralbaatorralba
committed
Refactored sto use CSV sink model. Also, added more sinks
1 parent ab62bb6 commit 720b5d6

File tree

17 files changed

+1304
-106
lines changed

17 files changed

+1304
-106
lines changed

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

Lines changed: 41 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -3,62 +3,55 @@
33
import java
44
import semmle.code.java.dataflow.FlowSources
55
import semmle.code.java.dataflow.TaintTracking
6-
7-
/** The interface `javax.xml.xpath.XPath` */
8-
private class XPath extends Interface {
9-
XPath() { this.hasQualifiedName("javax.xml.xpath", "XPath") }
10-
}
11-
12-
/** A call to methods of any class implementing the interface `XPath` that evaluate XPath expressions */
13-
private class XPathEvaluation extends MethodAccess {
14-
XPathEvaluation() {
15-
exists(Method m |
16-
this.getMethod() = m and m.getDeclaringType().getASourceSupertype*() instanceof XPath
17-
|
18-
m.hasName(["evaluate", "evaluateExpression", "compile"])
19-
)
20-
}
21-
22-
Expr getSink() { result = this.getArgument(0) }
23-
}
24-
25-
/** The interface `org.dom4j.Node` */
26-
private class Dom4JNode extends Interface {
27-
Dom4JNode() { this.hasQualifiedName("org.dom4j", "Node") }
28-
}
29-
30-
/** A call to methods of any class implementing the interface `Node` that evaluate XPath expressions */
31-
private class NodeXPathEvaluation extends MethodAccess {
32-
Expr sink;
33-
34-
NodeXPathEvaluation() {
35-
exists(Method m, int index |
36-
this.getMethod() = m and
37-
m.getDeclaringType().getASourceSupertype*() instanceof Dom4JNode and
38-
sink = this.getArgument(index)
39-
|
40-
m.hasName([
41-
"selectObject", "selectNodes", "selectSingleNode", "numberValueOf", "valueOf", "matches",
42-
"createXPath"
43-
]) and
44-
index = 0
45-
or
46-
m.hasName("selectNodes") and index in [0, 1]
47-
)
48-
}
49-
50-
Expr getSink() { result = sink }
51-
}
6+
import semmle.code.java.dataflow.ExternalFlow
527

538
/**
549
* A sink that represents a method that interprets XPath expressions.
5510
* Extend this class to add your own XPath Injection sinks.
5611
*/
5712
abstract class XPathInjectionSink extends DataFlow::Node { }
5813

14+
/** CSV sink models representing methods susceptible to XPath Injection attacks. */
15+
private class DefaultXPathInjectionSinkModel extends SinkModelCsv {
16+
override predicate row(string row) {
17+
row =
18+
[
19+
"javax.xml.xpath;XPath;true;evaluate;;;Argument[0];xpath",
20+
"javax.xml.xpath;XPath;true;evaluateExpression;;;Argument[0];xpath",
21+
"javax.xml.xpath;XPath;true;compile;;;Argument[0];xpath",
22+
"org.dom4j;Node;true;selectObject;;;Argument[0];xpath",
23+
"org.dom4j;Node;true;selectNodes;;;Argument[0..1];xpath",
24+
"org.dom4j;Node;true;selectSingleNode;;;Argument[0];xpath",
25+
"org.dom4j;Node;true;numberValueOf;;;Argument[0];xpath",
26+
"org.dom4j;Node;true;valueOf;;;Argument[0];xpath",
27+
"org.dom4j;Node;true;matches;;;Argument[0];xpath",
28+
"org.dom4j;Node;true;createXPath;;;Argument[0];xpath",
29+
"org.dom4j;DocumentFactory;true;createPattern;;;Argument[0];xpath",
30+
"org.dom4j;DocumentFactory;true;createXPath;;;Argument[0];xpath",
31+
"org.dom4j;DocumentFactory;true;createXPathFilter;;;Argument[0];xpath",
32+
"org.dom4j;DocumentHelper;false;createPattern;;;Argument[0];xpath",
33+
"org.dom4j;DocumentHelper;false;createXPath;;;Argument[0];xpath",
34+
"org.dom4j;DocumentHelper;false;createXPathFilter;;;Argument[0];xpath",
35+
"org.dom4j.tree;AbstractNode;true;createXPathFilter;;;Argument[0];xpath",
36+
"org.dom4j.tree;AbstractNode;true;createPattern;;;Argument[0];xpath",
37+
"org.dom4j.util;ProxyDocumentFactory;true;createPattern;;;Argument[0];xpath",
38+
"org.dom4j.util;ProxyDocumentFactory;true;createXPath;;;Argument[0];xpath",
39+
"org.dom4j.util;ProxyDocumentFactory;true;createXPathFilter;;;Argument[0];xpath"
40+
]
41+
}
42+
}
43+
44+
/** A default sink representing methods susceptible to XPath Injection attacks. */
5945
private class DefaultXPathInjectionSink extends XPathInjectionSink {
6046
DefaultXPathInjectionSink() {
61-
exists(NodeXPathEvaluation sink | sink.getSink() = this.asExpr()) or
62-
exists(XPathEvaluation sink | sink.getSink() = this.asExpr())
47+
sinkNode(this, "xpath")
48+
or
49+
exists(ClassInstanceExpr constructor |
50+
constructor.getConstructedType().getASourceSupertype*().hasQualifiedName("org.dom4j", "XPath")
51+
or
52+
constructor.getConstructedType().hasQualifiedName("org.dom4j.xpath", "XPathPattern")
53+
|
54+
this.asExpr() = constructor.getArgument(0)
55+
)
6356
}
6457
}

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

Lines changed: 77 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@
1111
import javax.xml.xpath.XPathExpressionException;
1212
import javax.xml.xpath.XPathFactory;
1313

14+
import org.dom4j.DocumentFactory;
15+
import org.dom4j.DocumentHelper;
16+
import org.dom4j.Namespace;
17+
import org.dom4j.io.SAXReader;
18+
import org.dom4j.util.ProxyDocumentFactory;
19+
import org.dom4j.xpath.DefaultXPath;
20+
import org.dom4j.xpath.XPathPattern;
1421
import org.w3c.dom.Document;
1522
import org.xml.sax.InputSource;
1623

@@ -48,71 +55,86 @@ public String evaluate(String expression, InputSource source) throws XPathExpres
4855

4956
}
5057

58+
private static class ProxyDocumentFactoryStub extends ProxyDocumentFactory {
59+
}
60+
5161
public void handle(HttpServletRequest request) throws Exception {
62+
String user = request.getParameter("user");
63+
String pass = request.getParameter("pass");
64+
String expression = "/users/user[@name='" + user + "' and @pass='" + pass + "']";
65+
5266
final String xmlStr = "<users>" + " <user name=\"aaa\" pass=\"pass1\"></user>"
5367
+ " <user name=\"bbb\" pass=\"pass2\"></user>" + "</users>";
5468
DocumentBuilderFactory domFactory = DocumentBuilderFactory.newInstance();
55-
domFactory.setNamespaceAware(true);
5669
DocumentBuilder builder = domFactory.newDocumentBuilder();
5770
InputSource xmlSource = new InputSource(new StringReader(xmlStr));
5871
Document doc = builder.parse(xmlSource);
5972

6073
XPathFactory factory = XPathFactory.newInstance();
6174
XPath xpath = factory.newXPath();
62-
XPathImplStub xpathStub = XPathImplStub.getInstance();
6375

64-
// Injectable data
65-
String user = request.getParameter("user");
66-
String pass = request.getParameter("pass");
67-
if (user != null && pass != null) {
68-
// Bad expression
69-
String expression1 = "/users/user[@name='" + user + "' and @pass='" + pass + "']";
70-
xpath.evaluate(expression1, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
71-
xpathStub.evaluate(expression1, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
72-
xpath.evaluateExpression(expression1, xmlSource); // $hasXPathInjection
73-
xpathStub.evaluateExpression(expression1, xmlSource); // $hasXPathInjection
74-
75-
// Bad expression
76-
XPathExpression expression2 = xpath.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
77-
xpathStub.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
78-
expression2.evaluate(doc, XPathConstants.BOOLEAN);
79-
80-
// Bad expression
81-
StringBuffer sb = new StringBuffer("/users/user[@name=");
82-
sb.append(user);
83-
sb.append("' and @pass='");
84-
sb.append(pass);
85-
sb.append("']");
86-
String query = sb.toString();
87-
XPathExpression expression3 = xpath.compile(query); // $hasXPathInjection
88-
xpathStub.compile(query); // $hasXPathInjection
89-
expression3.evaluate(doc, XPathConstants.BOOLEAN);
90-
91-
// Good expression
92-
String expression4 = "/users/user[@name=$user and @pass=$pass]";
93-
xpath.setXPathVariableResolver(v -> {
94-
switch (v.getLocalPart()) {
95-
case "user":
96-
return user;
97-
case "pass":
98-
return pass;
99-
default:
100-
throw new IllegalArgumentException();
101-
}
102-
});
103-
xpath.evaluate(expression4, doc, XPathConstants.BOOLEAN); // Safe
104-
105-
// Bad Dom4j
106-
org.dom4j.io.SAXReader reader = new org.dom4j.io.SAXReader();
107-
org.dom4j.Document document = reader.read(new ByteArrayInputStream(xmlStr.getBytes()));
108-
document.selectObject("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
109-
document.selectNodes("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
110-
document.selectNodes("/users/user[@name='test']", "/users/user[@pass='" + pass + "']"); // $hasXPathInjection
111-
document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
112-
document.valueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
113-
document.numberValueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
114-
document.matches("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
115-
document.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
116-
}
76+
xpath.evaluate(expression, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
77+
xpath.evaluateExpression(expression, xmlSource); // $hasXPathInjection
78+
xpath.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
79+
80+
XPathImplStub xpathStub = XPathImplStub.getInstance();
81+
xpathStub.evaluate(expression, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
82+
xpathStub.evaluateExpression(expression, xmlSource); // $hasXPathInjection
83+
xpathStub.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
84+
85+
StringBuffer sb = new StringBuffer("/users/user[@name=");
86+
sb.append(user);
87+
sb.append("' and @pass='");
88+
sb.append(pass);
89+
sb.append("']");
90+
String query = sb.toString();
91+
92+
xpath.compile(query); // $hasXPathInjection
93+
xpathStub.compile(query); // $hasXPathInjection
94+
95+
String expression4 = "/users/user[@name=$user and @pass=$pass]";
96+
xpath.setXPathVariableResolver(v -> {
97+
switch (v.getLocalPart()) {
98+
case "user":
99+
return user;
100+
case "pass":
101+
return pass;
102+
default:
103+
throw new IllegalArgumentException();
104+
}
105+
});
106+
xpath.evaluate(expression4, doc, XPathConstants.BOOLEAN); // Safe
107+
108+
SAXReader reader = new SAXReader();
109+
org.dom4j.Document document = reader.read(new ByteArrayInputStream(xmlStr.getBytes()));
110+
document.selectObject("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
111+
document.selectNodes("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
112+
document.selectNodes("/users/user[@name='test']", "/users/user[@pass='" + pass + "']"); // $hasXPathInjection
113+
document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
114+
document.valueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
115+
document.numberValueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
116+
document.matches("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
117+
document.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
118+
119+
new DefaultXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
120+
new XPathPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
121+
122+
DocumentFactory docFactory = DocumentFactory.getInstance();
123+
docFactory.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
124+
docFactory.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
125+
docFactory.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
126+
127+
DocumentHelper.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
128+
DocumentHelper.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
129+
DocumentHelper.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
130+
131+
ProxyDocumentFactoryStub proxyDocFactory = new ProxyDocumentFactoryStub();
132+
proxyDocFactory.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
133+
proxyDocFactory.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
134+
proxyDocFactory.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
135+
136+
Namespace namespace = new Namespace("prefix", "http://some.uri.io");
137+
namespace.createPattern("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
138+
namespace.createXPathFilter("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
117139
}
118140
}

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

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/dom4j-2.1.1:${testdir}/../../../../stubs/servlet-api-2.4
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/dom4j-2.1.1:${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jaxen-1.2.0

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

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
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+
package org.dom4j;
9+
10+
import java.io.Serializable;
11+
import java.util.Map;
12+
13+
import org.dom4j.rule.Pattern;
14+
import org.jaxen.VariableContext;
15+
16+
public class DocumentFactory implements Serializable {
17+
public DocumentFactory() {
18+
}
19+
20+
public static synchronized DocumentFactory getInstance() {
21+
return null;
22+
}
23+
24+
public Document createDocument() {
25+
return null;
26+
}
27+
28+
public Document createDocument(String encoding) {
29+
return null;
30+
}
31+
32+
public Document createDocument(Element rootElement) {
33+
return null;
34+
}
35+
36+
public Element createElement(String name) {
37+
return null;
38+
}
39+
40+
public Element createElement(String qualifiedName, String namespaceURI) {
41+
return null;
42+
}
43+
44+
public Namespace createNamespace(String prefix, String uri) {
45+
return null;
46+
}
47+
48+
public XPath createXPath(String xpathExpression) throws InvalidXPathException {
49+
return null;
50+
}
51+
52+
public XPath createXPath(String xpathExpression, VariableContext variableContext) {
53+
return null;
54+
}
55+
56+
public NodeFilter createXPathFilter(String xpathFilterExpression, VariableContext variableContext) {
57+
return null;
58+
}
59+
60+
public NodeFilter createXPathFilter(String xpathFilterExpression) {
61+
return null;
62+
}
63+
64+
public Pattern createPattern(String xpathPattern) {
65+
return null;
66+
}
67+
68+
public Map<String, String> getXPathNamespaceURIs() {
69+
return null;
70+
}
71+
72+
public void setXPathNamespaceURIs(Map<String, String> namespaceURIs) {
73+
}
74+
75+
}

0 commit comments

Comments
 (0)