Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
ruby: ['2.7', '3.0', '3.1', '3.2', '3.3', '3.4']
ruby: ['2.7', '3.0', '3.1', '3.2', '3.3', '3.4', '4.0']
fail-fast: false
steps:
- name: Checkout
Expand Down
9 changes: 0 additions & 9 deletions Rakefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
require 'bundler/gem_tasks'
require 'rake/testtask'
require 'rdoc/task'

desc 'Default: run unit tests.'
task default: :test
Expand All @@ -13,14 +12,6 @@ Rake::TestTask.new(:test) do |t|
t.warning = false
end

Rake::RDocTask.new(:rdoc) do |rdoc|
rdoc.rdoc_dir = 'rdoc'
rdoc.title = 'Devise'
rdoc.options << '--line-numbers' << '--inline-source'
rdoc.rdoc_files.include('README.md')
rdoc.rdoc_files.include('lib/**/*.rb')
end

task :console do
require 'irb'
require 'irb/completion'
Expand Down
5 changes: 2 additions & 3 deletions lib/ro_crate/model/directory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,8 @@ def full_entry_path(relative_path)
end

def list_all_files(source_directory, include_hidden: false)
args = ['**/*']
args << ::File::FNM_DOTMATCH if include_hidden
Dir.chdir(source_directory) { Dir.glob(*args) }.reject do |path|
flags = include_hidden ? ::File::FNM_DOTMATCH : 0
Dir.glob('**/*', flags, base: source_directory).reject do |path|
path == '.' || path == '..' || path.end_with?('/.')
end
end
Expand Down
56 changes: 38 additions & 18 deletions lib/ro_crate/reader.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
require 'zip/version'

module ROCrate
##
# A class to handle reading of RO-Crates from Zip files or directories.
class Reader
LEGACY_EXTRACT = Zip::VERSION.start_with?('2.').freeze

##
# Reads an RO-Crate from a directory or zip file.
#
Expand Down Expand Up @@ -42,15 +46,15 @@ def self.unzip_to(source, target)
#
# @param source [#read] An IO-like object containing a Zip file.
# @param target [String, ::File, Pathname] The target directory where the file should be unzipped.
def self.unzip_io_to(io, target)
Dir.chdir(target) do
Zip::InputStream.open(io) do |input|
while (entry = input.get_next_entry)
unless ::File.exist?(entry.name) || entry.name_is_directory?
FileUtils::mkdir_p(::File.dirname(entry.name))
::File.binwrite(entry.name, input.read)
end
end
def self.unzip_io_to(source, target)
target_path = Pathname(target)
Zip::InputStream.open(source) do |input|
while (entry = input.get_next_entry)
next if entry.name_is_directory?
dest = safe_join(target_path, entry.name)
next if dest.exist?
FileUtils.mkdir_p(dest.dirname)
::File.binwrite(dest, input.read)
end
end
end
Expand All @@ -60,15 +64,14 @@ def self.unzip_io_to(io, target)
#
# @param source [String, ::File, Pathname] The location of the zip file.
# @param target [String, ::File, Pathname] The target directory where the file should be unzipped.
def self.unzip_file_to(file_or_path, target)
Dir.chdir(target) do
Zip::File.open(file_or_path) do |zipfile|
zipfile.each do |entry|
unless ::File.exist?(entry.name)
FileUtils::mkdir_p(::File.dirname(entry.name))
zipfile.extract(entry, entry.name)
end
end
def self.unzip_file_to(source, target)
target_path = Pathname(target)
Zip::File.open(source) do |zipfile|
zipfile.each do |entry|
dest = safe_join(target_path, entry.name)
next if dest.exist?
Comment thread
fbacall marked this conversation as resolved.
FileUtils.mkdir_p(dest.dirname)
LEGACY_EXTRACT ? entry.extract(dest) : entry.extract(entry.name, destination_directory: target_path)
end
end
end
Expand Down Expand Up @@ -345,5 +348,22 @@ def self.detect_root_directory(source)

nil
end

##
# Safely joins a desired file path onto a base directory, raising an exception if the path attempts to traverse
# outside it.
#
# @param base [Pathname] The base directory where the file will go.
# @param path [String] The desired file path.
#
# @raise [ROCrate::ReadException] Raised if an unsafe path is given.
#
# @return [Pathname] The safely joined base + path.
def self.safe_join(base, path)
dest = base.join(path)
# Guard against zip-slip attacks.
raise ROCrate::ReadException, "Unsafe path in zip entry: #{path}" if dest.relative_path_from(base).each_filename.first == '..'
dest
Comment thread
fbacall marked this conversation as resolved.
end
end
end
2 changes: 1 addition & 1 deletion ro_crate.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Gem::Specification.new do |s|
s.licenses = ['MIT']
s.add_runtime_dependency 'addressable', '>= 2.7', '< 3'
s.add_runtime_dependency 'rubyzip', '>= 2.3', '< 4'
s.add_development_dependency 'rake', '~> 13.0.0'
s.add_development_dependency 'rake', '~> 13.4.2'
s.add_development_dependency 'test-unit', '~> 3.5.3'
s.add_development_dependency 'simplecov', '~> 0.21.2'
s.add_development_dependency 'yard', '~> 0.9.25'
Expand Down
Binary file added test/fixtures/unsafe/absolute1.zip
Binary file not shown.
Binary file added test/fixtures/unsafe/relative0.zip
Binary file not shown.
33 changes: 33 additions & 0 deletions test/reader_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,39 @@ class ReaderTest < Test::Unit::TestCase
assert_equal 'a_file', data.first.name
end

test 'protect against zip-slip' do
Dir.mktmpdir do |dir|
subdir = ::File.join(dir, 'subdir')
::Dir.mkdir(subdir)

# Relative
e = check_exception(ROCrate::ReadException) do
ROCrate::Reader.unzip_file_to(fixture_file('unsafe/relative0.zip').path, subdir)
end
assert_include e.message, 'Unsafe path in zip entry: ../moo'
refute ::File.exist?(::File.join(dir, 'moo'))

e = check_exception(ROCrate::ReadException) do
ROCrate::Reader.unzip_io_to(fixture_file('unsafe/relative0.zip'), subdir)
end
assert_include e.message, 'Unsafe path in zip entry: ../moo'
refute ::File.exist?(::File.join(dir, 'moo'))

# Absolute
e = check_exception(ROCrate::ReadException) do
ROCrate::Reader.unzip_file_to(fixture_file('unsafe/absolute1.zip').path, subdir)
end
assert_include e.message, 'Unsafe path in zip entry: /tmp/moo'
refute ::File.exist?(::File.join('/tmp', 'moo'))

e = check_exception(ROCrate::ReadException) do
ROCrate::Reader.unzip_io_to(fixture_file('unsafe/absolute1.zip'), subdir)
end
Comment thread
fbacall marked this conversation as resolved.
assert_include e.message, 'Unsafe path in zip entry: /tmp/moo'
refute ::File.exist?(::File.join('/tmp', 'moo'))
Comment thread
fbacall marked this conversation as resolved.
Outdated
end
end

private

def check_exception(exception_class)
Expand Down