github · max-schaefer · Mar 24, 2020 · Mar 19, 2020 · Mar 20, 2020 · Mar 20, 2020
diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.go b/ql/src/experimental/Security/CWE-643/XPathInjection.go
@@ -0,0 +1,32 @@
+package main
+
+import (
+	"fmt"
+	"net/http"
+
+	"github.com/ChrisTrenkamp/goxpath"
+	"github.com/ChrisTrenkamp/goxpath/tree"
+)
+
+func main() {}
+
+func processRequest(r *http.Request, doc tree.Node) {
+	r.ParseForm()
+	username := r.Form.Get("username")
+	password := r.Form.Get("password")
+
+	// BAD: User input used directly in an XPath expression
+	xPath := goxpath.MustParse("//users/user[login/text()='" + username + "' and password/text() = '" + password + "']/home_dir/text()")
+	unsafeRes, _ := xPath.ExecBool(doc)
+	fmt.Println(unsafeRes)
+
+	// GOOD: Value of parameters is defined here instead of directly in the query
+	opt := func(o *goxpath.Opts) {
+		o.Vars["username"] = tree.String(username)
+		o.Vars["password"] = tree.String(password)
+	}
+	// GOOD: Uses parameters to avoid including user input directly in XPath expression
+	xPath = goxpath.MustParse("//users/user[login/text()=$username and password/text() = $password]/home_dir/text()")
+	safeRes, _ := xPath.ExecBool(doc, opt)
+	fmt.Println(safeRes)
+}
diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp b/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp
@@ -0,0 +1,44 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+If an XPath expression is built using string concatenation, and the components of the concatenation
+include user input, a user is likely to be able to create a malicious XPath expression.
+</p>
+</overview>
+
+<recommendation>
+<p>
+If user input must be included in an XPath expression, pre-compile the query and use variable
+references to include the user input.
+</p>
+<p>
+For example, when using the <code>github.com/ChrisTrenkamp/goxpath</code> API, this can be done by creating a function that takes an <code>*goxpath.Opts</code> structure.
+In this structure you can then set the values of the variable references.
+This function can then be specified when calling <code>Exec()</code>, <code>Exec{Bool|Num|Node}()</code>, <code>ParseExec()</code> or <code>MustExec()</code>.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In the first example, the code accepts a user name specified by the user, and uses this
+unvalidated and unsanitized value in an XPath expression. This is vulnerable to the user providing
+special characters or string sequences that change the meaning of the XPath expression to search
+for different values.
+</p>
+
+<p>
+In the second example, the XPath expression is a hard-coded string that specifies some variables,
+which are safely resolved at runtime using the <code>goxpath.Opts</code> structure.
+</p>
+<sample src="XPathInjection.go" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://www.owasp.org/index.php?title=Testing_for_XPath_Injection_(OTG-INPVAL-010)">Testing for XPath Injection</a>.</li>
+<li>OWASP: <a href="https://www.owasp.org/index.php/XPATH_Injection">XPath Injection</a>.</li>
+</references>
+</qhelp>
diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.ql b/ql/src/experimental/Security/CWE-643/XPathInjection.ql
@@ -0,0 +1,188 @@
+/**
+ * @name XPath injection
+ * @description Building an XPath expression from user-controlled sources is vulnerable to insertion of
+ *              malicious code by the user.
+ * @kind path-problem
+ * @problem.severity error
+ * @id go/xml/xpath-injection
+ * @tags security
+ *       external/cwe/cwe-643
+ */
+
+import go
+import DataFlow::PathGraph
+
+class ByteSliceType extends SliceType {
+  ByteSliceType() { this.getElementType() instanceof Uint8Type }
+}
+
+class XPathInjectionConfiguration extends TaintTracking::Configuration {
+  XPathInjectionConfiguration() { this = "XPathInjectionConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof XPathInjectionSink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(Type t | t = node.getType().getUnderlyingType() |
+      not t instanceof StringType or not t instanceof ByteSliceType
+    )
+  }
+}
+
+abstract class XPathInjectionSink extends DataFlow::Node { }
+
+// https://github.com/antchfx/xpath
+class XPathSink extends XPathInjectionSink {
+  XPathSink() {
+    exists(Function f, string name | name.matches("Compile%") |
+      f.hasQualifiedName("github.com/antchfx/xpath", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f, string name | name.matches("MustCompile%") |
+      f.hasQualifiedName("github.com/antchfx/xpath", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f, string name | name.matches("Select%") |
+      f.hasQualifiedName("github.com/antchfx/xpath", name) and
+      this = f.getACall().getArgument(1)
+    )
+  }
+}
+
+// https://github.com/antchfx/htmlquery
+class HtmlQuerySink extends XPathInjectionSink {
+  HtmlQuerySink() {
+    exists(Function f, string name | name.matches("Find%") |
+      f.hasQualifiedName("github.com/antchfx/htmlquery", name) and
+      this = f.getACall().getArgument(1)
+    )
+    or
+    exists(Function f, string name | name.matches("Query%") |
+      f.hasQualifiedName("github.com/antchfx/htmlquery", name) and
+      this = f.getACall().getArgument(1)
+    )
+    or
+    exists(Function f |
+      f.hasQualifiedName("github.com/antchfx/htmlquery", "getQuery") and
+      this = f.getACall().getArgument(0)
+    )
+  }
+}
+
+// https://github.com/antchfx/xmlquery
+class XmlQuerySink extends XPathInjectionSink {
+  XmlQuerySink() {
+    exists(Function f, string name | name.matches("Find%") |
+      f.hasQualifiedName("github.com/antchfx/xmlquery", name) and
+      this = f.getACall().getArgument(1)
+    )
+    or
+    exists(Function f, string name | name.matches("Query%") |
+      f.hasQualifiedName("github.com/antchfx/xmlquery", name) and
+      this = f.getACall().getArgument(1)
+    )
+    or
+    exists(Function f, string name | name.matches("Select%") |
+      f.hasQualifiedName("github.com/antchfx/xmlquery", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f |
+      f.hasQualifiedName("github.com/antchfx/xmlquery", "getQuery") and
+      this = f.getACall().getArgument(0)
+    )
+  }
+}
+
+// https://github.com/antchfx/jsonquery
+class JsonQuerySink extends XPathInjectionSink {
+  JsonQuerySink() {
+    exists(Function f, string name | name.matches("Find%") |
+      f.hasQualifiedName("github.com/antchfx/jsonquery", name) and
+      this = f.getACall().getArgument(1)
+    )
+    or
+    exists(Function f, string name | name.matches("Query%") |
+      f.hasQualifiedName("github.com/antchfx/jsonquery", name) and
+      this = f.getACall().getArgument(1)
+    )
+    or
+    exists(Function f |
+      f.hasQualifiedName("github.com/antchfx/jsonquery", "getQuery") and
+      this = f.getACall().getArgument(0)
+    )
+  }
+}
+
+// https://github.com/go-xmlpath/xmlpath
+class XmlPathSink extends XPathInjectionSink {
+  XmlPathSink() {
+    exists(Function f, string name | name.matches("Compile%") |
+      f.hasQualifiedName("github.com/go-xmlpath/xmlpath", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f, string name | name.matches("MustCompile%") |
+      f.hasQualifiedName("github.com/go-xmlpath/xmlpath", name) and
+      this = f.getACall().getArgument(0)
+    )
+  }
+}
+
+// https://github.com/ChrisTrenkamp/goxpath
+class GoXPathSink extends XPathInjectionSink {
+  GoXPathSink() {
+    exists(Function f, string name | name.matches("Parse%") |
+      f.hasQualifiedName("github.com/ChrisTrenkamp/goxpath", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f, string name | name.matches("MustParse%") |
+      f.hasQualifiedName("github.com/ChrisTrenkamp/goxpath", name) and
+      this = f.getACall().getArgument(0)
+    )
+  }
+}
+
+// https://github.com/santhosh-tekuri/xpathparser
+class XPathParserSink extends XPathInjectionSink {
+  XPathParserSink() {
+    exists(Function f, string name | name.matches("Parse%") |
+      f.hasQualifiedName("github.com/santhosh-tekuri/xpathparser", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f, string name | name.matches("MustParse%") |
+      f.hasQualifiedName("github.com/santhosh-tekuri/xpathparser", name) and
+      this = f.getACall().getArgument(0)
+    )
+  }
+}
+
+// https://github.com/moovweb/gokogiri
+class GokogiriSink extends XPathInjectionSink {
+  GokogiriSink() {
+    exists(Function f, string name | name.matches("Compile%") |
+      f.hasQualifiedName("github.com/moovweb/gokogiri/xpath", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f, string name | name.matches("Search%") |
+      f.hasQualifiedName("github.com/moovweb/gokogiri/xml", name) and
+      this = f.getACall().getArgument(0)
+    )
+    or
+    exists(Function f, string name | name.matches("EvalXPath%") |
+      f.hasQualifiedName("github.com/moovweb/gokogiri/xml", name) and
+      this = f.getACall().getArgument(0)
+    )
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, XPathInjectionConfiguration c
+where c.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ flows here and is used in an XPath expression.",
+  source.getNode(), "A user-provided value"