Skip to content

Commit 089f9d8

Browse files
aibaarsaibaars
committed
Address comments
1 parent 2b07759 commit 089f9d8

File tree

4 files changed

+69
-32
lines changed

4 files changed

+69
-32
lines changed

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

Lines changed: 58 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -90,40 +90,69 @@ private class FeatureDTDLOAD extends Feature, TDTDLOAD {
9090
override string getConstantName() { result = "DTDLOAD" }
9191
}
9292

93+
private API::Node parseOptionsModule() {
94+
result = API::getTopLevelMember("Nokogiri").getMember("XML").getMember("ParseOptions")
95+
or
96+
result = API::getTopLevelMember("LibXML").getMember("XML").getMember("Options")
97+
or
98+
result = API::getTopLevelMember("XML").getMember("Options")
99+
}
100+
93101
private DataFlow::LocalSourceNode trackFeature(Feature f, boolean enable, TypeTracker t) {
94102
t.start() and
95103
(
96-
result.asExpr().getExpr().(IntegerLiteral).getValue().bitAnd(f.getValue()) = f.getValue() and
97-
enable = true
104+
// An integer literal with the feature-bit enabled/disabled
105+
exists(int bitValue |
106+
bitValue = result.asExpr().getExpr().(IntegerLiteral).getValue().bitAnd(f.getValue())
107+
|
108+
if bitValue = 0 then enable = false else enable = true
109+
)
98110
or
111+
// Use of a constant f
99112
enable = true and
100-
result =
101-
API::getTopLevelMember("Nokogiri")
102-
.getMember("XML")
103-
.getMember("ParseOptions")
104-
.getMember(f.getConstantName())
105-
.getAUse()
113+
result = parseOptionsModule().getMember(f.getConstantName()).getAUse()
106114
or
107-
enable = true and
108-
result =
109-
[API::getTopLevelMember("LibXML").getMember("XML"), API::getTopLevelMember("XML")]
110-
.getMember("Options")
111-
.getMember(f.getConstantName())
112-
.getAUse()
115+
// If a feature is enabled in any of the operands of the `|` and `|=` operators
116+
// then the feature is also enabled in the result of the operators.
117+
exists(CfgNodes::ExprNodes::OperationCfgNode operation |
118+
operation = result.asExpr().(CfgNodes::ExprNodes::OperationCfgNode) and
119+
(
120+
operation.getExpr() instanceof BitwiseOrExpr or
121+
operation.getExpr() instanceof AssignBitwiseOrExpr
122+
)
123+
|
124+
enable = true and
125+
operation.getAnOperand() = trackFeature(f, true).asExpr()
126+
or
127+
enable = false and
128+
operation.getAnOperand() = trackFeature(f, false).asExpr() and
129+
forall(DataFlow::Node n | n.asExpr() = operation.getAnOperand() | n != trackFeature(f, true))
130+
)
113131
or
114-
(
115-
result.asExpr().getExpr() instanceof BitwiseOrExpr or
116-
result.asExpr().getExpr() instanceof AssignBitwiseOrExpr or
117-
result.asExpr().getExpr() instanceof BitwiseAndExpr or
118-
result.asExpr().getExpr() instanceof AssignBitwiseAndExpr
119-
) and
120-
result.asExpr().(CfgNodes::ExprNodes::OperationCfgNode).getAnOperand() =
121-
trackFeature(f, enable).asExpr()
132+
// If a feature is disabled in any of the operands of the `&` and `&=` operators
133+
// then the feature is also disabled in the result of the operators.
134+
exists(CfgNodes::ExprNodes::OperationCfgNode operation |
135+
operation = result.asExpr().(CfgNodes::ExprNodes::OperationCfgNode) and
136+
(
137+
operation.getExpr() instanceof BitwiseAndExpr or
138+
operation.getExpr() instanceof AssignBitwiseAndExpr
139+
)
140+
|
141+
enable = false and
142+
operation.getAnOperand() = trackFeature(f, false).asExpr()
143+
or
144+
enable = true and
145+
operation.getAnOperand() = trackFeature(f, true).asExpr() and
146+
forall(DataFlow::Node n | n.asExpr() = operation.getAnOperand() | n != trackFeature(f, false))
147+
)
122148
or
149+
// The complement operator toggles a feature from enabled to disabled and vice-versa
123150
result.asExpr().getExpr() instanceof ComplementExpr and
124151
result.asExpr().(CfgNodes::ExprNodes::OperationCfgNode).getAnOperand() =
125152
trackFeature(f, enable.booleanNot()).asExpr()
126153
or
154+
// Nokogiri has a ParseOptions class that is a wrapper around the bit-fields and
155+
// provides methods for querying and updating the fields.
127156
result =
128157
API::getTopLevelMember("Nokogiri")
129158
.getMember("XML")
@@ -132,6 +161,9 @@ private DataFlow::LocalSourceNode trackFeature(Feature f, boolean enable, TypeTr
132161
result.asExpr().(CfgNodes::ExprNodes::CallCfgNode).getArgument(0) =
133162
trackFeature(f, enable).asExpr()
134163
or
164+
// The Nokogiri ParseOptions class has methods for setting/unsetting features.
165+
// The method names are the lowercase variants of the constant names, with a "no"
166+
// prefix for unsetting a feature.
135167
exists(CfgNodes::ExprNodes::CallCfgNode call |
136168
enable = true and
137169
call.getExpr().(MethodCall).getMethodName() = f.getConstantName().toLowerCase()
@@ -140,16 +172,13 @@ private DataFlow::LocalSourceNode trackFeature(Feature f, boolean enable, TypeTr
140172
call.getExpr().(MethodCall).getMethodName() = "no" + f.getConstantName().toLowerCase()
141173
|
142174
(
143-
result.asExpr() = call
144-
or
175+
// these methods update the receiver
145176
result.flowsTo(any(DataFlow::Node n | n.asExpr() = call.getReceiver()))
177+
or
178+
// in addition they return the (updated) receiver to allow chaining calls.
179+
result.asExpr() = call
146180
)
147181
)
148-
or
149-
exists(CfgNodes::ExprNodes::CallCfgNode call |
150-
trackFeature(f, enable).asExpr() = call.getReceiver() and
151-
result.asExpr() = call
152-
)
153182
)
154183
or
155184
exists(TypeTracker t2 | result = trackFeature(f, enable, t2).track(t2, t))

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,6 @@ class LibXmlRubyXXE < ApplicationController
1111
XML::Document.string(content, { options: 2 })
1212
XML::Parser.string(content, { options: 2 })
1313

14-
LibXML::XML::Parser.file(content, { options: 1 }) # OK
14+
LibXML::XML::Parser.file(content, { options: 2048 }) # OK
1515

1616
end

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,15 @@ class NokogiriXXE < ApplicationController
1616
Nokogiri::XML::parse(content, nil, nil, (Nokogiri::XML::ParseOptions.new 0).noent)
1717

1818
Nokogiri::XML::parse(content) { |x| x.noent }
19-
Nokogiri::XML::parse(content) { |x| x.nononet }
19+
Nokogiri::XML::parse(content) { |x| x.nononet } #FAIL
2020
Nokogiri::XML::parse(content) { |x| x.nodtdload } # OK
2121

2222
Nokogiri::XML::parse(content) { |x| x.nonet.noent.nodtdload }
2323

24-
Nokogiri::XML::parse(content, nil, nil, 1) # OK
24+
Nokogiri::XML::parse(content, nil, nil, 2048) # OK
2525
Nokogiri::XML::parse(content, nil, nil, 3)
2626
Nokogiri::XML::parse(content) { |x| x.nonet.nodtdload } # OK
27+
Nokogiri::XML::parse(content, nil, nil, Nokogiri::XML::ParseOptions::NOENT & ~Nokogiri::XML::ParseOptions::NOBLANKS)
28+
Nokogiri::XML::parse(content, nil, nil, ~Nokogiri::XML::ParseOptions::NONET | Nokogiri::XML::ParseOptions::NOBLANKS)
2729

2830
end

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ edges
2020
| Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:19:26:19:32 | content |
2121
| Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:22:26:22:32 | content |
2222
| Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:25:26:25:32 | content |
23+
| Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:27:26:27:32 | content |
24+
| Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:28:26:28:32 | content |
2325
nodes
2426
| LibXmlRuby.rb:3:15:3:20 | call to params : | semmle.label | call to params : |
2527
| LibXmlRuby.rb:4:34:4:40 | content | semmle.label | content |
@@ -44,6 +46,8 @@ nodes
4446
| Nokogiri.rb:19:26:19:32 | content | semmle.label | content |
4547
| Nokogiri.rb:22:26:22:32 | content | semmle.label | content |
4648
| Nokogiri.rb:25:26:25:32 | content | semmle.label | content |
49+
| Nokogiri.rb:27:26:27:32 | content | semmle.label | content |
50+
| Nokogiri.rb:28:26:28:32 | content | semmle.label | content |
4751
subpaths
4852
#select
4953
| LibXmlRuby.rb:4:34:4:40 | content | LibXmlRuby.rb:3:15:3:20 | call to params : | LibXmlRuby.rb:4:34:4:40 | content | Unsafe parsing of XML file from $@. | LibXmlRuby.rb:3:15:3:20 | call to params | user input |
@@ -67,3 +71,5 @@ subpaths
6771
| Nokogiri.rb:19:26:19:32 | content | Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:19:26:19:32 | content | Unsafe parsing of XML file from $@. | Nokogiri.rb:3:15:3:20 | call to params | user input |
6872
| Nokogiri.rb:22:26:22:32 | content | Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:22:26:22:32 | content | Unsafe parsing of XML file from $@. | Nokogiri.rb:3:15:3:20 | call to params | user input |
6973
| Nokogiri.rb:25:26:25:32 | content | Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:25:26:25:32 | content | Unsafe parsing of XML file from $@. | Nokogiri.rb:3:15:3:20 | call to params | user input |
74+
| Nokogiri.rb:27:26:27:32 | content | Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:27:26:27:32 | content | Unsafe parsing of XML file from $@. | Nokogiri.rb:3:15:3:20 | call to params | user input |
75+
| Nokogiri.rb:28:26:28:32 | content | Nokogiri.rb:3:15:3:20 | call to params : | Nokogiri.rb:28:26:28:32 | content | Unsafe parsing of XML file from $@. | Nokogiri.rb:3:15:3:20 | call to params | user input |

0 commit comments

Comments
 (0)