Skip to content

Commit 2f46277

Browse files
aibaarsaibaars
authored
Merge pull request #286 from github/aibaars/xxe
XXE query
2 parents f61161e + 5a454bb commit 2f46277

File tree

11 files changed

+462
-0
lines changed

11 files changed

+462
-0
lines changed

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,3 +503,38 @@ module CodeExecution {
503503
abstract DataFlow::Node getCode();
504504
}
505505
}
506+
507+
/**
508+
* A data-flow node that parses XML content.
509+
*
510+
* Extend this class to refine existing API models. If you want to model new APIs,
511+
* extend `XmlParserCall::Range` instead.
512+
*/
513+
class XmlParserCall extends DataFlow::Node {
514+
XmlParserCall::Range range;
515+
516+
XmlParserCall() { this = range }
517+
518+
/** Gets the argument that specifies the XML content to be parsed. */
519+
DataFlow::Node getInput() { result = range.getInput() }
520+
521+
/** Holds if this XML parser call is configured to process external entities */
522+
predicate externalEntitiesEnabled() { range.externalEntitiesEnabled() }
523+
}
524+
525+
/** Provides a class for modeling new XML parsing APIs. */
526+
module XmlParserCall {
527+
/**
528+
* A data-flow node that parses XML content.
529+
*
530+
* Extend this class to model new APIs. If you want to refine existing API models,
531+
* extend `class XmlParserCall` instead.
532+
*/
533+
abstract class Range extends DataFlow::Node {
534+
/** Gets the argument that specifies the XML content to be parsed. */
535+
abstract DataFlow::Node getInput();
536+
537+
/** Holds if this XML parser call is configured to process external entities */
538+
abstract predicate externalEntitiesEnabled();
539+
}
540+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,4 @@ private import codeql.ruby.frameworks.ActionView
88
private import codeql.ruby.frameworks.StandardLibrary
99
private import codeql.ruby.frameworks.Files
1010
private import codeql.ruby.frameworks.HttpClients
11+
private import codeql.ruby.frameworks.XmlParsing

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

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
private import codeql.ruby.Concepts
2+
private import codeql.ruby.AST
3+
private import codeql.ruby.DataFlow
4+
private import codeql.ruby.typetracking.TypeTracker
5+
private import codeql.ruby.ApiGraphs
6+
private import codeql.ruby.controlflow.CfgNodes as CfgNodes
7+
8+
private class NokogiriXmlParserCall extends XmlParserCall::Range, DataFlow::CallNode {
9+
NokogiriXmlParserCall() {
10+
this =
11+
[
12+
API::getTopLevelMember("Nokogiri").getMember("XML"),
13+
API::getTopLevelMember("Nokogiri").getMember("XML").getMember("Document"),
14+
API::getTopLevelMember("Nokogiri")
15+
.getMember("XML")
16+
.getMember("SAX")
17+
.getMember("Parser")
18+
.getInstance()
19+
].getAMethodCall("parse")
20+
}
21+
22+
override DataFlow::Node getInput() { result = this.getArgument(0) }
23+
24+
override predicate externalEntitiesEnabled() {
25+
this.getArgument(3) =
26+
[trackEnableFeature(TNOENT()), trackEnableFeature(TDTDLOAD()), trackDisableFeature(TNONET())]
27+
or
28+
// calls to methods that enable/disable features in a block argument passed to this parser call.
29+
// For example:
30+
// ```ruby
31+
// doc.parse(...) { |options| options.nononet; options.noent }
32+
// ```
33+
this.asExpr()
34+
.getExpr()
35+
.(MethodCall)
36+
.getBlock()
37+
.getAStmt()
38+
.getAChild*()
39+
.(MethodCall)
40+
.getMethodName() = ["noent", "dtdload", "nononet"]
41+
}
42+
}
43+
44+
private class LibXmlRubyXmlParserCall extends XmlParserCall::Range, DataFlow::CallNode {
45+
LibXmlRubyXmlParserCall() {
46+
this =
47+
[API::getTopLevelMember("LibXML").getMember("XML"), API::getTopLevelMember("XML")]
48+
.getMember(["Document", "Parser"])
49+
.getAMethodCall(["file", "io", "string"])
50+
}
51+
52+
override DataFlow::Node getInput() { result = this.getArgument(0) }
53+
54+
override predicate externalEntitiesEnabled() {
55+
exists(Pair pair |
56+
pair = this.getArgument(1).asExpr().getExpr().(HashLiteral).getAKeyValuePair() and
57+
pair.getKey().(Literal).getValueText() = "options" and
58+
pair.getValue() =
59+
[
60+
trackEnableFeature(TNOENT()), trackEnableFeature(TDTDLOAD()),
61+
trackDisableFeature(TNONET())
62+
].asExpr().getExpr()
63+
)
64+
}
65+
}
66+
67+
private newtype TFeature =
68+
TNOENT() or
69+
TNONET() or
70+
TDTDLOAD()
71+
72+
class Feature extends TFeature {
73+
abstract int getValue();
74+
75+
string toString() { result = getConstantName() }
76+
77+
abstract string getConstantName();
78+
}
79+
80+
private class FeatureNOENT extends Feature, TNOENT {
81+
override int getValue() { result = 2 }
82+
83+
override string getConstantName() { result = "NOENT" }
84+
}
85+
86+
private class FeatureNONET extends Feature, TNONET {
87+
override int getValue() { result = 2048 }
88+
89+
override string getConstantName() { result = "NONET" }
90+
}
91+
92+
private class FeatureDTDLOAD extends Feature, TDTDLOAD {
93+
override int getValue() { result = 4 }
94+
95+
override string getConstantName() { result = "DTDLOAD" }
96+
}
97+
98+
private API::Node parseOptionsModule() {
99+
result = API::getTopLevelMember("Nokogiri").getMember("XML").getMember("ParseOptions")
100+
or
101+
result =
102+
API::getTopLevelMember("LibXML").getMember("XML").getMember("Parser").getMember("Options")
103+
or
104+
result = API::getTopLevelMember("XML").getMember("Parser").getMember("Options")
105+
}
106+
107+
private predicate bitWiseAndOr(CfgNodes::ExprNodes::OperationCfgNode operation) {
108+
operation.getExpr() instanceof BitwiseAndExpr or
109+
operation.getExpr() instanceof AssignBitwiseAndExpr or
110+
operation.getExpr() instanceof BitwiseOrExpr or
111+
operation.getExpr() instanceof AssignBitwiseOrExpr
112+
}
113+
114+
private DataFlow::LocalSourceNode trackFeature(Feature f, boolean enable, TypeTracker t) {
115+
t.start() and
116+
(
117+
// An integer literal with the feature-bit enabled/disabled
118+
exists(int bitValue |
119+
bitValue = result.asExpr().getExpr().(IntegerLiteral).getValue().bitAnd(f.getValue())
120+
|
121+
if bitValue = 0 then enable = false else enable = true
122+
)
123+
or
124+
// Use of a constant f
125+
enable = true and
126+
result = parseOptionsModule().getMember(f.getConstantName()).getAUse()
127+
or
128+
// Treat `&`, `&=`, `|` and `|=` operators as if they preserve the on/off states
129+
// of their operands. This is an overapproximation but likely to work well in practice
130+
// because it makes little sense to explicitly set a feature to both `on` and `off` in the
131+
// same code.
132+
exists(CfgNodes::ExprNodes::OperationCfgNode operation |
133+
bitWiseAndOr(operation) and
134+
operation = result.asExpr().(CfgNodes::ExprNodes::OperationCfgNode) and
135+
operation.getAnOperand() = trackFeature(f, enable).asExpr()
136+
)
137+
or
138+
// The complement operator toggles a feature from enabled to disabled and vice-versa
139+
result.asExpr().getExpr() instanceof ComplementExpr and
140+
result.asExpr().(CfgNodes::ExprNodes::OperationCfgNode).getAnOperand() =
141+
trackFeature(f, enable.booleanNot()).asExpr()
142+
or
143+
// Nokogiri has a ParseOptions class that is a wrapper around the bit-fields and
144+
// provides methods for querying and updating the fields.
145+
result =
146+
API::getTopLevelMember("Nokogiri")
147+
.getMember("XML")
148+
.getMember("ParseOptions")
149+
.getAnInstantiation() and
150+
result.asExpr().(CfgNodes::ExprNodes::CallCfgNode).getArgument(0) =
151+
trackFeature(f, enable).asExpr()
152+
or
153+
// The Nokogiri ParseOptions class has methods for setting/unsetting features.
154+
// The method names are the lowercase variants of the constant names, with a "no"
155+
// prefix for unsetting a feature.
156+
exists(CfgNodes::ExprNodes::CallCfgNode call |
157+
enable = true and
158+
call.getExpr().(MethodCall).getMethodName() = f.getConstantName().toLowerCase()
159+
or
160+
enable = false and
161+
call.getExpr().(MethodCall).getMethodName() = "no" + f.getConstantName().toLowerCase()
162+
|
163+
(
164+
// these methods update the receiver
165+
result.flowsTo(any(DataFlow::Node n | n.asExpr() = call.getReceiver()))
166+
or
167+
// in addition they return the (updated) receiver to allow chaining calls.
168+
result.asExpr() = call
169+
)
170+
)
171+
)
172+
or
173+
exists(TypeTracker t2 | result = trackFeature(f, enable, t2).track(t2, t))
174+
}
175+
176+
private DataFlow::Node trackFeature(Feature f, boolean enable) {
177+
trackFeature(f, enable, TypeTracker::end()).flowsTo(result)
178+
}
179+
180+
private DataFlow::Node trackEnableFeature(Feature f) { result = trackFeature(f, true) }
181+
182+
private DataFlow::Node trackDisableFeature(Feature f) { result = trackFeature(f, false) }

‎ql/src/queries/security/cwe-611/Xxe.qhelp

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Parsing untrusted XML files with a weakly configured XML parser may lead to an
7+
XML External Entity (XXE) attack. This type of attack uses external entity references
8+
to access arbitrary files on a system, carry out denial-of-service (DoS) attacks, or server-side
9+
request forgery. Even when the result of parsing is not returned to the user, DoS attacks are still possible
10+
and out-of-band data retrieval techniques may allow attackers to steal sensitive data.
11+
</p>
12+
</overview>
13+
14+
<recommendation>
15+
<p>
16+
The easiest way to prevent XXE attacks is to disable external entity handling when
17+
parsing untrusted data. How this is done depends on the library being used. Note that some
18+
libraries, such as <code>rexml</code>, <code>nokogiri</code> and <code>libxml-ruby</code>,
19+
disable entity expansion by default, so unless you have explicitly enabled entity expansion,
20+
no further action needs to be taken.
21+
</p>
22+
</recommendation>
23+
24+
<example>
25+
<p>
26+
The following example uses the <code>nokogiri</code> XML parser to parse a string <code>xmlSrc</code>.
27+
If that string is from an untrusted source, this code may be vulnerable to an XXE attack, since
28+
the parser is invoked with the <code>noent</code> option set:
29+
</p>
30+
<sample src="examples/Xxe.rb"/>
31+
32+
<p>
33+
To guard against XXE attacks, the <code>noent</code> option should be omitted or cleared
34+
(e.g. using <code>nonoent</code>). This means that no entity expansion is undertaken at all,
35+
not even for standard internal entities such as <code>&amp;amp;</code> or <code>&amp;gt;</code>.
36+
If desired, these entities can be expanded in a separate step using utility functions.
37+
</p>
38+
<sample src="examples/XxeGood.rb"/>
39+
</example>
40+
41+
<references>
42+
<li>
43+
OWASP:
44+
<a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing">XML External Entity (XXE) Processing</a>.
45+
</li>
46+
<li>
47+
Timothy Morgen:
48+
<a href="https://research.nccgroup.com/2014/05/19/xml-schema-dtd-and-entity-attacks-a-compendium-of-known-techniques/">XML Schema, DTD, and Entity Attacks</a>.
49+
</li>
50+
<li>
51+
Timur Yunusov, Alexey Osipov:
52+
<a href="https://www.slideshare.net/qqlan/bh-ready-v4">XML Out-Of-Band Data Retrieval</a>.
53+
</li>
54+
</references>
55+
</qhelp>

‎ql/src/queries/security/cwe-611/Xxe.ql

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/**
2+
* @name XML external entity expansion
3+
* @description Parsing user input as an XML document with external
4+
* entity expansion is vulnerable to XXE attacks.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 9.1
8+
* @precision high
9+
* @id rb/xxe
10+
* @tags security
11+
* external/cwe/cwe-611
12+
* external/cwe/cwe-776
13+
* external/cwe/cwe-827
14+
*/
15+
16+
import ruby
17+
import codeql.ruby.dataflow.RemoteFlowSources
18+
import codeql.ruby.TaintTracking
19+
import codeql.ruby.Concepts
20+
import codeql.ruby.DataFlow
21+
import DataFlow::PathGraph
22+
23+
class UnsafeXxeSink extends DataFlow::ExprNode {
24+
UnsafeXxeSink() {
25+
exists(XmlParserCall parse |
26+
parse.getInput() = this and
27+
parse.externalEntitiesEnabled()
28+
)
29+
}
30+
}
31+
32+
class XxeConfig extends TaintTracking::Configuration {
33+
XxeConfig() { this = "XXE.ql::XxeConfig" }
34+
35+
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
36+
37+
override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeXxeSink }
38+
}
39+
40+
from DataFlow::PathNode source, DataFlow::PathNode sink, XxeConfig conf
41+
where conf.hasFlowPath(source, sink)
42+
select sink.getNode(), source, sink, "Unsafe parsing of XML file from $@.", source.getNode(),
43+
"user input"

‎ql/src/queries/security/cwe-611/examples/Xxe.rb

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
require "nokogiri"
2+
3+
def process_data1
4+
xmlSrc = request.body
5+
doc = Nokogiri::XML.parse(xmlSrc, nil, nil, Nokogiri::XML::ParseOptions::NOENT) # BAD
6+
end
7+
8+
def process_data2
9+
xmlSrc = request.body
10+
doc = Nokogiri::XML.parse(xmlSrc) { |config| config.noent } # BAD
11+
end
12+

‎ql/src/queries/security/cwe-611/examples/XxeGood.rb

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
require "nokogiri"
2+
3+
def process_data1
4+
xmlSrc = request.body
5+
doc = Nokogiri::XML.parse(xmlSrc) # GOOD
6+
end
7+
8+
def process_data2
9+
xmlSrc = request.body
10+
doc = Nokogiri::XML.parse(xmlSrc) { |config| config.nonoent } # GOOD
11+
end
12+

‎ql/test/query-tests/security/cwe-611/LibXmlRuby.rb

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
class LibXmlRubyXXE < ApplicationController
2+
3+
content = params[:xml]
4+
LibXML::XML::Document.string(content, { options: 2 | 2048, encoding: 'utf-8' })
5+
LibXML::XML::Document.file(content, { options: LibXML::XML::Parser::Options::NOENT | 2048})
6+
LibXML::XML::Document.io(content, { options: XML::Parser::Options::NOENT | 2048 })
7+
LibXML::XML::Parser.string(content, { options: 2 | 2048 })
8+
LibXML::XML::Parser.file(content, { options: 3 | 2048 })
9+
LibXML::XML::Parser.io(content, { options: 2 | 2048})
10+
11+
XML::Document.string(content, { options: 2 | 2048 })
12+
XML::Parser.string(content, { options: 2 | 2048 })
13+
14+
LibXML::XML::Parser.file(content, { options: 2048 }) # OK
15+
16+
end

0 commit comments

Comments
 (0)