Skip to content

Commit 0281dbd

Browse files
thiggy1342thiggy1342
authored
remove Zip::Entry.extract from query
1 parent 540c510 commit 0281dbd

File tree

3 files changed

+25
-65
lines changed

3 files changed

+25
-65
lines changed

‎ruby/ql/src/experimental/decompression-api/DecompressionApi.ql

Lines changed: 16 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,39 +18,26 @@ import codeql.ruby.TaintTracking
1818
import DataFlow::PathGraph
1919

2020
class DecompressionApiUse extends DataFlow::Node {
21-
22-
// this should find the first argument of Zlib::Inflate.inflate or Zip::File.extract
23-
DecompressionApiUse() {
24-
this = API::getTopLevelMember("Zlib").getMember("Inflate").getAMethodCall("inflate").getArgument(0) or
25-
this = API::getTopLevelMember("Zip").getMember("Entry").getAMethodCall("extract").getArgument(0)
26-
}
27-
21+
// this should find the first argument of Zlib::Inflate.inflate or Zip::File.extract
22+
DecompressionApiUse() {
23+
this =
24+
API::getTopLevelMember("Zlib").getMember("Inflate").getAMethodCall("inflate").getArgument(0)
25+
}
2826
}
2927

3028
class Configuration extends TaintTracking::Configuration {
31-
Configuration() { this = "DecompressionApiUse" }
32-
33-
// this predicate will be used to contstrain our query to find instances where only remote user-controlled data flows to the sink
34-
override predicate isSource(DataFlow::Node source) {
35-
source instanceof RemoteFlowSource
36-
}
29+
Configuration() { this = "DecompressionApiUse" }
3730

38-
// our Decompression APIs defined above will the the sinks we use for this query
39-
override predicate isSink(DataFlow::Node sink) {
40-
sink instanceof DecompressionApiUse
41-
}
31+
// this predicate will be used to contstrain our query to find instances where only remote user-controlled data flows to the sink
32+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
4233

43-
// I think it would also be helpful to reduce false positives by adding a simple sanitizer config in the event
44-
// that the code first checks the file name against a string literal or array of string literals before calling
45-
// the decompression API
46-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
47-
guard instanceof StringConstCompare or
48-
guard instanceof StringConstArrayInclusionCall
49-
}
50-
}
51-
34+
// our Decompression APIs defined above will the the sinks we use for this query
35+
override predicate isSink(DataFlow::Node sink) { sink instanceof DecompressionApiUse }
36+
}
5237

5338
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
54-
where
55-
config.hasFlowPath(source, sink)
56-
select sink.getNode(), source, sink, "This call to $@ is unsafe because user-controlled data is used to set the object being decompressed, which could lead to a denial of service attack or malicious code extracted from an unknown source.", sink.getNode().asExpr().getExpr().getParent().toString(), sink.getNode().asExpr().getExpr().getParent().toString()
39+
where config.hasFlowPath(source, sink)
40+
select sink.getNode(), source, sink,
41+
"This call to $@ is unsafe because user-controlled data is used to set the object being decompressed, which could lead to a denial of service attack or malicious code extracted from an unknown source.",
42+
sink.getNode().asExpr().getExpr().getParent(),
43+
sink.getNode().asExpr().getExpr().getParent().toString()

‎ruby/ql/test/query-tests/security/decompression-api/DecompressionApi.expected

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
edges
2-
| decompression_api.rb:3:31:3:36 | call to params : | decompression_api.rb:3:31:3:44 | ...[...] |
3-
| decompression_api.rb:13:44:13:49 | call to params : | decompression_api.rb:13:44:13:57 | ...[...] |
2+
| decompression_api.rb:3:31:3:36 | call to params : | decompression_api.rb:3:31:3:43 | ...[...] |
3+
| decompression_api.rb:13:44:13:49 | call to params : | decompression_api.rb:13:44:13:56 | ...[...] |
44
nodes
55
| decompression_api.rb:3:31:3:36 | call to params : | semmle.label | call to params : |
6-
| decompression_api.rb:3:31:3:44 | ...[...] | semmle.label | ...[...] |
6+
| decompression_api.rb:3:31:3:43 | ...[...] | semmle.label | ...[...] |
77
| decompression_api.rb:13:44:13:49 | call to params : | semmle.label | call to params : |
8-
| decompression_api.rb:13:44:13:57 | ...[...] | semmle.label | ...[...] |
8+
| decompression_api.rb:13:44:13:56 | ...[...] | semmle.label | ...[...] |
99
subpaths
1010
#select
11-
| decompression_api.rb:3:31:3:44 | ...[...] | decompression_api.rb:3:31:3:36 | call to params : | decompression_api.rb:3:31:3:44 | ...[...] | This call to $@ is unsafe because user-controlled data is used to set the object being decompressed, which could lead to a denial of service attack or malicious code extracted from an unknown source. | call to inflate | call to inflate |
12-
| decompression_api.rb:13:44:13:57 | ...[...] | decompression_api.rb:13:44:13:49 | call to params : | decompression_api.rb:13:44:13:57 | ...[...] | This call to $@ is unsafe because user-controlled data is used to set the object being decompressed, which could lead to a denial of service attack or malicious code extracted from an unknown source. | call to inflate | call to inflate |
11+
| decompression_api.rb:3:31:3:43 | ...[...] | decompression_api.rb:3:31:3:36 | call to params : | decompression_api.rb:3:31:3:43 | ...[...] | This call to $@ is unsafe because user-controlled data is used to set the object being decompressed, which could lead to a denial of service attack or malicious code extracted from an unknown source. | decompression_api.rb:3:9:3:44 | call to inflate | call to inflate |
12+
| decompression_api.rb:13:44:13:56 | ...[...] | decompression_api.rb:13:44:13:49 | call to params : | decompression_api.rb:13:44:13:56 | ...[...] | This call to $@ is unsafe because user-controlled data is used to set the object being decompressed, which could lead to a denial of service attack or malicious code extracted from an unknown source. | decompression_api.rb:13:9:13:57 | call to inflate | call to inflate |

‎ruby/ql/test/query-tests/security/decompression-api/decompression_api.rb

Lines changed: 3 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,15 @@
11
class TestController < ActionController::Base
22
def unsafe_zlib_unzip
3-
Zlib::Inflate.inflate(params[:fname])
3+
Zlib::Inflate.inflate(params[:file])
44
end
55

66
def safe_zlib_unzip
7-
Zlib::Inflate.inflate("testfile.gz")
7+
Zlib::Inflate.inflate(file)
88
end
99

1010

1111
DECOMPRESSION_LIB = Zlib
1212
def unsafe_zlib_unzip_const
13-
DECOMPRESSION_LIB::Inflate.inflate(params[:fname])
13+
DECOMPRESSION_LIB::Inflate.inflate(params[:file])
1414
end
15-
16-
def unsafe_zlib_unzip
17-
Zip::File.open(params[:fname]) do |zip_file|
18-
zip_file.each do |entry|
19-
entry.extract(entry.name)
20-
end
21-
end
22-
end
23-
24-
def safe_zlib_unzip
25-
Zip::Entry.extract("testfile.gz")
26-
end
27-
28-
def sanitized_zlib_unzip
29-
fname = params[:fname]
30-
if fname == "safe_file.gz"
31-
Zlib::Inflate.inflate(fname)
32-
end
33-
end
34-
35-
def sanitized_array_zlib_unzip
36-
fname = params[:fname]
37-
if ["safe_file1.gz", "safe_file2.gz"].include? fname
38-
Zlib::Inflate.inflate(fname)
39-
end
40-
end
41-
4215
end

0 commit comments

Comments
 (0)