Skip to content

Commit 525fe12

Browse files
nickrolfenickrolfe
authored
Merge pull request #10585 from github/nickrolfe/libxml-xxe
Ruby: detect uses of LibXML with entity substitution enabled by default
2 parents 1496c4f + 8ca1e1b commit 525fe12

File tree

9 files changed

+134
-0
lines changed

9 files changed

+134
-0
lines changed

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

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

48+
/**
49+
* Holds if `assign` enables the `default_substitute_entities` option in
50+
* libxml-ruby.
51+
*/
52+
private predicate enablesLibXmlDefaultEntitySubstitution(AssignExpr assign) {
53+
// look for an assignment inside:
54+
// XML.class_eval do
55+
// def self.default_substitute_entities
56+
// ...
57+
// end
58+
// end
59+
exists(MethodCall classEvalCall, SingletonMethod defaultSubstituteEntitiesMethod |
60+
classEvalCall.getMethodName() = "class_eval" and
61+
classEvalCall.getReceiver().(ConstantReadAccess).getName() = "XML" and
62+
defaultSubstituteEntitiesMethod.getName() = "default_substitute_entities" and
63+
defaultSubstituteEntitiesMethod.getEnclosingCallable() = classEvalCall.getBlock() and
64+
defaultSubstituteEntitiesMethod = assign.getEnclosingMethod()
65+
) and
66+
// that assignment should be to `default_substitute_entities`.
67+
exists(MethodCall c | c = assign.getLeftOperand() |
68+
c.getMethodName() = "default_substitute_entities" and
69+
c.getReceiver().(ConstantReadAccess).getName() = "XML"
70+
) and
71+
// and the right-hand side should be true
72+
assign.getRightOperand().getConstantValue().isBoolean(true)
73+
}
74+
4875
private class LibXmlRubyXmlParserCall extends XmlParserCall::Range, DataFlow::CallNode {
4976
LibXmlRubyXmlParserCall() {
5077
this =
@@ -66,9 +93,63 @@ private class LibXmlRubyXmlParserCall extends XmlParserCall::Range, DataFlow::Ca
6693
trackDisableFeature(TNONET())
6794
].asExpr()
6895
)
96+
or
97+
// If the database contains a call to set `default_substitute_entities` to
98+
// true, then we assume external entities are enabled for this call
99+
exists(AssignExpr ae | enablesLibXmlDefaultEntitySubstitution(ae))
69100
}
70101
}
71102

103+
/**
104+
* Holds if `call` sets `ActiveSupport::XmlMini.backend` to `"LibXML"`.
105+
*/
106+
private predicate setsXmlMiniBackendToLibXml(DataFlow::CallNode call) {
107+
call = API::getTopLevelMember("ActiveSupport").getMember("XmlMini").getAMethodCall("backend=") and
108+
call.getArgument(0)
109+
.asExpr()
110+
.(CfgNodes::ExprNodes::AssignExprCfgNode)
111+
.getRhs()
112+
.getConstantValue()
113+
.isString("LibXML")
114+
}
115+
116+
/**
117+
* Holds if the database contains a call that sets
118+
* `ActiveSupport::XmlMini.backend` to `"LibXML"` as well as a call to enable
119+
* LibXML's `default_substitute_entities`.
120+
*/
121+
private predicate xmlMiniEntitySubstitutionEnabled() {
122+
setsXmlMiniBackendToLibXml(_) and
123+
enablesLibXmlDefaultEntitySubstitution(_)
124+
}
125+
126+
/**
127+
* A call to `ActiveSupport::XmlMini.parse` considered as an `XmlParserCall`.
128+
*/
129+
private class XmlMiniXmlParserCall extends XmlParserCall::Range, DataFlow::CallNode {
130+
XmlMiniXmlParserCall() {
131+
this = API::getTopLevelMember("ActiveSupport").getMember("XmlMini").getAMethodCall("parse")
132+
}
133+
134+
override DataFlow::Node getInput() { result = this.getArgument(0) }
135+
136+
override predicate externalEntitiesEnabled() { xmlMiniEntitySubstitutionEnabled() }
137+
}
138+
139+
/**
140+
* A call to `Hash.from_xml` or `Hash.from_trusted_xml` considered as an
141+
* `XmlParserCall`.
142+
*/
143+
private class HashFromXmlXmlParserCall extends XmlParserCall::Range, DataFlow::CallNode {
144+
HashFromXmlXmlParserCall() {
145+
this = API::getTopLevelMember("Hash").getAMethodCall(["from_xml", "from_trusted_xml"])
146+
}
147+
148+
override DataFlow::Node getInput() { result = this.getArgument(0) }
149+
150+
override predicate externalEntitiesEnabled() { xmlMiniEntitySubstitutionEnabled() }
151+
}
152+
72153
private newtype TFeature =
73154
TNOENT() or
74155
TNONET() or

‎ruby/ql/src/change-notes/2022-09-27-libxml-xxe.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `rb/xxe` query has been updated to add the following sinks for XML external entity expansion:
5+
1. Calls to parse XML using `LibXML` when its `default_substitute_entities` option is enabled.
6+
2. Uses of the Rails methods `ActiveSupport::XmlMini.parse`, `Hash.from_xml`, and `Hash.from_trusted_xml` when `ActiveSupport::XmlMini` is configured to use `LibXML` as its backend, and its `default_substitute_entities` option is enabled.

‎ruby/ql/test/query-tests/security/cwe-611/libxml-backend/LibXmlBackend.rb

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
require 'xml'
2+
require 'libxml'
3+
4+
# Change the ActiveSupport XML backend from REXML to LibXML
5+
ActiveSupport::XmlMini.backend = 'LibXML'
6+
7+
# Allow entity replacement in LibXML parsing
8+
LibXML::XML.class_eval do
9+
def self.default_substitute_entities
10+
XML.default_substitute_entities = true
11+
end
12+
end
13+
14+
class LibXmlRubyXXE < ApplicationController
15+
def foo
16+
content = params[:xml]
17+
18+
LibXML::XML::Parser.file(content, { options: 2048 })
19+
Hash.from_xml(content)
20+
Hash.from_trusted_xml(content)
21+
ActiveSupport::XmlMini.parse(content)
22+
end
23+
end

‎ruby/ql/test/query-tests/security/cwe-611/libxml-backend/Xxe.expected

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
edges
2+
| LibXmlBackend.rb:16:15:16:20 | call to params : | LibXmlBackend.rb:16:15:16:26 | ...[...] : |
3+
| LibXmlBackend.rb:16:15:16:26 | ...[...] : | LibXmlBackend.rb:18:30:18:36 | content |
4+
| LibXmlBackend.rb:16:15:16:26 | ...[...] : | LibXmlBackend.rb:19:19:19:25 | content |
5+
| LibXmlBackend.rb:16:15:16:26 | ...[...] : | LibXmlBackend.rb:20:27:20:33 | content |
6+
| LibXmlBackend.rb:16:15:16:26 | ...[...] : | LibXmlBackend.rb:21:34:21:40 | content |
7+
nodes
8+
| LibXmlBackend.rb:16:15:16:20 | call to params : | semmle.label | call to params : |
9+
| LibXmlBackend.rb:16:15:16:26 | ...[...] : | semmle.label | ...[...] : |
10+
| LibXmlBackend.rb:18:30:18:36 | content | semmle.label | content |
11+
| LibXmlBackend.rb:19:19:19:25 | content | semmle.label | content |
12+
| LibXmlBackend.rb:20:27:20:33 | content | semmle.label | content |
13+
| LibXmlBackend.rb:21:34:21:40 | content | semmle.label | content |
14+
subpaths
15+
#select
16+
| LibXmlBackend.rb:18:30:18:36 | content | LibXmlBackend.rb:16:15:16:20 | call to params : | LibXmlBackend.rb:18:30:18:36 | content | XML parsing depends on a $@ without guarding against external entity expansion. | LibXmlBackend.rb:16:15:16:20 | call to params | user-provided value |
17+
| LibXmlBackend.rb:19:19:19:25 | content | LibXmlBackend.rb:16:15:16:20 | call to params : | LibXmlBackend.rb:19:19:19:25 | content | XML parsing depends on a $@ without guarding against external entity expansion. | LibXmlBackend.rb:16:15:16:20 | call to params | user-provided value |
18+
| LibXmlBackend.rb:20:27:20:33 | content | LibXmlBackend.rb:16:15:16:20 | call to params : | LibXmlBackend.rb:20:27:20:33 | content | XML parsing depends on a $@ without guarding against external entity expansion. | LibXmlBackend.rb:16:15:16:20 | call to params | user-provided value |
19+
| LibXmlBackend.rb:21:34:21:40 | content | LibXmlBackend.rb:16:15:16:20 | call to params : | LibXmlBackend.rb:21:34:21:40 | content | XML parsing depends on a $@ without guarding against external entity expansion. | LibXmlBackend.rb:16:15:16:20 | call to params | user-provided value |

‎ruby/ql/test/query-tests/security/cwe-611/Xxe.qlref renamed to ‎ruby/ql/test/query-tests/security/cwe-611/libxml-backend/Xxe.qlref

File renamed without changes.

‎ruby/ql/test/query-tests/security/cwe-611/LibXmlRuby.rb renamed to ‎ruby/ql/test/query-tests/security/cwe-611/xxe/LibXmlRuby.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,4 +13,8 @@ class LibXmlRubyXXE < ApplicationController
1313

1414
LibXML::XML::Parser.file(content, { options: 2048 }) # OK
1515

16+
Hash.from_xml(content) # OK - entity substitution is disabled by default
17+
Hash.from_trusted_xml(content) # OK - entity substitution is disabled by default
18+
ActiveSupport::XmlMini.parse(content) # OK - entity substitution is disabled by default
19+
1620
end

‎ruby/ql/test/query-tests/security/cwe-611/Nokogiri.rb renamed to ‎ruby/ql/test/query-tests/security/cwe-611/xxe/Nokogiri.rb

File renamed without changes.

‎ruby/ql/test/query-tests/security/cwe-611/Xxe.expected renamed to ‎ruby/ql/test/query-tests/security/cwe-611/xxe/Xxe.expected

File renamed without changes.

‎ruby/ql/test/query-tests/security/cwe-611/xxe/Xxe.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
queries/security/cwe-611/Xxe.ql

0 commit comments

Comments
 (0)