mirror of
https://github.com/github/codeql.git
synced 2026-04-29 02:35:15 +02:00
Add ZipSlip/TarSlip query for ruby
This commit is contained in:
gregxsunday
115
ruby/ql/lib/codeql/ruby/security/ZipSlipCustomizations.qll
Normal file
115
ruby/ql/lib/codeql/ruby/security/ZipSlipCustomizations.qll
Normal file
@@ -0,0 +1,115 @@
|
||||
/**
|
||||
* 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
|
||||
|
||||
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.
|
||||
*/
|
||||
class GzipReaderOpen extends Source {
|
||||
GzipReaderOpen() {
|
||||
this = API::getTopLevelMember("Zlib").getMember("GzipReader").getReturn("open").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.
|
||||
*/
|
||||
class TarReaderInstance extends Source {
|
||||
TarReaderInstance() {
|
||||
this =
|
||||
API::getTopLevelMember("Gem")
|
||||
.getMember("Package")
|
||||
.getMember("TarReader")
|
||||
.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 `Zip::File.open(path)`, considered a flow source.
|
||||
*/
|
||||
class ZipFileOpen extends Source {
|
||||
ZipFileOpen() {
|
||||
this = API::getTopLevelMember("Zip").getMember("File").getReturn("open").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 comparison with a constant string, considered as a sanitizer-guard.
|
||||
*/
|
||||
class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
|
||||
|
||||
/**
|
||||
* An inclusion check against an array of constant strings, considered as a
|
||||
* sanitizer-guard.
|
||||
*/
|
||||
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 modelling of `start_with?` or other comparisons to avoid false-negatives.
|
||||
*/
|
||||
class ExpandedPathStartsWithAsSanitizer extends Sanitizer {
|
||||
ExpandedPathStartsWithAsSanitizer() {
|
||||
exists(DataFlow::CallNode cn |
|
||||
cn.getMethodName() = "expand_path" and
|
||||
this = cn.getArgument(0)
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
40
ruby/ql/lib/codeql/ruby/security/ZipSlipQuery.qll
Normal file
40
ruby/ql/lib/codeql/ruby/security/ZipSlipQuery.qll
Normal file
@@ -0,0 +1,40 @@
|
||||
/**
|
||||
* 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 or node instanceof Path::PathSanitization
|
||||
}
|
||||
}
|
||||
75
ruby/ql/src/queries/security/cwe-022/ZipSlip.qhelp
Normal file
75
ruby/ql/src/queries/security/cwe-022/ZipSlip.qhelp
Normal file
@@ -0,0 +1,75 @@
|
||||
<!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.py" />
|
||||
|
||||
<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.py" />
|
||||
|
||||
</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>
|
||||
Python Library Reference:
|
||||
<a href="https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extract">TarFile.extract</a>.
|
||||
</li>
|
||||
<li>
|
||||
Python Library Reference:
|
||||
<a href="https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extractall">TarFile.extractall</a>.
|
||||
</li>
|
||||
|
||||
</references>
|
||||
</qhelp>
|
||||
22
ruby/ql/src/queries/security/cwe-022/ZipSlip.ql
Normal file
22
ruby/ql/src/queries/security/cwe-022/ZipSlip.ql
Normal 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.security.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"
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -0,0 +1,30 @@
|
||||
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:33:5:33:24 | call to open : | zip_slip.rb:33:35:33:39 | entry : |
|
||||
| zip_slip.rb:33:35:33:39 | entry : | zip_slip.rb:34:17:34:26 | call to name |
|
||||
| zip_slip.rb:53:12:53:54 | call to open : | zip_slip.rb:54:11:54:14 | gzip : |
|
||||
| zip_slip.rb:54:11:54:14 | gzip : | zip_slip.rb:60:42:60:56 | compressed_file : |
|
||||
| zip_slip.rb:60:42:60:56 | compressed_file : | zip_slip.rb:61:7:61:21 | compressed_file : |
|
||||
| zip_slip.rb:61:7:61:21 | compressed_file : | zip_slip.rb:61:32:61:36 | entry : |
|
||||
| zip_slip.rb:61:32:61:36 | entry : | zip_slip.rb:63:21:63: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:33:5:33:24 | call to open : | semmle.label | call to open : |
|
||||
| zip_slip.rb:33:35:33:39 | entry : | semmle.label | entry : |
|
||||
| zip_slip.rb:34:17:34:26 | call to name | semmle.label | call to name |
|
||||
| zip_slip.rb:53:12:53:54 | call to open : | semmle.label | call to open : |
|
||||
| zip_slip.rb:54:11:54:14 | gzip : | semmle.label | gzip : |
|
||||
| zip_slip.rb:60:42:60:56 | compressed_file : | semmle.label | compressed_file : |
|
||||
| zip_slip.rb:61:7:61:21 | compressed_file : | semmle.label | compressed_file : |
|
||||
| zip_slip.rb:61:32:61:36 | entry : | semmle.label | entry : |
|
||||
| zip_slip.rb:63:21:63: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: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 |
|
||||
| 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 |
|
||||
@@ -0,0 +1 @@
|
||||
queries/security/cwe-022/ZipSlip.ql
|
||||
@@ -0,0 +1,99 @@
|
||||
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
|
||||
|
||||
# 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
|
||||
|
||||
# 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)
|
||||
uncompressed_data = gz.read
|
||||
puts uncompressed_data
|
||||
gz.close
|
||||
end
|
||||
zlib.each do |entry|
|
||||
entry_path = entry.full_name
|
||||
::File.open(entry_path, 'wb') do |os|
|
||||
entry.read
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
end
|
||||
Reference in New Issue
Block a user