Skip to content

Commit 2097a00

Browse files
am0o0am0o0
committed
apply code review suggestions, fix qldoc, add experimental additional taint steps that can improve performance
1 parent 2e4e5ef commit 2097a00

File tree

3 files changed

+85
-47
lines changed

3 files changed

+85
-47
lines changed

‎ruby/ql/src/experimental/CWE-522-DecompressionBombs/DecompressionBombs.qhelp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
<p>When you want to decompress a user-provided compressed file you must be careful about the decompression ratio or read these files within a loop byte by byte to be able to manage the decompressed size in each cycle of the loop.</p>
1313

1414
</recommendation>
15-
<p>Please read official RubyZip Documentation <a href="https://github.com/rubyzip/rubyzip/#size-validation">here</a>
15+
<p>Please read official RubyZip Documentation <a href="https://github.com/rubyzip/rubyzip/#size-validation">here</a></p>
1616
<example>
1717
<p>Rubyzip: According to <a href="https://github.com/rubyzip/rubyzip/#reading-a-zip-file">official</a> Documentation</p>
1818
<sample src="example_good.rb" />

‎ruby/ql/src/experimental/CWE-522-DecompressionBombs/DecompressionBombs.ql

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
* external/cwe/cwe-409
1212
*/
1313

14+
import ruby
1415
private import codeql.ruby.Concepts
1516
private import codeql.ruby.DataFlow
1617
private import codeql.ruby.TaintTracking
@@ -26,40 +27,31 @@ class IoCopyStream extends DataFlow::CallNode {
2627
}
2728

2829
module BombsConfig implements DataFlow::ConfigSig {
29-
predicate isBarrier(DataFlow::Node node) {
30-
node.getLocation().hasLocationInfo("%spec%", _, _, _, _)
31-
}
32-
3330
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
3431

3532
predicate isSink(DataFlow::Node sink) { sink instanceof DecompressionBomb::Sink }
3633

3734
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
3835
any(DecompressionBomb::AdditionalTaintStep ats).isAdditionalTaintStep(nodeFrom, nodeTo)
3936
or
40-
exists(API::Node n | n = API::root().getMember("File").getMethod("open") |
37+
exists(API::Node n | n = API::getTopLevelMember("File").getMethod("open") |
4138
nodeFrom = n.getParameter(0).asSink() and
4239
nodeTo = n.getReturn().asSource()
4340
)
4441
or
45-
exists(File::FileOpen n |
46-
nodeFrom = n.getAPathArgument() and
47-
nodeTo = n
48-
)
42+
nodeFrom = nodeTo.(File::FileOpen).getAPathArgument()
4943
or
50-
exists(API::Node n | n = API::root().getMember("StringIO").getMethod("new") |
44+
exists(API::Node n | n = API::getTopLevelMember("StringIO").getMethod("new") |
5145
nodeFrom = n.getParameter(0).asSink() and
5246
nodeTo = n.getReturn().asSource()
5347
)
5448
or
55-
exists(IoCopyStream n |
56-
nodeFrom = n.getAPathArgument() and
57-
nodeTo = n
58-
)
49+
nodeFrom = nodeTo.(IoCopyStream).getAPathArgument()
5950
or
6051
// following can be a global additional step
6152
exists(DataFlow::CallNode cn |
62-
cn.getMethodName() = "open" and cn.getReceiver().toString() = "self"
53+
cn.getMethodName() = "open" and
54+
cn.getReceiver().getExprNode().getExpr() instanceof Ast::SelfVariableAccess
6355
|
6456
nodeFrom = cn.getArgument(0) and
6557
nodeTo = cn

‎ruby/ql/src/experimental/CWE-522-DecompressionBombs/DecompressionBombs.qll

Lines changed: 77 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,12 @@ import codeql.ruby.dataflow.RemoteFlowSources
66

77
module DecompressionBomb {
88
/**
9-
* The Sinks of uncontrolled data decompression
9+
* A abstract class responsible for extending new decompression sinks
10+
*
11+
* can be a path, stream of compressed data,
12+
* or a call to function that use pipe
1013
*/
11-
class Sink extends DataFlow::Node {
12-
Sink() { this = any(Range r).sink() }
13-
}
14+
abstract class Sink extends DataFlow::Node { }
1415

1516
/**
1617
* The additional taint steps that need for creating taint tracking or dataflow.
@@ -23,19 +24,6 @@ module DecompressionBomb {
2324
*/
2425
abstract predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ);
2526
}
26-
27-
/**
28-
* A abstract class responsible for extending new decompression sinks
29-
*/
30-
abstract class Range extends API::Node {
31-
/**
32-
* Gets the sink of responsible for decompression node
33-
*
34-
* it can be a path, stream of compressed data,
35-
* or a call to function that use pipe
36-
*/
37-
abstract DataFlow::Node sink();
38-
}
3927
}
4028

4129
module Zlib {
@@ -54,10 +42,10 @@ module Zlib {
5442
* `Zlib::GzipReader.zcat`
5543
* `Zlib::GzipReader.new`
5644
*/
57-
class DecompressionBombSink extends DecompressionBomb::Range {
58-
DecompressionBombSink() { this = gzipReaderInstance().getMethod(["open", "new", "zcat"]) }
59-
60-
override DataFlow::Node sink() { result = this.getReturn().asSource() }
45+
class DecompressionBombSink extends DecompressionBomb::Sink {
46+
DecompressionBombSink() {
47+
this = gzipReaderInstance().getMethod(["open", "new", "zcat"]).getReturn().asSource()
48+
}
6149
}
6250

6351
/**
@@ -94,10 +82,10 @@ module ZipInputStream {
9482
*
9583
* as source of decompression bombs, they need an additional taint step for a dataflow or taint tracking query
9684
*/
97-
class DecompressionBombSink extends DecompressionBomb::Range {
98-
DecompressionBombSink() { this = zipInputStream().getMethod(["open", "new"]) }
99-
100-
override DataFlow::Node sink() { result = this.getReturn().asSource() }
85+
class DecompressionBombSink extends DecompressionBomb::Sink {
86+
DecompressionBombSink() {
87+
this = zipInputStream().getMethod(["open", "new"]).getReturn().asSource()
88+
}
10189
}
10290

10391
/**
@@ -144,12 +132,14 @@ module ZipFile {
144132
* `ZipEntry.extract`
145133
* A sanitizer exists inside the nodes which have `entry.size > someOBJ`
146134
*/
147-
class DecompressionBombSink extends DecompressionBomb::Range {
148-
DecompressionBombSink() { this = rubyZipNode(zipFile()) }
149-
150-
override DataFlow::Node sink() {
151-
result = this.getMethod(["extract", "read"]).getReturn().asSource() and
152-
not exists(this.getMethod("size").getReturn().getMethod(">").getParameter(0))
135+
class DecompressionBombSink extends DecompressionBomb::Sink {
136+
DecompressionBombSink() {
137+
exists(API::Node rubyZip | rubyZip = rubyZipNode(zipFile()) |
138+
this = rubyZip.getMethod(["extract", "read"]).getReturn().asSource() and
139+
not exists(
140+
rubyZip.getMethod("size").getReturn().getMethod([">", " <", "<=", " >="]).getParameter(0)
141+
)
142+
)
153143
}
154144
}
155145

@@ -166,4 +156,60 @@ module ZipFile {
166156
)
167157
}
168158
}
159+
160+
predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
161+
exists(API::Node zipFile | zipFile = zipFile().getMethod("open") |
162+
nodeFrom = zipFile.getParameter(0).asSink() and
163+
nodeTo = rubyZipNode(zipFile).getMethod(["extract", "read"]).getReturn().asSource()
164+
)
165+
}
166+
// /**
167+
// * The additional taint steps that need for creating taint tracking or dataflow for `Zip::File`.
168+
// */
169+
// class AdditionalTaintStep1 extends DecompressionBomb::AdditionalTaintStep {
170+
// AdditionalTaintStep1() { this = "AdditionalTaintStep" }
171+
// override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
172+
// exists(API::Node zipFile | zipFile = zipFile().getMethod("open") |
173+
// nodeFrom = zipFile.getParameter(0).asSink() and
174+
// nodeTo = zipFile.asSource()
175+
// )
176+
// }
177+
// }
178+
// API::Node rubyZipNode2() {
179+
// result = zipFile().getMethod("open")
180+
// or
181+
// result = rubyZipNode2().getMethod(_)
182+
// or
183+
// result = rubyZipNode2().getReturn()
184+
// or
185+
// result = rubyZipNode2().getParameter(_)
186+
// or
187+
// result = rubyZipNode2().getAnElement()
188+
// or
189+
// result = rubyZipNode2().getBlock()
190+
// }
191+
// /**
192+
// * The additional taint steps that need for creating taint tracking or dataflow for `Zip::File`.
193+
// */
194+
// class AdditionalTaintStep2 extends DecompressionBomb::AdditionalTaintStep {
195+
// AdditionalTaintStep2() { this = "AdditionalTaintStep" }
196+
// override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
197+
// exists(API::Node zipFileOpen | zipFileOpen = rubyZipNode2() |
198+
// nodeFrom = zipFileOpen.getReturn().asSource() and
199+
// nodeTo = zipFileOpen.getMethod(["extract", "read"]).getReturn().asSource()
200+
// )
201+
// }
202+
// }
203+
// /**
204+
// * The additional taint steps that need for creating taint tracking or dataflow for `Zip::File`.
205+
// */
206+
// class AdditionalTaintStep3 extends DecompressionBomb::AdditionalTaintStep {
207+
// AdditionalTaintStep3() { this = "AdditionalTaintStep" }
208+
// override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
209+
// exists(API::Node zipFileOpen | zipFileOpen = rubyZipNode2() |
210+
// nodeFrom = zipFileOpen.asCall() and
211+
// nodeTo = zipFileOpen.getMethod(["extract", "read"]).getReturn().asSource()
212+
// )
213+
// }
214+
// }
169215
}

0 commit comments

Comments
 (0)