Skip to content

Commit d89c10d

Browse files
alexrfordalexrford
authored
Merge pull request github#13130 from maikypedia/maikypedia/xpath-injection
Ruby : XPath Injection Query (CWE-643)
2 parents 9193de6 + dbb55ff commit d89c10d

File tree

14 files changed

+612
-0
lines changed

14 files changed

+612
-0
lines changed

‎ruby/ql/lib/codeql/ruby/Concepts.qll

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -843,6 +843,58 @@ module XmlParserCall {
843843
}
844844
}
845845

846+
/**
847+
* A data-flow node that constructs an XPath expression.
848+
*
849+
* If it is important that the XPath expression is indeed executed, then use `XPathExecution`.
850+
*
851+
* Extend this class to refine existing API models. If you want to model new APIs,
852+
* extend `XPathConstruction::Range` instead.
853+
*/
854+
class XPathConstruction extends DataFlow::Node instanceof XPathConstruction::Range {
855+
/** Gets the argument that specifies the XPath expressions to be constructed. */
856+
DataFlow::Node getXPath() { result = super.getXPath() }
857+
}
858+
859+
/** Provides a class for modeling new XPath construction APIs. */
860+
module XPathConstruction {
861+
/**
862+
* A data-flow node that constructs an XPath expression.
863+
*
864+
* Extend this class to model new APIs. If you want to refine existing API models,
865+
* extend `XPathConstruction` instead.
866+
*/
867+
abstract class Range extends DataFlow::Node {
868+
/** Gets the argument that specifies the XPath expressions to be constructed. */
869+
abstract DataFlow::Node getXPath();
870+
}
871+
}
872+
873+
/**
874+
* A data-flow node that executes an XPath expression.
875+
*
876+
* Extend this class to refine existing API models. If you want to model new APIs,
877+
* extend `XPathExecution::Range` instead.
878+
*/
879+
class XPathExecution extends DataFlow::Node instanceof XPathExecution::Range {
880+
/** Gets the argument that specifies the XPath expressions to be executed. */
881+
DataFlow::Node getXPath() { result = super.getXPath() }
882+
}
883+
884+
/** Provides a class for modeling new XPath execution APIs. */
885+
module XPathExecution {
886+
/**
887+
* A data-flow node that executes an XPath expression.
888+
*
889+
* Extend this class to model new APIs. If you want to refine existing API models,
890+
* extend `XPathExecution` instead.
891+
*/
892+
abstract class Range extends DataFlow::Node {
893+
/** Gets the argument that specifies the XPath expressions to be executed. */
894+
abstract DataFlow::Node getXPath();
895+
}
896+
}
897+
846898
/**
847899
* A data-flow node that may represent a database object in an ORM system.
848900
*

‎ruby/ql/lib/codeql/ruby/frameworks/XmlParsing.qll

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,17 @@ private class NokogiriXmlParserCall extends XmlParserCall::Range, DataFlow::Call
4545
}
4646
}
4747

48+
/** Execution of a XPath statement. */
49+
private class NokogiriXPathExecution extends XPathExecution::Range, DataFlow::CallNode {
50+
NokogiriXPathExecution() {
51+
exists(NokogiriXmlParserCall parserCall |
52+
this = parserCall.getAMethodCall(["xpath", "at_xpath", "search", "at"])
53+
)
54+
}
55+
56+
override DataFlow::Node getXPath() { result = this.getArgument(0) }
57+
}
58+
4859
/**
4960
* Holds if `assign` enables the `default_substitute_entities` option in
5061
* libxml-ruby.
@@ -123,6 +134,40 @@ private predicate xmlMiniEntitySubstitutionEnabled() {
123134
enablesLibXmlDefaultEntitySubstitution(_)
124135
}
125136

137+
/** Execution of a XPath statement. */
138+
private class LibXmlXPathExecution extends XPathExecution::Range, DataFlow::CallNode {
139+
LibXmlXPathExecution() {
140+
exists(LibXmlRubyXmlParserCall parserCall |
141+
this = parserCall.getAMethodCall(["find", "find_first"])
142+
)
143+
}
144+
145+
override DataFlow::Node getXPath() { result = this.getArgument(0) }
146+
}
147+
148+
/** A call to `REXML::Document.new`, considered as a XML parsing. */
149+
private class RexmlParserCall extends XmlParserCall::Range, DataFlow::CallNode {
150+
RexmlParserCall() {
151+
this = API::getTopLevelMember("REXML").getMember("Document").getAnInstantiation()
152+
}
153+
154+
override DataFlow::Node getInput() { result = this.getArgument(0) }
155+
156+
/** No option for parsing */
157+
override predicate externalEntitiesEnabled() { none() }
158+
}
159+
160+
/** Execution of a XPath statement. */
161+
private class RexmlXPathExecution extends XPathExecution::Range, DataFlow::CallNode {
162+
RexmlXPathExecution() {
163+
this =
164+
[API::getTopLevelMember("REXML").getMember("XPath"), API::getTopLevelMember("XPath")]
165+
.getAMethodCall(["each", "first", "match"])
166+
}
167+
168+
override DataFlow::Node getXPath() { result = this.getArgument(1) }
169+
}
170+
126171
/**
127172
* A call to `ActiveSupport::XmlMini.parse` considered as an `XmlParserCall`.
128173
*/

‎ruby/ql/lib/codeql/ruby/security/XpathInjectionCustomizations.qll

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/**
2+
* Provides class and predicates to track external data that
3+
* may represent malicious xpath query objects.
4+
*
5+
* This module is intended to be imported into a taint-tracking query.
6+
*/
7+
8+
private import codeql.ruby.Concepts
9+
private import codeql.ruby.DataFlow
10+
private import codeql.ruby.dataflow.BarrierGuards
11+
private import codeql.ruby.dataflow.RemoteFlowSources
12+
13+
/** Models Xpath Injection related classes and functions */
14+
module XpathInjection {
15+
/** A data flow source for "XPath injection" vulnerabilities. */
16+
abstract class Source extends DataFlow::Node { }
17+
18+
/** A data flow sink for "XPath injection" vulnerabilities */
19+
abstract class Sink extends DataFlow::Node { }
20+
21+
/** A sanitizer for "XPath injection" vulnerabilities. */
22+
abstract class Sanitizer extends DataFlow::Node { }
23+
24+
/**
25+
* A source of remote user input, considered as a flow source.
26+
*/
27+
private class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
28+
29+
/**
30+
* An execution of an XPath expression, considered as a sink.
31+
*/
32+
private class XPathExecutionAsSink extends Sink {
33+
XPathExecutionAsSink() { this = any(XPathExecution e).getXPath() }
34+
}
35+
36+
/**
37+
* A construction of an XPath expression, considered as a sink.
38+
*/
39+
private class XPathConstructionAsSink extends Sink {
40+
XPathConstructionAsSink() { this = any(XPathConstruction c).getXPath() }
41+
}
42+
43+
/**
44+
* A comparison with a constant string, considered as a sanitizer-guard.
45+
*/
46+
private class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
47+
48+
/**
49+
* An inclusion check against an array of constant strings, considered as a
50+
* sanitizer-guard.
51+
*/
52+
private class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
53+
StringConstArrayInclusionCallBarrier
54+
{ }
55+
}

‎ruby/ql/lib/codeql/ruby/security/XpathInjectionQuery.qll

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* Provides a taint-tracking configuration for detecting "Xpath Injection" vulnerabilities.
3+
*
4+
* Note, for performance reasons: only import this file if
5+
* `XpathInjection::Configuration` is needed, otherwise
6+
* `XpathInjectionCustomizations` should be imported instead.
7+
*/
8+
9+
private import codeql.ruby.DataFlow
10+
private import codeql.ruby.TaintTracking
11+
import XpathInjectionCustomizations::XpathInjection
12+
13+
/** Provides a taint-tracking configuration for detecting "Xpath Injection" vulnerabilities. */
14+
module XpathInjection {
15+
/**
16+
* A taint-tracking configuration for detecting "Xpath Injection" vulnerabilities.
17+
*/
18+
private module Config implements DataFlow::ConfigSig {
19+
predicate isSource(DataFlow::Node source) { source instanceof Source }
20+
21+
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
22+
23+
predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
24+
}
25+
26+
import TaintTracking::Global<Config>
27+
}

‎ruby/ql/src/change-notes/2023-05-11-xpath-injection-query.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new experimental query, `rb/xpath-injection`, to detect cases where XPath statements are constructed from user input in an unsafe manner.

‎ruby/ql/src/experimental/xpath-injection/XpathInjection.qhelp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
If an XPath expression is built using string concatenation, and the components of the concatenation
7+
include user input, it makes it very easy for a user to create a malicious XPath expression.
8+
</p>
9+
</overview>
10+
11+
<recommendation>
12+
<p>
13+
If user input must be included in an XPath expression, either sanitize the data or use variable
14+
references to safely embed it without altering the structure of the expression.
15+
</p>
16+
</recommendation>
17+
18+
<example>
19+
<p>
20+
The following example uses the <code>nokogiri</code>, <code>rexml</code> and <code>libxml</code> XML parsers to parse a string <code>xml</code>.
21+
Then the xpath query is controlled by the user and hence leads to a vulnerability.
22+
</p>
23+
<sample src="examples/XPathBad.rb"/>
24+
25+
<p>
26+
To guard against XPath Injection attacks, the user input should be sanitized.
27+
</p>
28+
<sample src="examples/XPathGood.rb"/>
29+
</example>
30+
31+
<references>
32+
<li>
33+
OWASP:
34+
<a href="https://owasp.org/www-community/attacks/XPATH_Injection">XPath injection</a>.
35+
</li>
36+
</references>
37+
</qhelp>

‎ruby/ql/src/experimental/xpath-injection/XpathInjection.ql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name XPath query built from user-controlled sources
3+
* @description Building a XPath query from user-controlled sources is vulnerable to insertion of
4+
* malicious XPath code by the user.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 9.8
8+
* @precision high
9+
* @id rb/xpath-injection
10+
* @tags security
11+
* external/cwe/cwe-643
12+
*/
13+
14+
import codeql.ruby.DataFlow
15+
import codeql.ruby.security.XpathInjectionQuery
16+
import XpathInjection::PathGraph
17+
18+
from XpathInjection::PathNode source, XpathInjection::PathNode sink
19+
where XpathInjection::flowPath(source, sink)
20+
select sink.getNode(), source, sink, "XPath expression depends on a $@.", source.getNode(),
21+
"user-provided value"

‎ruby/ql/src/experimental/xpath-injection/examples/XPathBad.rb

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
require 'nokogiri'
2+
require 'rexml'
3+
require 'libxml'
4+
5+
class BadNokogiriController < ActionController::Base
6+
def some_request_handler
7+
name = params["name"]
8+
xml = <<-XML
9+
<root>
10+
<foo>bar</foo>
11+
<password>THIS IS SECRET</password>
12+
</root>
13+
XML
14+
doc = Nokogiri::XML.parse(xml)
15+
results = doc.xpath("//#{name}")
16+
end
17+
end
18+
19+
class BadRexmlController < ActionController::Base
20+
def some_request_handler
21+
name = params["name"]
22+
xml = <<-XML
23+
<root>
24+
<foo>bar</foo>
25+
<password>THIS IS SECRET</password>
26+
</root>
27+
XML
28+
doc = REXML::Document.new(xml)
29+
results = REXML::XPath.first(doc, "//#{name}")
30+
end
31+
end
32+
33+
class BadLibxmlController < ActionController::Base
34+
def some_request_handler
35+
name = params["name"]
36+
xml = <<-XML
37+
<root>
38+
<foo>bar</foo>
39+
<password>THIS IS SECRET</password>
40+
</root>
41+
XML
42+
doc = LibXML::XML::Document.string(xml)
43+
results = doc.find_first("//#{name}")
44+
end
45+
end

‎ruby/ql/src/experimental/xpath-injection/examples/XPathGood.rb

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
require 'nokogiri'
2+
require 'rexml'
3+
require 'libxml'
4+
5+
class BadNokogiriController < ActionController::Base
6+
def some_request_handler
7+
name = params["name"]
8+
xml = <<-XML
9+
<root>
10+
<foo>bar</foo>
11+
<password>THIS IS SECRET</password>
12+
</root>
13+
XML
14+
doc = Nokogiri::XML.parse(xml)
15+
name = if ["foo", "foo2"].include? name
16+
name
17+
else
18+
name = "foo"
19+
end
20+
results = doc.xpath("//#{name}")
21+
end
22+
end
23+
24+
class BadRexmlController < ActionController::Base
25+
def some_request_handler
26+
name = params["name"]
27+
xml = <<-XML
28+
<root>
29+
<foo>bar</foo>
30+
<password>THIS IS SECRET</password>
31+
</root>
32+
XML
33+
doc = REXML::Document.new(xml)
34+
name = if ["foo", "foo2"].include? name
35+
name
36+
else
37+
name = "foo"
38+
end
39+
results = REXML::XPath.first(doc, "//#{name}")
40+
end
41+
end
42+
43+
class BadLibxmlController < ActionController::Base
44+
def some_request_handler
45+
name = params["name"]
46+
xml = <<-XML
47+
<root>
48+
<foo>bar</foo>
49+
<password>THIS IS SECRET</password>
50+
</root>
51+
XML
52+
doc = LibXML::XML::Document.string(xml)
53+
name = if ["foo", "foo2"].include? name
54+
name
55+
else
56+
name = "foo"
57+
end
58+
results = doc.find_first("//#{name}")
59+
end
60+
end

0 commit comments

Comments
 (0)