hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-29 10:45:15 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #12208 from gregxsunday/main

Browse Source 
Add ZipSlip and TarSlip query to ruby
This commit is contained in:
Arthur Baars
2023-03-06 10:40:06 +01:00
committed by GitHub
parent b8e123dc08 48007d14d5
commit d2ab40c184
10 changed files with 513 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

136
ruby/ql/lib/codeql/ruby/experimental/ZipSlipCustomizations.qll Normal file
View File

@@ -0,0 +1,136 @@
/**
* Provides default sources, sinks and sanitizers for reasoning about
* zip slip vulnerabilities, as well as extension points for
* adding your own.
*/
private import codeql.ruby.AST
private import codeql.ruby.ApiGraphs
private import codeql.ruby.CFG
private import codeql.ruby.Concepts
private import codeql.ruby.DataFlow
private import codeql.ruby.dataflow.BarrierGuards
private import codeql.ruby.dataflow.RemoteFlowSources
/**
* Provides default sources, sinks and sanitizers for reasoning about
* zip slip vulnerabilities, as well as extension points for
* adding your own.
*/
module ZipSlip {
/**
* A data flow source for zip slip vulnerabilities.
*/
abstract class Source extends DataFlow::Node { }
/**
* A data flow sink for zip slip vulnerabilities.
*/
abstract class Sink extends DataFlow::Node { }
/**
* A sanitizer for zip slip vulnerabilities.
*/
abstract class Sanitizer extends DataFlow::Node { }
/**
* A file system access, considered as a flow sink.
*/
class FileSystemAccessAsSink extends Sink {
FileSystemAccessAsSink() { this = any(FileSystemAccess e).getAPathArgument() }
}
/**
* A call to `Zlib::GzipReader.open(path)`, considered a flow source.
*/
private class GzipReaderOpen extends Source {
GzipReaderOpen() {
(
this = API::getTopLevelMember("Zlib").getMember("GzipReader").getReturn("open").asSource()
or
this = API::getTopLevelMember("Zlib").getMember("GzipReader").getInstance().asSource()
) and
// If argument refers to a string object, then it's a hardcoded path and
// this file is safe.
not this.(DataFlow::CallNode)
.getArgument(0)
.getALocalSource()
.getConstantValue()
.isStringlikeValue(_)
}
}
/**
* A call to `Gem::Package::TarReader.new(file_stream)`, considered a flow source.
*/
private class TarReaderInstance extends Source {
TarReaderInstance() {
exists(API::MethodAccessNode newTarReader |
newTarReader =
API::getTopLevelMember("Gem").getMember("Package").getMember("TarReader").getMethod("new")
|
// Unlike in two other modules, there's no check for the constant path because TarReader class is called with an `io` object and not filepath.
// This, of course, can be modeled but probably in the internal IO.qll file
// For now, I'm leaving this prone to false-positives
not exists(newTarReader.getBlock()) and this = newTarReader.getReturn().asSource()
or
this = newTarReader.getBlock().getParameter(0).asSource()
)
}
}
/**
* A call to `Zip::File.open(path)`, considered a flow source.
*/
private class ZipFileOpen extends Source {
ZipFileOpen() {
exists(API::MethodAccessNode zipOpen |
zipOpen = API::getTopLevelMember("Zip").getMember("File").getMethod("open") and
// If argument refers to a string object, then it's a hardcoded path and
// this file is safe.
not zipOpen
.getCallNode()
.getArgument(0)
.getALocalSource()
.getConstantValue()
.isStringlikeValue(_)
|
// the case with variable assignment `zip_file = Zip::File.open(path)`
not exists(zipOpen.getBlock()) and this = zipOpen.getReturn().asSource()
or
// the case with direct block`Zip::File.open(path) do |zip_file|` case
this = zipOpen.getBlock().getParameter(0).asSource()
)
}
}
/**
* A comparison with a constant string, considered as a sanitizer-guard.
*/
private class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
/**
* An inclusion check against an array of constant strings, considered as a
* sanitizer-guard.
*/
private class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
StringConstArrayInclusionCallBarrier { }
/**
* A sanitizer like `File.expand_path(path).start_with?` where `path` is a path of a single entry inside the archive.
* It is assumed that if `File.expand_path` is called, it is to verify the path is safe so there's no modeling of `start_with?` or other comparisons to avoid false-negatives.
*/
private class ExpandedPathStartsWithAsSanitizer extends Sanitizer {
ExpandedPathStartsWithAsSanitizer() {
exists(DataFlow::CallNode cn |
cn.getMethodName() = "expand_path" and
this = cn.getArgument(0)
)
}
}
/**
* Existing PathSanitization model created for regular path traversals
*/
private class PathSanitizationAsSanitizer extends Sanitizer instanceof Path::PathSanitization { }
}

38
ruby/ql/lib/codeql/ruby/experimental/ZipSlipQuery.qll Normal file
View File

@@ -0,0 +1,38 @@
/**
* Provides a taint tracking configuration for reasoning about
* zip slip vulnerabilities.
*/
import ZipSlipCustomizations
private import codeql.ruby.Concepts
private import codeql.ruby.DataFlow
private import codeql.ruby.TaintTracking
private import codeql.ruby.ApiGraphs
/**
* A taint-tracking configuration for reasoning about zip slip
* vulnerabilities.
*/
class Configuration extends TaintTracking::Configuration {
Configuration() { this = "ZipSlip" }
override predicate isSource(DataFlow::Node source) { source instanceof ZipSlip::Source }
override predicate isSink(DataFlow::Node sink) { sink instanceof ZipSlip::Sink }
/**
* This should actually be
* `and cn = API::getTopLevelMember("Gem").getMember("Package").getMember("TarReader").getMember("Entry").getAMethodCall("full_name")` and similar for other classes
* 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`
* and `name` for `Zip::File`
*/
override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
exists(DataFlow::CallNode cn |
cn.getReceiver() = nodeFrom and
cn.getMethodName() in ["full_name", "name"] and
cn = nodeTo
)
}
override predicate isSanitizer(DataFlow::Node node) { node instanceof ZipSlip::Sanitizer }
}

4
ruby/ql/src/change-notes/2023-02-17-zip-slip-query.md Normal file
View File

@@ -0,0 +1,4 @@
---
category: newQuery
---
* Added a new query, `rb/zip-slip`, to detect arbitrary file writes during extraction of zip/tar archives.

79
ruby/ql/src/experimental/cwe-022-zipslip/ZipSlip.qhelp Normal file
View File

@@ -0,0 +1,79 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>Extracting files from a malicious tar archive without validating that the destination file path
is within the destination directory can cause files outside the destination directory to be
overwritten, due to the possible presence of directory traversal elements (<code>..</code>) in
archive paths.</p>
<p>Tar archives contain archive entries representing each file in the archive. These entries
include a file path for the entry, but these file paths are not restricted and may contain
unexpected special elements such as the directory traversal element (<code>..</code>). If these
file paths are used to determine an output file to write the contents of the archive item to, then
the file may be written to an unexpected location. This can result in sensitive information being
revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
files.</p>
<p>For example, if a tar archive contains a file entry <code>..\sneaky-file</code>, and the tar archive
is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
written to <code>c:\sneaky-file</code>.</p>
</overview>
<recommendation>
<p>Ensure that output paths constructed from tar archive entries are validated
to prevent writing files to unexpected locations.</p>
<p>The recommended way of writing an output file from a tar archive entry is to check that
<code>".."</code> does not occur in the path.
</p>
</recommendation>
<example>
<p>
In this example an archive is extracted without validating file paths.
If <code>archive.tar</code> contained relative paths (for
instance, if it were created by something like <code>tar -cf archive.tar
../file.txt</code>) then executing this code could write to locations
outside the destination directory.
</p>
<sample src="examples/zip_slip_bad.rb" />
<p>To fix this vulnerability, we need to check that the path does not
contain any <code>".."</code> elements in it.
</p>
<sample src="examples/zip_slip_good.rb" />
</example>
<references>
<li>
Snyk:
<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
</li>
<li>
OWASP:
<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
</li>
<li>
class
<a href="https://docs.ruby-lang.org/en/2.4.0/Gem/Package/TarReader.html">Gem::Package::TarReader</a>.
</li>
<li>
class
<a href="https://ruby-doc.org/stdlib-2.4.0/libdoc/zlib/rdoc/Zlib/GzipReader.html">Zlib::GzipReader</a>.
</li>
<li>
class
<a href="https://www.rubydoc.info/github/rubyzip/rubyzip/Zip/File">Zip::File</a>.
</li>
</references>
</qhelp>

22
ruby/ql/src/experimental/cwe-022-zipslip/ZipSlip.ql Normal file
View File

@@ -0,0 +1,22 @@
/**
* @name Arbitrary file write during zipfile/tarfile extraction
* @description Extracting files from a malicious tar archive without validating that the
* destination file path is within the destination directory can cause files outside
* the destination directory to be overwritten.
* @kind path-problem
* @id rb/zip-slip
* @problem.severity error
* @security-severity 7.5
* @precision medium
* @tags security
* external/cwe/cwe-022
*/
import ruby
import codeql.ruby.experimental.ZipSlipQuery
import DataFlow::PathGraph
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "This file extraction depends on a $@.", source.getNode(),
"potentially untrusted source"

21
ruby/ql/src/experimental/cwe-022-zipslip/examples/zip_slip_bad.rb Normal file
View File

@@ -0,0 +1,21 @@
class FilesController < ActionController::Base
def zipFileUnsafe
path = params[:path]
Zip::File.open(path).each do |entry|
File.open(entry.name, "wb") do |os|
entry.read
end
end
end
def tarReaderUnsafe
path = params[:path]
file_stream = IO.new(IO.sysopen(path))
tarfile = Gem::Package::TarReader.new(file_stream)
tarfile.each do |entry|
::File.open(entry.full_name, "wb") do |os|
entry.read
end
end
end
end

25
ruby/ql/src/experimental/cwe-022-zipslip/examples/zip_slip_good.rb Normal file
View File

@@ -0,0 +1,25 @@
class FilesController < ActionController::Base
def zipFileSafe
path = params[:path]
Zip::File.open(path).each do |entry|
entry_path = entry.name
next if !File.expand_path(entry_path).start_with?('/safepath/')
File.open(entry_path, "wb") do |os|
entry.read
end
end
end
def tarReaderSafe
path = params[:path]
file_stream = IO.new(IO.sysopen(path))
tarfile = Gem::Package::TarReader.new(file_stream)
tarfile.each do |entry|
entry_path = entry.full_name
raise ExtractFailed if entry_path != "/safepath"
::File.open(entry_path, "wb") do |os|
entry.read
end
end
end
end

54
ruby/ql/test/query-tests/experimental/cwe-022-ZipSlip/ZipSlip.expected Normal file
View File

@@ -0,0 +1,54 @@
edges
| zip_slip.rb:8:15:8:54 | call to new : | zip_slip.rb:9:5:9:11 | tarfile : |
| zip_slip.rb:9:5:9:11 | tarfile : | zip_slip.rb:9:22:9:26 | entry : |
| zip_slip.rb:9:22:9:26 | entry : | zip_slip.rb:10:19:10:33 | call to full_name |
| zip_slip.rb:20:50:20:56 | tarfile : | zip_slip.rb:21:7:21:13 | tarfile : |
| zip_slip.rb:21:7:21:13 | tarfile : | zip_slip.rb:21:30:21:34 | entry : |
| zip_slip.rb:21:30:21:34 | entry : | zip_slip.rb:22:21:22:35 | call to full_name |
| zip_slip.rb:46:5:46:24 | call to open : | zip_slip.rb:46:35:46:39 | entry : |
| zip_slip.rb:46:35:46:39 | entry : | zip_slip.rb:47:17:47:26 | call to name |
| zip_slip.rb:56:30:56:37 | zip_file : | zip_slip.rb:57:7:57:14 | zip_file : |
| zip_slip.rb:57:7:57:14 | zip_file : | zip_slip.rb:57:25:57:29 | entry : |
| zip_slip.rb:57:25:57:29 | entry : | zip_slip.rb:58:19:58:28 | call to name |
| zip_slip.rb:90:12:90:54 | call to open : | zip_slip.rb:91:11:91:14 | gzip : |
| zip_slip.rb:91:11:91:14 | gzip : | zip_slip.rb:97:42:97:56 | compressed_file : |
| zip_slip.rb:97:42:97:56 | compressed_file : | zip_slip.rb:98:7:98:21 | compressed_file : |
| zip_slip.rb:98:7:98:21 | compressed_file : | zip_slip.rb:98:32:98:36 | entry : |
| zip_slip.rb:98:32:98:36 | entry : | zip_slip.rb:100:21:100:30 | entry_path |
| zip_slip.rb:123:12:123:34 | call to new : | zip_slip.rb:124:7:124:8 | gz : |
| zip_slip.rb:124:7:124:8 | gz : | zip_slip.rb:124:19:124:23 | entry : |
| zip_slip.rb:124:19:124:23 | entry : | zip_slip.rb:126:21:126:30 | entry_path |
nodes
| zip_slip.rb:8:15:8:54 | call to new : | semmle.label | call to new : |
| zip_slip.rb:9:5:9:11 | tarfile : | semmle.label | tarfile : |
| zip_slip.rb:9:22:9:26 | entry : | semmle.label | entry : |
| zip_slip.rb:10:19:10:33 | call to full_name | semmle.label | call to full_name |
| zip_slip.rb:20:50:20:56 | tarfile : | semmle.label | tarfile : |
| zip_slip.rb:21:7:21:13 | tarfile : | semmle.label | tarfile : |
| zip_slip.rb:21:30:21:34 | entry : | semmle.label | entry : |
| zip_slip.rb:22:21:22:35 | call to full_name | semmle.label | call to full_name |
| zip_slip.rb:46:5:46:24 | call to open : | semmle.label | call to open : |
| zip_slip.rb:46:35:46:39 | entry : | semmle.label | entry : |
| zip_slip.rb:47:17:47:26 | call to name | semmle.label | call to name |
| zip_slip.rb:56:30:56:37 | zip_file : | semmle.label | zip_file : |
| zip_slip.rb:57:7:57:14 | zip_file : | semmle.label | zip_file : |
| zip_slip.rb:57:25:57:29 | entry : | semmle.label | entry : |
| zip_slip.rb:58:19:58:28 | call to name | semmle.label | call to name |
| zip_slip.rb:90:12:90:54 | call to open : | semmle.label | call to open : |
| zip_slip.rb:91:11:91:14 | gzip : | semmle.label | gzip : |
| zip_slip.rb:97:42:97:56 | compressed_file : | semmle.label | compressed_file : |
| zip_slip.rb:98:7:98:21 | compressed_file : | semmle.label | compressed_file : |
| zip_slip.rb:98:32:98:36 | entry : | semmle.label | entry : |
| zip_slip.rb:100:21:100:30 | entry_path | semmle.label | entry_path |
| zip_slip.rb:123:12:123:34 | call to new : | semmle.label | call to new : |
| zip_slip.rb:124:7:124:8 | gz : | semmle.label | gz : |
| zip_slip.rb:124:19:124:23 | entry : | semmle.label | entry : |
| zip_slip.rb:126:21:126:30 | entry_path | semmle.label | entry_path |
subpaths
#select
| 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 |
| zip_slip.rb:22:21:22:35 | call to full_name | zip_slip.rb:20:50:20:56 | tarfile : | zip_slip.rb:22:21:22:35 | call to full_name | This file extraction depends on a $@. | zip_slip.rb:20:50:20:56 | tarfile | potentially untrusted source |
| zip_slip.rb:47:17:47:26 | call to name | zip_slip.rb:46:5:46:24 | call to open : | zip_slip.rb:47:17:47:26 | call to name | This file extraction depends on a $@. | zip_slip.rb:46:5:46:24 | call to open | potentially untrusted source |
| zip_slip.rb:58:19:58:28 | call to name | zip_slip.rb:56:30:56:37 | zip_file : | zip_slip.rb:58:19:58:28 | call to name | This file extraction depends on a $@. | zip_slip.rb:56:30:56:37 | zip_file | potentially untrusted source |
| zip_slip.rb:100:21:100:30 | entry_path | zip_slip.rb:90:12:90:54 | call to open : | zip_slip.rb:100:21:100:30 | entry_path | This file extraction depends on a $@. | zip_slip.rb:90:12:90:54 | call to open | potentially untrusted source |
| zip_slip.rb:126:21:126:30 | entry_path | zip_slip.rb:123:12:123:34 | call to new : | zip_slip.rb:126:21:126:30 | entry_path | This file extraction depends on a $@. | zip_slip.rb:123:12:123:34 | call to new | potentially untrusted source |

1
ruby/ql/test/query-tests/experimental/cwe-022-ZipSlip/ZipSlip.qlref Normal file
View File

@@ -0,0 +1 @@
experimental/cwe-022-zipslip/ZipSlip.ql

133
ruby/ql/test/query-tests/experimental/cwe-022-ZipSlip/zip_slip.rb Normal file
View File

@@ -0,0 +1,133 @@
require 'zip'
class TestController < ActionController::Base
# BAD
def tarReaderUnsafe
path = params[:path]
file_stream = IO.new(IO.sysopen(path))
tarfile = Gem::Package::TarReader.new(file_stream)
tarfile.each do |entry|
::File.open(entry.full_name, "wb") do |os|
entry.read
end
end
end
# BAD
def tarReaderBlockUnsafe
path = params[:path]
file_stream = IO.new(IO.sysopen(path))
Gem::Package::TarReader.new(file_stream) do |tarfile|
tarfile.each_entry do |entry|
::File.open(entry.full_name, "wb") do |os|
entry.read
end
end
end
end
# GOOD
def tarReadeSanitizedExpandPath
path = params[:path]
file_stream = IO.new(IO.sysopen(path))
tarfile = Gem::Package::TarReader.new(file_stream)
tarfile.each do |entry|
entry_path = entry.full_name
next if !File.expand_path(entry_path).start_with?("/safepath/")
::File.open(entry_path, "wb") do |os|
entry.read
end
end
end
# BAD
def zipFileUnsafe
path = params[:path]
Zip::File.open(path).each do |entry|
File.open(entry.name, "wb") do |os|
entry.read
end
end
end
# BAD
def zipFileBlockUnsafe
path = params[:path]
Zip::File.open(path) do |zip_file|
zip_file.each do |entry|
File.open(entry.name, "wb") do |os|
entry.read
end
end
end
end
# GOOD
def zipFileBlockSafeHardcodedPath
path = '/safepath.zip'
Zip::File.open(path) do |zip_file|
zip_file.each do |entry|
File.open(entry.name, "wb") do |os|
entry.read
end
end
end
end
# GOOD
def zipFileSanitizedConstCompare
path = params[:path]
Zip::File.open(path).each do |entry|
entry_path = entry.name
raise ExtractFailed if entry_path != "/safepath"
File.open(entry_path, "wb") do |os|
entry.read
end
end
end
def get_compressed_file_stream(compressed_file_path)
gzip = Zlib::GzipReader.open(compressed_file_path)
yield(gzip)
end
# BAD
def gzipReaderUnsafe
path = params[:path]
get_compressed_file_stream(path) do |compressed_file|
compressed_file.each do |entry|
entry_path = entry.full_name
::File.open(entry_path, 'wb') do |os|
entry.read
end
end
end
end
# GOOD
def gzipReaderSafeConstPath
path = "/safe.zip"
zlib = Zlib::GzipReader.open(path)
zlib.each do |entry|
entry_path = entry.full_name
::File.open(entry_path, 'wb') do |os|
entry.read
end
end
end
# BAD
def gzipReaderUnsafeNewInstance
path = params[:path]
File.open(path, 'rb') do |f|
gz = Zlib::GzipReader.new(f)
gz.each do |entry|
entry_path = entry.full_name
::File.open(entry_path, 'wb') do |os|
entry.read
end
end
end
end
end