Support more methods that evaluate XPath expressions

atorralba · atorralba · commit 2bb2baf6f70e · 2021-05-06T09:18:49.000+02:00
diff --git a/java/ql/src/semmle/code/java/security/XPath.qll b/java/ql/src/semmle/code/java/security/XPath.qll
@@ -9,13 +9,13 @@ private class XPath extends RefType {
   XPath() { this.hasQualifiedName("javax.xml.xpath", "XPath") }
 }
 
-/** A call to `XPath.evaluate` or `XPath.compile` */
-private class XPathEvaluateOrCompile extends MethodAccess {
-  XPathEvaluateOrCompile() {
+/** A call to methods of any class implementing the interface `XPath` that evaluate XPath expressions */
+private class XPathEvaluation extends MethodAccess {
+  XPathEvaluation() {
     exists(Method m |
-      this.getMethod() = m and m.getDeclaringType() instanceof XPath
+      this.getMethod() = m and m.getDeclaringType().getASourceSupertype*() instanceof XPath
     |
-      m.hasName(["evaluate", "compile"])
+      m.hasName(["evaluate", "evaluateExpression", "compile"])
     )
   }
 }
@@ -25,13 +25,16 @@ private class Dom4JNode extends Interface {
   Dom4JNode() { this.hasQualifiedName("org.dom4j", "Node") }
 }
 
-/** A call to `Node.selectNodes` or `Node.selectSingleNode` */
-private class NodeSelectNodes extends MethodAccess {
-  NodeSelectNodes() {
+/** A call to methods of any class implementing the interface `Node` that evaluate XPath expressions */
+private class NodeXPathEvaluation extends MethodAccess {
+  NodeXPathEvaluation() {
     exists(Method m |
       this.getMethod() = m and m.getDeclaringType().getASourceSupertype*() instanceof Dom4JNode
     |
-      m.hasName(["selectNodes", "selectSingleNode"])
+      m.hasName([
+          "selectObject", "selectNodes", "selectSingleNode", "numberValueOf", "valueOf", "matches",
+          "createXPath"
+        ])
     )
   }
 }
@@ -44,7 +47,7 @@ abstract class XPathInjectionSink extends DataFlow::Node { }
 
 private class DefaultXPathInjectionSink extends XPathInjectionSink {
   DefaultXPathInjectionSink() {
-    exists(NodeSelectNodes sink | sink.getArgument(0) = this.asExpr()) or
-    exists(XPathEvaluateOrCompile sink | sink.getArgument(0) = this.asExpr())
+    exists(NodeXPathEvaluation sink | sink.getArgument(0) = this.asExpr()) or
+    exists(XPathEvaluation sink | sink.getArgument(0) = this.asExpr())
   }
 }
diff --git a/java/ql/test/experimental/query-tests/security/CWE-643/A.java b/java/ql/test/experimental/query-tests/security/CWE-643/A.java
@@ -2,27 +2,64 @@
 import java.io.StringReader;
 
 import javax.servlet.http.HttpServletRequest;
+import javax.xml.namespace.QName;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathConstants;
 import javax.xml.xpath.XPathExpression;
+import javax.xml.xpath.XPathExpressionException;
 import javax.xml.xpath.XPathFactory;
 
 import org.w3c.dom.Document;
 import org.xml.sax.InputSource;
 
 public class A {
+    private static abstract class XPathImplStub implements XPath {
+        public static XPathImplStub getInstance() {
+            return null;
+        }
+
+        @Override
+        public XPathExpression compile(String expression) throws XPathExpressionException {
+            return null;
+        }
+
+        @Override
+        public Object evaluate(String expression, Object item, QName returnType) throws XPathExpressionException {
+            return null;
+        }
+
+        @Override
+        public String evaluate(String expression, Object item) throws XPathExpressionException {
+            return null;
+        }
+
+        @Override
+        public Object evaluate(String expression, InputSource source, QName returnType)
+                throws XPathExpressionException {
+            return null;
+        }
+
+        @Override
+        public String evaluate(String expression, InputSource source) throws XPathExpressionException {
+            return null;
+        }
+
+    }
+
     public void handle(HttpServletRequest request) throws Exception {
         final String xmlStr = "<users>" + "   <user name=\"aaa\" pass=\"pass1\"></user>"
                 + "   <user name=\"bbb\" pass=\"pass2\"></user>" + "</users>";
         DocumentBuilderFactory domFactory = DocumentBuilderFactory.newInstance();
         domFactory.setNamespaceAware(true);
         DocumentBuilder builder = domFactory.newDocumentBuilder();
-        Document doc = builder.parse(new InputSource(new StringReader(xmlStr)));
+        InputSource xmlSource = new InputSource(new StringReader(xmlStr));
+        Document doc = builder.parse(xmlSource);
 
         XPathFactory factory = XPathFactory.newInstance();
         XPath xpath = factory.newXPath();
+        XPathImplStub xpathStub = XPathImplStub.getInstance();
 
         // Injectable data
         String user = request.getParameter("user");
@@ -31,9 +68,13 @@ public void handle(HttpServletRequest request) throws Exception {
             // Bad expression
             String expression1 = "/users/user[@name='" + user + "' and @pass='" + pass + "']";
             xpath.evaluate(expression1, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
+            xpathStub.evaluate(expression1, doc, XPathConstants.BOOLEAN); // $hasXPathInjection
+            xpath.evaluateExpression(expression1, xmlSource); // $hasXPathInjection
+            xpathStub.evaluateExpression(expression1, xmlSource); // $hasXPathInjection
 
             // Bad expression
             XPathExpression expression2 = xpath.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
+            xpathStub.compile("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
             expression2.evaluate(doc, XPathConstants.BOOLEAN);
 
             // Bad expression
@@ -44,6 +85,7 @@ public void handle(HttpServletRequest request) throws Exception {
             sb.append("']");
             String query = sb.toString();
             XPathExpression expression3 = xpath.compile(query); // $hasXPathInjection
+            xpathStub.compile(query); // $hasXPathInjection
             expression3.evaluate(doc, XPathConstants.BOOLEAN);
 
             // Good expression
@@ -63,9 +105,14 @@ public void handle(HttpServletRequest request) throws Exception {
             // Bad Dom4j
             org.dom4j.io.SAXReader reader = new org.dom4j.io.SAXReader();
             org.dom4j.Document document = reader.read(new ByteArrayInputStream(xmlStr.getBytes()));
-            document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']") // $hasXPathInjection
-                    .hasContent();
+            document.selectObject("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
             document.selectNodes("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
+            document.selectNodes("/users/user[@name='test']", "/users/user[@pass='" + pass + "']"); // Safe-ish
+            document.selectSingleNode("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
+            document.valueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
+            document.numberValueOf("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
+            document.matches("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
+            document.createXPath("/users/user[@name='" + user + "' and @pass='" + pass + "']"); // $hasXPathInjection
         }
     }
 }
diff --git a/java/ql/test/stubs/dom4j-2.1.1/org/dom4j/Document.java b/java/ql/test/stubs/dom4j-2.1.1/org/dom4j/Document.java
@@ -17,9 +17,6 @@
 
 public interface Document extends Branch {
 
-    public Node selectSingleNode(String xpathExpression);
-
-    public List selectNodes(String xpathExpression);
 }
 
 /*
diff --git a/java/ql/test/stubs/dom4j-2.1.1/org/dom4j/InvalidXPathException.java b/java/ql/test/stubs/dom4j-2.1.1/org/dom4j/InvalidXPathException.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2001-2005 (C) MetaStuff, Ltd. All Rights Reserved.
+ *
+ * This software is open source.
+ * See the bottom of this file for the licence.
+ */
+
+/*
+ * Adapted from DOM4J version 2.1.1 as available at
+ *   https://search.maven.org/remotecontent?filepath=org/dom4j/dom4j/2.1.1/dom4j-2.1.1-sources.jar
+ * Only relevant stubs of this file have been retained for test purposes.
+ */
+
+package org.dom4j;
+
+public class InvalidXPathException extends IllegalArgumentException {
+    public InvalidXPathException(String xpath) {
+    }
+
+    public InvalidXPathException(String xpath, String reason) {
+    }
+
+}
+
+/*
+ * Redistribution and use of this software and associated documentation
+ * ("Software"), with or without modification, are permitted provided that the
+ * following conditions are met:
+ * 
+ * 1. Redistributions of source code must retain copyright statements and
+ * notices. Redistributions must also contain a copy of this document.
+ * 
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ * 
+ * 3. The name "DOM4J" must not be used to endorse or promote products derived
+ * from this Software without prior written permission of MetaStuff, Ltd. For
+ * written permission, please contact dom4j-info@metastuff.com.
+ * 
+ * 4. Products derived from this Software may not be called "DOM4J" nor may
+ * "DOM4J" appear in their names without prior written permission of MetaStuff,
+ * Ltd. DOM4J is a registered trademark of MetaStuff, Ltd.
+ * 
+ * 5. Due credit should be given to the DOM4J Project - http://www.dom4j.org
+ * 
+ * THIS SOFTWARE IS PROVIDED BY METASTUFF, LTD. AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESSED OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL METASTUFF, LTD. OR ITS CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ * 
+ * Copyright 2001-2005 (C) MetaStuff, Ltd. All Rights Reserved.
+ */
diff --git a/java/ql/test/stubs/dom4j-2.1.1/org/dom4j/Node.java b/java/ql/test/stubs/dom4j-2.1.1/org/dom4j/Node.java
@@ -6,26 +6,72 @@
  */
 
 /*
- * Adapted from DOM4J version 2.1.1 as available at
- *   https://search.maven.org/remotecontent?filepath=org/dom4j/dom4j/2.1.1/dom4j-2.1.1-sources.jar
- * Only relevant stubs of this file have been retained for test purposes.
- */
+* Adapted from DOM4J version 2.1.1 as available at
+*   https://search.maven.org/remotecontent?filepath=org/dom4j/dom4j/2.1.1/dom4j-2.1.1-sources.jar
+* Only relevant stubs of this file have been retained for test purposes.
+*/
+
 package org.dom4j;
 
-public interface Node {
+import java.util.List;
+
+import javax.xml.xpath.XPath;
+
+public interface Node extends Cloneable {
+
+    List<Node> selectNodes(String xpathExpression);
+
+    Object selectObject(String xpathExpression);
+
+    List<Node> selectNodes(String xpathExpression, String comparisonXPathExpression);
+
+    List<Node> selectNodes(String xpathExpression, String comparisonXPathExpression, boolean removeDuplicates);
+
+    Node selectSingleNode(String xpathExpression);
+
+    String valueOf(String xpathExpression);
+
+    Number numberValueOf(String xpathExpression);
+
+    boolean matches(String xpathExpression);
+
+    XPath createXPath(String xpathExpression) throws InvalidXPathException;
 
-    public boolean hasContent();
 }
 
 /*
+ * Redistribution and use of this software and associated documentation
+ * ("Software"), with or without modification, are permitted provided that the
+ * following conditions are met:
+ * 
+ * 1. Redistributions of source code must retain copyright statements and
+ * notices. Redistributions must also contain a copy of this document.
+ * 
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ * 
+ * 3. The name "DOM4J" must not be used to endorse or promote products derived
+ * from this Software without prior written permission of MetaStuff, Ltd. For
+ * written permission, please contact dom4j-info@metastuff.com.
+ * 
+ * 4. Products derived from this Software may not be called "DOM4J" nor may
+ * "DOM4J" appear in their names without prior written permission of MetaStuff,
+ * Ltd. DOM4J is a registered trademark of MetaStuff, Ltd.
+ * 
+ * 5. Due credit should be given to the DOM4J Project - http://www.dom4j.org
+ * 
+ * THIS SOFTWARE IS PROVIDED BY METASTUFF, LTD. AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESSED OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL METASTUFF, LTD. OR ITS CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ * 
  * Copyright 2001-2005 (C) MetaStuff, Ltd. All Rights Reserved.
- *
- * This software is open source. See the bottom of this file for the licence.
- */
-
-/*
- * Adapted from DOM4J version 2.1.1 as available at
- * https://search.maven.org/remotecontent?filepath=org/dom4j/dom4j/2.1.1/dom4j-2
- * .1.1-sources.jar Only relevant stubs of this file have been retained for test
- * purposes.
  */

Original file line number	Diff line number	Diff line change
`@@ -9,13 +9,13 @@ private class XPath extends RefType {`
`9`	`9`	`XPath() { this.hasQualifiedName("javax.xml.xpath", "XPath") }`
`10`	`10`	`}`
`11`	`11`
`12`		-/** A call to `XPath.evaluate` or `XPath.compile` */
`13`		`-private class XPathEvaluateOrCompile extends MethodAccess {`
`14`		`- XPathEvaluateOrCompile() {`
	`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`	`15`	`exists(Method m \|`
`16`		`- this.getMethod() = m and m.getDeclaringType() instanceof XPath`
	`16`	`+ this.getMethod() = m and m.getDeclaringType().getASourceSupertype*() instanceof XPath`
`17`	`17`	`\|`
`18`		`- m.hasName(["evaluate", "compile"])`
	`18`	`+ m.hasName(["evaluate", "evaluateExpression", "compile"])`
`19`	`19`	`)`
`20`	`20`	`}`
`21`	`21`	`}`
`@@ -25,13 +25,16 @@ private class Dom4JNode extends Interface {`
`25`	`25`	`Dom4JNode() { this.hasQualifiedName("org.dom4j", "Node") }`
`26`	`26`	`}`
`27`	`27`
`28`		-/** A call to `Node.selectNodes` or `Node.selectSingleNode` */
`29`		`-private class NodeSelectNodes extends MethodAccess {`
`30`		`- NodeSelectNodes() {`
	`28`	+/** A call to methods of any class implementing the interface `Node` that evaluate XPath expressions */
	`29`	`+private class NodeXPathEvaluation extends MethodAccess {`
	`30`	`+ NodeXPathEvaluation() {`
`31`	`31`	`exists(Method m \|`
`32`	`32`	`this.getMethod() = m and m.getDeclaringType().getASourceSupertype*() instanceof Dom4JNode`
`33`	`33`	`\|`
`34`		`- m.hasName(["selectNodes", "selectSingleNode"])`
	`34`	`+ m.hasName([`
	`35`	`+ "selectObject", "selectNodes", "selectSingleNode", "numberValueOf", "valueOf", "matches",`
	`36`	`+ "createXPath"`
	`37`	`+ ])`
`35`	`38`	`)`
`36`	`39`	`}`
`37`	`40`	`}`
`@@ -44,7 +47,7 @@ abstract class XPathInjectionSink extends DataFlow::Node { }`
`44`	`47`
`45`	`48`	`private class DefaultXPathInjectionSink extends XPathInjectionSink {`
`46`	`49`	`DefaultXPathInjectionSink() {`
`47`		`- exists(NodeSelectNodes sink \| sink.getArgument(0) = this.asExpr()) or`
`48`		`- exists(XPathEvaluateOrCompile sink \| sink.getArgument(0) = this.asExpr())`
	`50`	`+ exists(NodeXPathEvaluation sink \| sink.getArgument(0) = this.asExpr()) or`
	`51`	`+ exists(XPathEvaluation sink \| sink.getArgument(0) = this.asExpr())`
`49`	`52`	`}`
`50`	`53`	`}`
Original file line number	Diff line number	Diff line change
`@@ -17,9 +17,6 @@`
`17`	`17`
`18`	`18`	`public interface Document extends Branch {`
`19`	`19`
`20`		`- public Node selectSingleNode(String xpathExpression);`
`21`		`-`
`22`		`- public List selectNodes(String xpathExpression);`
`23`	`20`	`}`
`24`	`21`
`25`	`22`	`/*`