From d6ff6b74c5d55134c4e31e74f5b44a4bef5dd05a Mon Sep 17 00:00:00 2001 From: intrigus Date: Thu, 19 Mar 2020 22:26:37 +0100 Subject: [PATCH 1/7] CWE-643 XPathInjection on Go --- .../Security/CWE-643/XPathInjection.go | 19 +++ .../Security/CWE-643/XPathInjection.qhelp | 45 ++++++ .../Security/CWE-643/XPathInjection.ql | 143 ++++++++++++++++++ 3 files changed, 207 insertions(+) create mode 100644 ql/src/experimental/Security/CWE-643/XPathInjection.go create mode 100644 ql/src/experimental/Security/CWE-643/XPathInjection.qhelp create mode 100644 ql/src/experimental/Security/CWE-643/XPathInjection.ql diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.go b/ql/src/experimental/Security/CWE-643/XPathInjection.go new file mode 100644 index 000000000..5d51fa328 --- /dev/null +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.go @@ -0,0 +1,19 @@ +package main + +import ( + "net/http" + "github.com/moovweb/gokogiri" +) + +func processRequest(r *http.Request, doc *XmlDocument) { + r.parseForm() + username := r.Form.Get("username") + password := r.Form.Get("password") + + root := doc.Root() + // BAD: User input used directly in an XPath expression + doc, _ := root.SearchWithVariables("//users/user[login/text()='" + username + "' and password/text() = '" + password + "']/home_dir/text()") + + // GOOD: Uses parameters to avoid including user input directly in XPath expression + doc, _ := root.SearchWithVariables("//users/user[login/text()=$username and password/text() = $password]/home_dir/text()") +} diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp b/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp new file mode 100644 index 000000000..4996733a3 --- /dev/null +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp @@ -0,0 +1,45 @@ + + + +

+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. +

+ + + +

+If user input must be included in an XPath expression, pre-compile the query and use variable +references to include the user input. +

+

+For exmaple, when using the github.com/moovweb/gokogiri API, this can be done by creating a custom subtype of +xpath.VariableScope, and implementing +ResolveVariable(string, string) to return the user provided data. This +custom scope can be specified when calling SearchWithVariables(), EvalXPath() or EvalXPathAsBoolean(). +

+ + + + +

+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. +

+ +

+In the second example, the XPath expression is a hard-coded string that specifies some variables, +which are safely replaced at runtime using a custom xpath.VariableScope. +

+ + + + +
  • OWASP: Testing for XPath Injection.
    • +
  • OWASP: XPath Injection.
    • +     +     diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.ql b/ql/src/experimental/Security/CWE-643/XPathInjection.ql new file mode 100644 index 000000000..2e34aaca3 --- /dev/null +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.ql @@ -0,0 +1,143 @@ +/** + * @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 + * @precision high + * @id go/xml/xpath-injection + * @tags security + * external/cwe/cwe-643 + */ + +import go +import semmle.go.dataflow.TaintTracking +import DataFlow::PathGraph + +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 } +} + +abstract class XPathInjectionSink extends DataFlow::Node { } + +Function getAMatchingFunction(string package, int argumentNumber, CallExpr call, Expr argument) { + result.getPackage().getName() = package and + call.getTarget() = result and + call.getArgument(argumentNumber) = argument +} + +// https://github.com/antchfx/xpath +class XPathSink extends XPathInjectionSink { + XPathSink() { + exists(CallExpr call | + getAMatchingFunction("xpath", 0, call, this.asExpr()).getName().matches("Compile") + or + getAMatchingFunction("xpath", 0, call, this.asExpr()).getName().matches("MustCompile") + or + getAMatchingFunction("xpath", 1, call, this.asExpr()).getName().matches("Select") + ) + } +} + +// https://github.com/antchfx/htmlquery +class HtmlQuerySink extends XPathInjectionSink { + HtmlQuerySink() { + exists(CallExpr call | + getAMatchingFunction("htmlquery", 1, call, this.asExpr()).getName().matches("Find%") + or + getAMatchingFunction("htmlquery", 1, call, this.asExpr()).getName().matches("Query%") + or + getAMatchingFunction("htmlquery", 0, call, this.asExpr()).getName().matches("getQuery") + ) + } +} + +// https://github.com/antchfx/xmlquery +class XmlQuerySink extends XPathInjectionSink { + XmlQuerySink() { + exists(CallExpr call | + getAMatchingFunction("xmlquery", 1, call, this.asExpr()).getName().matches("Find%") + or + getAMatchingFunction("xmlquery", 1, call, this.asExpr()).getName().matches("Query%") + or + getAMatchingFunction("xmlquery", 0, call, this.asExpr()).getName().matches("getQuery") + or + getAMatchingFunction("xmlquery", 0, call, this.asExpr()).getName().matches("Select%") + ) + } +} + +// https://github.com/antchfx/jsonquery +class JsonQuerySink extends XPathInjectionSink { + JsonQuerySink() { + exists(CallExpr call | + getAMatchingFunction("jsonquery", 1, call, this.asExpr()).getName().matches("Find%") + or + getAMatchingFunction("jsonquery", 1, call, this.asExpr()).getName().matches("Query%") + or + getAMatchingFunction("jsonquery", 0, call, this.asExpr()).getName().matches("getQuery") + ) + } +} + +// https://github.com/go-xmlpath/xmlpath +class XmlPathSink extends XPathInjectionSink { + XmlPathSink() { + exists(CallExpr call | + getAMatchingFunction("xmlpath", 0, call, this.asExpr()).getName().matches("Compile%") + or + getAMatchingFunction("xmlpath", 0, call, this.asExpr()).getName().matches("MustCompile") + ) + } +} + +// https://github.com/ChrisTrenkamp/goxpath +class GoXPathSink extends XPathInjectionSink { + GoXPathSink() { + exists(CallExpr call | + getAMatchingFunction("goxpath", 0, call, this.asExpr()).getName().matches("Parse") + or + getAMatchingFunction("goxpath", 0, call, this.asExpr()).getName().matches("MustParse") + or + getAMatchingFunction("goxpath", 0, call, this.asExpr()).getName().matches("ParseExec") + ) + } +} + +// https://github.com/santhosh-tekuri/xpathparser +class XPathParserSink extends XPathInjectionSink { + XPathParserSink() { + exists(CallExpr call | + getAMatchingFunction("xpathparser", 0, call, this.asExpr()).getName().matches("Parse") + or + getAMatchingFunction("xpathparser", 0, call, this.asExpr()).getName().matches("MustParse") + ) + } +} + +// https://github.com/moovweb/gokogiri +class GokogiriSink extends XPathInjectionSink { + GokogiriSink() { + exists(CallExpr call | + getAMatchingFunction("xpath", 0, call, this.asExpr()).getName().matches("Compile") + or + // TODO: The following cases will have false positives in case a *xpath.Expression is supplied + // I don't know how to fix this, since I'm new to the Go QL flavour. + // https://github.com/moovweb/gokogiri/blob/a1a828153468a7518b184e698f6265904108d957/xml/node.go#L613 + getAMatchingFunction("xml", 0, call, this.asExpr()).getName().matches("Search%") + or + getAMatchingFunction("xml", 0, call, this.asExpr()).getName().matches("EvalXPath%") + ) + } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, XPathInjectionConfiguration c +where c.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "$@ flows to here and is used in an XPath expression.", + source.getNode(), "User-provided value" From ec40cf0379cbbadafa84c54428a3ac218ca969d3 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Fri, 20 Mar 2020 13:55:31 +0100 Subject: [PATCH 2/7] Apply suggestions from review Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> --- ql/src/experimental/Security/CWE-643/XPathInjection.qhelp | 4 ++-- ql/src/experimental/Security/CWE-643/XPathInjection.ql | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp b/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp index 4996733a3..91bd0ca9e 100644 --- a/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp @@ -15,10 +15,10 @@ If user input must be included in an XPath expression, pre-compile the query and references to include the user input.

    -For exmaple, when using the github.com/moovweb/gokogiri API, this can be done by creating a custom subtype of +For example, when using the github.com/moovweb/gokogiri API, this can be done by creating a custom subtype of xpath.VariableScope, and implementing ResolveVariable(string, string) to return the user provided data. This -custom scope can be specified when calling SearchWithVariables(), EvalXPath() or EvalXPathAsBoolean(). +custom scope can be specified when calling SearchWithVariables(), EvalXPath(), or EvalXPathAsBoolean().

    diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.ql b/ql/src/experimental/Security/CWE-643/XPathInjection.ql index 2e34aaca3..f24aefa82 100644 --- a/ql/src/experimental/Security/CWE-643/XPathInjection.ql +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.ql @@ -139,5 +139,5 @@ class GokogiriSink extends XPathInjectionSink { from DataFlow::PathNode source, DataFlow::PathNode sink, XPathInjectionConfiguration c where c.hasFlowPath(source, sink) -select sink.getNode(), source, sink, "$@ flows to here and is used in an XPath expression.", - source.getNode(), "User-provided value" +select sink.getNode(), source, sink, "$@ flows here and is used in an XPath expression.", + source.getNode(), "A user-provided value" From c7ead88b91dcf1c6a0d07c4d5091256a62b47ff0 Mon Sep 17 00:00:00 2001 From: intrigus Date: Fri, 20 Mar 2020 20:07:28 +0100 Subject: [PATCH 3/7] Restructure query, add default sanitizer --- .../Security/CWE-643/XPathInjection.ql | 167 +++++++++++------- 1 file changed, 106 insertions(+), 61 deletions(-) diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.ql b/ql/src/experimental/Security/CWE-643/XPathInjection.ql index f24aefa82..985222715 100644 --- a/ql/src/experimental/Security/CWE-643/XPathInjection.ql +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.ql @@ -11,36 +11,43 @@ */ import go -import semmle.go.dataflow.TaintTracking 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 isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof XPathInjectionSink } + + override predicate isSanitizer(DataFlow::Node node) { + not node.asExpr().getType() instanceof StringType or + not node.asExpr().getType() instanceof ByteSliceType + } } abstract class XPathInjectionSink extends DataFlow::Node { } -Function getAMatchingFunction(string package, int argumentNumber, CallExpr call, Expr argument) { - result.getPackage().getName() = package and - call.getTarget() = result and - call.getArgument(argumentNumber) = argument -} - // https://github.com/antchfx/xpath class XPathSink extends XPathInjectionSink { XPathSink() { - exists(CallExpr call | - getAMatchingFunction("xpath", 0, call, this.asExpr()).getName().matches("Compile") - or - getAMatchingFunction("xpath", 0, call, this.asExpr()).getName().matches("MustCompile") - or - getAMatchingFunction("xpath", 1, call, this.asExpr()).getName().matches("Select") + exists(Function f | + f.hasQualifiedName("github.com/antchfx/xpath", "Compile%") and + this = f.getACall().getArgument(0) + ) + or + exists(Function f | + f.hasQualifiedName("github.com/antchfx/xpath", "MustCompile%") and + this = f.getACall().getArgument(0) + ) + or + exists(Function f | + f.hasQualifiedName("github.com/antchfx/xpath", "Select%") and + this = f.getACall().getArgument(1) ) } } @@ -48,12 +55,19 @@ class XPathSink extends XPathInjectionSink { // https://github.com/antchfx/htmlquery class HtmlQuerySink extends XPathInjectionSink { HtmlQuerySink() { - exists(CallExpr call | - getAMatchingFunction("htmlquery", 1, call, this.asExpr()).getName().matches("Find%") - or - getAMatchingFunction("htmlquery", 1, call, this.asExpr()).getName().matches("Query%") - or - getAMatchingFunction("htmlquery", 0, call, this.asExpr()).getName().matches("getQuery") + 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) ) } } @@ -61,14 +75,24 @@ class HtmlQuerySink extends XPathInjectionSink { // https://github.com/antchfx/xmlquery class XmlQuerySink extends XPathInjectionSink { XmlQuerySink() { - exists(CallExpr call | - getAMatchingFunction("xmlquery", 1, call, this.asExpr()).getName().matches("Find%") - or - getAMatchingFunction("xmlquery", 1, call, this.asExpr()).getName().matches("Query%") - or - getAMatchingFunction("xmlquery", 0, call, this.asExpr()).getName().matches("getQuery") - or - getAMatchingFunction("xmlquery", 0, call, this.asExpr()).getName().matches("Select%") + 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) ) } } @@ -76,12 +100,19 @@ class XmlQuerySink extends XPathInjectionSink { // https://github.com/antchfx/jsonquery class JsonQuerySink extends XPathInjectionSink { JsonQuerySink() { - exists(CallExpr call | - getAMatchingFunction("jsonquery", 1, call, this.asExpr()).getName().matches("Find%") - or - getAMatchingFunction("jsonquery", 1, call, this.asExpr()).getName().matches("Query%") - or - getAMatchingFunction("jsonquery", 0, call, this.asExpr()).getName().matches("getQuery") + 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) ) } } @@ -89,10 +120,14 @@ class JsonQuerySink extends XPathInjectionSink { // https://github.com/go-xmlpath/xmlpath class XmlPathSink extends XPathInjectionSink { XmlPathSink() { - exists(CallExpr call | - getAMatchingFunction("xmlpath", 0, call, this.asExpr()).getName().matches("Compile%") - or - getAMatchingFunction("xmlpath", 0, call, this.asExpr()).getName().matches("MustCompile") + 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) ) } } @@ -100,12 +135,14 @@ class XmlPathSink extends XPathInjectionSink { // https://github.com/ChrisTrenkamp/goxpath class GoXPathSink extends XPathInjectionSink { GoXPathSink() { - exists(CallExpr call | - getAMatchingFunction("goxpath", 0, call, this.asExpr()).getName().matches("Parse") - or - getAMatchingFunction("goxpath", 0, call, this.asExpr()).getName().matches("MustParse") - or - getAMatchingFunction("goxpath", 0, call, this.asExpr()).getName().matches("ParseExec") + 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) ) } } @@ -113,10 +150,14 @@ class GoXPathSink extends XPathInjectionSink { // https://github.com/santhosh-tekuri/xpathparser class XPathParserSink extends XPathInjectionSink { XPathParserSink() { - exists(CallExpr call | - getAMatchingFunction("xpathparser", 0, call, this.asExpr()).getName().matches("Parse") - or - getAMatchingFunction("xpathparser", 0, call, this.asExpr()).getName().matches("MustParse") + 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) ) } } @@ -124,20 +165,24 @@ class XPathParserSink extends XPathInjectionSink { // https://github.com/moovweb/gokogiri class GokogiriSink extends XPathInjectionSink { GokogiriSink() { - exists(CallExpr call | - getAMatchingFunction("xpath", 0, call, this.asExpr()).getName().matches("Compile") - or - // TODO: The following cases will have false positives in case a *xpath.Expression is supplied - // I don't know how to fix this, since I'm new to the Go QL flavour. - // https://github.com/moovweb/gokogiri/blob/a1a828153468a7518b184e698f6265904108d957/xml/node.go#L613 - getAMatchingFunction("xml", 0, call, this.asExpr()).getName().matches("Search%") - or - getAMatchingFunction("xml", 0, call, this.asExpr()).getName().matches("EvalXPath%") + 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) +from DataFlow::PathNode source, DataFlow::PathNode sink, XPathInjectionConfiguration c, Function f +where c.hasFlowPath(source, sink) and f.getName().matches("Compile%") select sink.getNode(), source, sink, "$@ flows here and is used in an XPath expression.", source.getNode(), "A user-provided value" From 948b79df876ba58858656feb9fd5426236190816 Mon Sep 17 00:00:00 2001 From: intrigus Date: Fri, 20 Mar 2020 21:09:28 +0100 Subject: [PATCH 4/7] Update xpath example, use goxpath package --- .../Security/CWE-643/XPathInjection.go | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.go b/ql/src/experimental/Security/CWE-643/XPathInjection.go index 5d51fa328..869c98acb 100644 --- a/ql/src/experimental/Security/CWE-643/XPathInjection.go +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.go @@ -1,19 +1,32 @@ package main import ( - "net/http" - "github.com/moovweb/gokogiri" + "fmt" + "net/http" + + "github.com/ChrisTrenkamp/goxpath" + "github.com/ChrisTrenkamp/goxpath/tree" ) -func processRequest(r *http.Request, doc *XmlDocument) { - r.parseForm() - username := r.Form.Get("username") - password := r.Form.Get("password") - - root := doc.Root() - // BAD: User input used directly in an XPath expression - doc, _ := root.SearchWithVariables("//users/user[login/text()='" + username + "' and password/text() = '" + password + "']/home_dir/text()") +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: Uses parameters to avoid including user input directly in XPath expression - doc, _ := root.SearchWithVariables("//users/user[login/text()=$username and password/text() = $password]/home_dir/text()") + // 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) } From d81c9b145e825f2f779287404941df5b2ea01a8c Mon Sep 17 00:00:00 2001 From: intrigus Date: Fri, 20 Mar 2020 21:32:12 +0100 Subject: [PATCH 5/7] Update query help to use goxpath --- .../experimental/Security/CWE-643/XPathInjection.qhelp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp b/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp index 91bd0ca9e..23d693ac7 100644 --- a/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.qhelp @@ -15,10 +15,9 @@ If user input must be included in an XPath expression, pre-compile the query and references to include the user input.

    -For example, when using the github.com/moovweb/gokogiri API, this can be done by creating a custom subtype of -xpath.VariableScope, and implementing -ResolveVariable(string, string) to return the user provided data. This -custom scope can be specified when calling SearchWithVariables(), EvalXPath(), or EvalXPathAsBoolean(). +For example, when using the github.com/ChrisTrenkamp/goxpath API, this can be done by creating a function that takes an *goxpath.Opts structure. +In this structure you can then set the values of the variable references. +This function can then be specified when calling Exec(), Exec{Bool|Num|Node}(), ParseExec() or MustExec().

    @@ -33,7 +32,7 @@ for different values.

    In the second example, the XPath expression is a hard-coded string that specifies some variables, -which are safely replaced at runtime using a custom xpath.VariableScope. +which are safely resolved at runtime using the goxpath.Opts structure.

    From 9187bacd3c5b1b21e2200e047af95b86a6701456 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Mon, 23 Mar 2020 16:45:56 +0100 Subject: [PATCH 6/7] Apply suggestion from code review Use getUnderlyingType() to account for named aliases. Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> --- ql/src/experimental/Security/CWE-643/XPathInjection.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.ql b/ql/src/experimental/Security/CWE-643/XPathInjection.ql index 985222715..b0b00062e 100644 --- a/ql/src/experimental/Security/CWE-643/XPathInjection.ql +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.ql @@ -25,8 +25,9 @@ class XPathInjectionConfiguration extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof XPathInjectionSink } override predicate isSanitizer(DataFlow::Node node) { - not node.asExpr().getType() instanceof StringType or - not node.asExpr().getType() instanceof ByteSliceType + exists(Type t | t = node.getType().getUnderlyingType() | + not t instanceof StringType or not t instanceof ByteSliceType + ) } } From 1f635806b35d684ef72848d713a25f7ae2fc56bc Mon Sep 17 00:00:00 2001 From: intrigus Date: Mon, 23 Mar 2020 16:49:45 +0100 Subject: [PATCH 7/7] Fix copy-paste errors, remove debugging code --- .../Security/CWE-643/XPathInjection.ql | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/ql/src/experimental/Security/CWE-643/XPathInjection.ql b/ql/src/experimental/Security/CWE-643/XPathInjection.ql index b0b00062e..453710d71 100644 --- a/ql/src/experimental/Security/CWE-643/XPathInjection.ql +++ b/ql/src/experimental/Security/CWE-643/XPathInjection.ql @@ -4,7 +4,6 @@ * malicious code by the user. * @kind path-problem * @problem.severity error - * @precision high * @id go/xml/xpath-injection * @tags security * external/cwe/cwe-643 @@ -25,9 +24,9 @@ class XPathInjectionConfiguration extends TaintTracking::Configuration { 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 - ) + exists(Type t | t = node.getType().getUnderlyingType() | + not t instanceof StringType or not t instanceof ByteSliceType + ) } } @@ -36,18 +35,18 @@ abstract class XPathInjectionSink extends DataFlow::Node { } // https://github.com/antchfx/xpath class XPathSink extends XPathInjectionSink { XPathSink() { - exists(Function f | - f.hasQualifiedName("github.com/antchfx/xpath", "Compile%") and + 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 | - f.hasQualifiedName("github.com/antchfx/xpath", "MustCompile%") and + 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 | - f.hasQualifiedName("github.com/antchfx/xpath", "Select%") and + exists(Function f, string name | name.matches("Select%") | + f.hasQualifiedName("github.com/antchfx/xpath", name) and this = f.getACall().getArgument(1) ) } @@ -183,7 +182,7 @@ class GokogiriSink extends XPathInjectionSink { } } -from DataFlow::PathNode source, DataFlow::PathNode sink, XPathInjectionConfiguration c, Function f -where c.hasFlowPath(source, sink) and f.getName().matches("Compile%") +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"