Skip to content

Commit d1aaa9a

Browse files
gregxsundaygregxsunday
committed
Add ZipSlip/TarSlip query for ruby
1 parent 3b1b3b4 commit d1aaa9a

File tree

9 files changed

+428
-0
lines changed

9 files changed

+428
-0
lines changed

‎ruby/ql/lib/codeql/ruby/security/ZipSlipCustomizations.qll

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* zip slip vulnerabilities, as well as extension points for
4+
* adding your own.
5+
*/
6+
7+
private import codeql.ruby.AST
8+
private import codeql.ruby.ApiGraphs
9+
private import codeql.ruby.CFG
10+
private import codeql.ruby.Concepts
11+
private import codeql.ruby.DataFlow
12+
private import codeql.ruby.dataflow.BarrierGuards
13+
private import codeql.ruby.dataflow.RemoteFlowSources
14+
15+
module ZipSlip {
16+
/**
17+
* A data flow source for zip slip vulnerabilities.
18+
*/
19+
abstract class Source extends DataFlow::Node { }
20+
21+
/**
22+
* A data flow sink for zip slip vulnerabilities.
23+
*/
24+
abstract class Sink extends DataFlow::Node { }
25+
26+
/**
27+
* A sanitizer for zip slip vulnerabilities.
28+
*/
29+
abstract class Sanitizer extends DataFlow::Node { }
30+
31+
/**
32+
* A file system access, considered as a flow sink.
33+
*/
34+
class FileSystemAccessAsSink extends Sink {
35+
FileSystemAccessAsSink() { this = any(FileSystemAccess e).getAPathArgument() }
36+
}
37+
38+
/**
39+
* A call to `Zlib::GzipReader.open(path)`, considered a flow source.
40+
*/
41+
class GzipReaderOpen extends Source {
42+
GzipReaderOpen() {
43+
this = API::getTopLevelMember("Zlib").getMember("GzipReader").getReturn("open").asSource() and
44+
// If argument refers to a string object, then it's a hardcoded path and
45+
// this file is safe.
46+
not this.(DataFlow::CallNode)
47+
.getArgument(0)
48+
.getALocalSource()
49+
.getConstantValue()
50+
.isStringlikeValue(_)
51+
}
52+
}
53+
54+
/**
55+
* A call to `Gem::Package::TarReader.new(file_stream)`, considered a flow source.
56+
*/
57+
class TarReaderInstance extends Source {
58+
TarReaderInstance() {
59+
this =
60+
API::getTopLevelMember("Gem")
61+
.getMember("Package")
62+
.getMember("TarReader")
63+
.getInstance()
64+
.asSource() and
65+
// If argument refers to a string object, then it's a hardcoded path and
66+
// this file is safe.
67+
not this.(DataFlow::CallNode)
68+
.getArgument(0)
69+
.getALocalSource()
70+
.getConstantValue()
71+
.isStringlikeValue(_)
72+
}
73+
}
74+
75+
/**
76+
* A call to `Zip::File.open(path)`, considered a flow source.
77+
*/
78+
class ZipFileOpen extends Source {
79+
ZipFileOpen() {
80+
this = API::getTopLevelMember("Zip").getMember("File").getReturn("open").asSource() and
81+
// If argument refers to a string object, then it's a hardcoded path and
82+
// this file is safe.
83+
not this.(DataFlow::CallNode)
84+
.getArgument(0)
85+
.getALocalSource()
86+
.getConstantValue()
87+
.isStringlikeValue(_)
88+
}
89+
}
90+
91+
/**
92+
* A comparison with a constant string, considered as a sanitizer-guard.
93+
*/
94+
class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
95+
96+
/**
97+
* An inclusion check against an array of constant strings, considered as a
98+
* sanitizer-guard.
99+
*/
100+
class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
101+
StringConstArrayInclusionCallBarrier { }
102+
103+
/**
104+
* A sanitizer like `File.expand_path(path).start_with?` where `path` is a path of a single entry inside the archive.
105+
* It is assumed that if `File.expand_path` is called, it is to verify the path is safe so there's no modelling of `start_with?` or other comparisons to avoid false-negatives.
106+
*/
107+
class ExpandedPathStartsWithAsSanitizer extends Sanitizer {
108+
ExpandedPathStartsWithAsSanitizer() {
109+
exists(DataFlow::CallNode cn |
110+
cn.getMethodName() = "expand_path" and
111+
this = cn.getArgument(0)
112+
)
113+
}
114+
}
115+
}

‎ruby/ql/lib/codeql/ruby/security/ZipSlipQuery.qll

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about
3+
* zip slip vulnerabilities.
4+
*/
5+
6+
import ZipSlipCustomizations
7+
private import codeql.ruby.Concepts
8+
private import codeql.ruby.DataFlow
9+
private import codeql.ruby.TaintTracking
10+
private import codeql.ruby.ApiGraphs
11+
12+
/**
13+
* A taint-tracking configuration for reasoning about zip slip
14+
* vulnerabilities.
15+
*/
16+
class Configuration extends TaintTracking::Configuration {
17+
Configuration() { this = "ZipSlip" }
18+
19+
override predicate isSource(DataFlow::Node source) { source instanceof ZipSlip::Source }
20+
21+
override predicate isSink(DataFlow::Node sink) { sink instanceof ZipSlip::Sink }
22+
23+
/**
24+
* This should actually be
25+
* `and cn = API::getTopLevelMember("Gem").getMember("Package").getMember("TarReader").getMember("Entry").getAMethodCall("full_name")` and similar for other classes
26+
* but I couldn't make it work so there's only check for the method name called on the entry. It is `full_name` for `Gem::Package::TarReader::Entry` and `Zlib`
27+
* and `name` for `Zip::File`
28+
*/
29+
override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
30+
exists(DataFlow::CallNode cn |
31+
cn.getReceiver() = nodeFrom and
32+
cn.getMethodName() in ["full_name", "name"] and
33+
cn = nodeTo
34+
)
35+
}
36+
37+
override predicate isSanitizer(DataFlow::Node node) {
38+
node instanceof ZipSlip::Sanitizer or node instanceof Path::PathSanitization
39+
}
40+
}

‎ruby/ql/src/queries/security/cwe-022/ZipSlip.qhelp

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>Extracting files from a malicious tar archive without validating that the destination file path
8+
is within the destination directory can cause files outside the destination directory to be
9+
overwritten, due to the possible presence of directory traversal elements (<code>..</code>) in
10+
archive paths.</p>
11+
12+
<p>Tar archives contain archive entries representing each file in the archive. These entries
13+
include a file path for the entry, but these file paths are not restricted and may contain
14+
unexpected special elements such as the directory traversal element (<code>..</code>). If these
15+
file paths are used to determine an output file to write the contents of the archive item to, then
16+
the file may be written to an unexpected location. This can result in sensitive information being
17+
revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
18+
files.</p>
19+
20+
<p>For example, if a tar archive contains a file entry <code>..\sneaky-file</code>, and the tar archive
21+
is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
22+
in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
23+
written to <code>c:\sneaky-file</code>.</p>
24+
25+
</overview>
26+
<recommendation>
27+
28+
<p>Ensure that output paths constructed from tar archive entries are validated
29+
to prevent writing files to unexpected locations.</p>
30+
31+
<p>The recommended way of writing an output file from a tar archive entry is to check that
32+
<code>".."</code> does not occur in the path.
33+
</p>
34+
35+
</recommendation>
36+
37+
<example>
38+
<p>
39+
In this example an archive is extracted without validating file paths.
40+
If <code>archive.tar</code> contained relative paths (for
41+
instance, if it were created by something like <code>tar -cf archive.tar
42+
../file.txt</code>) then executing this code could write to locations
43+
outside the destination directory.
44+
</p>
45+
46+
<sample src="examples/zip_slip_bad.py" />
47+
48+
<p>To fix this vulnerability, we need to check that the path does not
49+
contain any <code>".."</code> elements in it.
50+
</p>
51+
52+
<sample src="examples/zip_slip_good.py" />
53+
54+
</example>
55+
<references>
56+
57+
<li>
58+
Snyk:
59+
<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
60+
</li>
61+
<li>
62+
OWASP:
63+
<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
64+
</li>
65+
<li>
66+
Python Library Reference:
67+
<a href="https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extract">TarFile.extract</a>.
68+
</li>
69+
<li>
70+
Python Library Reference:
71+
<a href="https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extractall">TarFile.extractall</a>.
72+
</li>
73+
74+
</references>
75+
</qhelp>

‎ruby/ql/src/queries/security/cwe-022/ZipSlip.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Arbitrary file write during zipfile/tarfile extraction
3+
* @description Extracting files from a malicious tar archive without validating that the
4+
* destination file path is within the destination directory can cause files outside
5+
* the destination directory to be overwritten.
6+
* @kind path-problem
7+
* @id rb/zip-slip
8+
* @problem.severity error
9+
* @security-severity 7.5
10+
* @precision medium
11+
* @tags security
12+
* external/cwe/cwe-022
13+
*/
14+
15+
import ruby
16+
import codeql.ruby.security.ZipSlipQuery
17+
import DataFlow::PathGraph
18+
19+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
20+
where cfg.hasFlowPath(source, sink)
21+
select sink.getNode(), source, sink, "This file extraction depends on a $@.", source.getNode(),
22+
"potentially untrusted source"

‎ruby/ql/src/queries/security/cwe-022/examples/zip_slip_bad.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
class FilesController < ActionController::Base
2+
def zipFileUnsafe
3+
path = params[:path]
4+
Zip::File.open(path).each do |entry|
5+
File.open(entry.name, "wb") do |os|
6+
entry.read
7+
end
8+
end
9+
end
10+
11+
def tarReaderUnsafe
12+
path = params[:path]
13+
file_stream = IO.new(IO.sysopen(path))
14+
tarfile = Gem::Package::TarReader.new(file_stream)
15+
tarfile.each do |entry|
16+
::File.open(entry.full_name, "wb") do |os|
17+
entry.read
18+
end
19+
end
20+
end
21+
end

‎ruby/ql/src/queries/security/cwe-022/examples/zip_slip_good.rb

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
class FilesController < ActionController::Base
2+
def zipFileSafe
3+
path = params[:path]
4+
Zip::File.open(path).each do |entry|
5+
entry_path = entry.name
6+
next if !File.expand_path(entry_path).start_with?('/safepath/')
7+
File.open(entry_path, "wb") do |os|
8+
entry.read
9+
end
10+
end
11+
end
12+
13+
def tarReaderSafe
14+
path = params[:path]
15+
file_stream = IO.new(IO.sysopen(path))
16+
tarfile = Gem::Package::TarReader.new(file_stream)
17+
tarfile.each do |entry|
18+
entry_path = entry.full_name
19+
raise ExtractFailed if entry_path != "/safepath"
20+
::File.open(entry_path, "wb") do |os|
21+
entry.read
22+
end
23+
end
24+
end
25+
end

‎ruby/ql/test/query-tests/security/cwe-022-ZipSlip/ZipSlip.expected

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
edges
2+
| zip_slip.rb:8:15:8:54 | call to new : | zip_slip.rb:9:5:9:11 | tarfile : |
3+
| zip_slip.rb:9:5:9:11 | tarfile : | zip_slip.rb:9:22:9:26 | entry : |
4+
| zip_slip.rb:9:22:9:26 | entry : | zip_slip.rb:10:19:10:33 | call to full_name |
5+
| zip_slip.rb:33:5:33:24 | call to open : | zip_slip.rb:33:35:33:39 | entry : |
6+
| zip_slip.rb:33:35:33:39 | entry : | zip_slip.rb:34:17:34:26 | call to name |
7+
| zip_slip.rb:53:12:53:54 | call to open : | zip_slip.rb:54:11:54:14 | gzip : |
8+
| zip_slip.rb:54:11:54:14 | gzip : | zip_slip.rb:60:42:60:56 | compressed_file : |
9+
| zip_slip.rb:60:42:60:56 | compressed_file : | zip_slip.rb:61:7:61:21 | compressed_file : |
10+
| zip_slip.rb:61:7:61:21 | compressed_file : | zip_slip.rb:61:32:61:36 | entry : |
11+
| zip_slip.rb:61:32:61:36 | entry : | zip_slip.rb:63:21:63:30 | entry_path |
12+
nodes
13+
| zip_slip.rb:8:15:8:54 | call to new : | semmle.label | call to new : |
14+
| zip_slip.rb:9:5:9:11 | tarfile : | semmle.label | tarfile : |
15+
| zip_slip.rb:9:22:9:26 | entry : | semmle.label | entry : |
16+
| zip_slip.rb:10:19:10:33 | call to full_name | semmle.label | call to full_name |
17+
| zip_slip.rb:33:5:33:24 | call to open : | semmle.label | call to open : |
18+
| zip_slip.rb:33:35:33:39 | entry : | semmle.label | entry : |
19+
| zip_slip.rb:34:17:34:26 | call to name | semmle.label | call to name |
20+
| zip_slip.rb:53:12:53:54 | call to open : | semmle.label | call to open : |
21+
| zip_slip.rb:54:11:54:14 | gzip : | semmle.label | gzip : |
22+
| zip_slip.rb:60:42:60:56 | compressed_file : | semmle.label | compressed_file : |
23+
| zip_slip.rb:61:7:61:21 | compressed_file : | semmle.label | compressed_file : |
24+
| zip_slip.rb:61:32:61:36 | entry : | semmle.label | entry : |
25+
| zip_slip.rb:63:21:63:30 | entry_path | semmle.label | entry_path |
26+
subpaths
27+
#select
28+
| zip_slip.rb:10:19:10:33 | call to full_name | zip_slip.rb:8:15:8:54 | call to new : | zip_slip.rb:10:19:10:33 | call to full_name | This file extraction depends on a $@. | zip_slip.rb:8:15:8:54 | call to new | potentially untrusted source |
29+
| zip_slip.rb:34:17:34:26 | call to name | zip_slip.rb:33:5:33:24 | call to open : | zip_slip.rb:34:17:34:26 | call to name | This file extraction depends on a $@. | zip_slip.rb:33:5:33:24 | call to open | potentially untrusted source |
30+
| zip_slip.rb:63:21:63:30 | entry_path | zip_slip.rb:53:12:53:54 | call to open : | zip_slip.rb:63:21:63:30 | entry_path | This file extraction depends on a $@. | zip_slip.rb:53:12:53:54 | call to open | potentially untrusted source |

‎ruby/ql/test/query-tests/security/cwe-022-ZipSlip/ZipSlip.qlref

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

0 commit comments

Comments
 (0)