diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 50a2192..951af8b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -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 diff --git a/Rakefile b/Rakefile index 33a59df..d356330 100644 --- a/Rakefile +++ b/Rakefile @@ -1,6 +1,5 @@ require 'bundler/gem_tasks' require 'rake/testtask' -require 'rdoc/task' desc 'Default: run unit tests.' task default: :test @@ -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' diff --git a/lib/ro_crate/model/directory.rb b/lib/ro_crate/model/directory.rb index 91018e7..3cd7ef5 100644 --- a/lib/ro_crate/model/directory.rb +++ b/lib/ro_crate/model/directory.rb @@ -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 diff --git a/lib/ro_crate/reader.rb b/lib/ro_crate/reader.rb index 368a011..ab6b43a 100644 --- a/lib/ro_crate/reader.rb +++ b/lib/ro_crate/reader.rb @@ -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. # @@ -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 @@ -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? + FileUtils.mkdir_p(dest.dirname) + LEGACY_EXTRACT ? entry.extract(dest) : entry.extract(entry.name, destination_directory: target_path) end end end @@ -345,5 +348,28 @@ 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. + begin + unsafe = dest.expand_path.relative_path_from(base.expand_path).each_filename.first == '..' + rescue ArgumentError # Handle unjoinable paths, e.g. on different drives. + unsafe = true + end + raise ROCrate::ReadException, "Unsafe path in zip entry: #{path}" if unsafe + + dest + end end end diff --git a/ro_crate.gemspec b/ro_crate.gemspec index ba975ca..f9e98d3 100644 --- a/ro_crate.gemspec +++ b/ro_crate.gemspec @@ -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' diff --git a/test/fixtures/unsafe/absolute1.zip b/test/fixtures/unsafe/absolute1.zip new file mode 100644 index 0000000..27c615d Binary files /dev/null and b/test/fixtures/unsafe/absolute1.zip differ diff --git a/test/fixtures/unsafe/relative0.zip b/test/fixtures/unsafe/relative0.zip new file mode 100644 index 0000000..d27a0d0 Binary files /dev/null and b/test/fixtures/unsafe/relative0.zip differ diff --git a/test/reader_test.rb b/test/reader_test.rb index 268facb..825e181 100644 --- a/test/reader_test.rb +++ b/test/reader_test.rb @@ -386,19 +386,49 @@ class ReaderTest < Test::Unit::TestCase assert_equal 'a_file', data.first.name end - private + 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' - def check_exception(exception_class) - e = nil - assert_raise(exception_class) do + e = check_exception(ROCrate::ReadException) do + ROCrate::Reader.unzip_io_to(fixture_file('unsafe/absolute1.zip'), subdir) + end + assert_include e.message, 'Unsafe path in zip entry: /tmp/moo' + + # Simulate ArgumentError in safe_join begin - yield - rescue exception_class => e - raise e + original_expand_path = Pathname.instance_method(:expand_path) + Pathname.define_method(:expand_path) do |*args| + raise ArgumentError, 'Oh no' + end + 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' + ensure + Pathname.define_method(:expand_path, original_expand_path) end end - - e end test 'reads spec 1.1 RO-Crate and preserves version' do diff --git a/test/test_helper.rb b/test/test_helper.rb index bcade3c..674b482 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -5,10 +5,37 @@ require 'ro_crate' require 'webmock/test_unit' +class Test::Unit::TestCase + def teardown + self._opened_files.each do |f| + f.close unless f.closed? + end + end + + def _opened_files + @opened_files ||= [] + end +end + def fixture_file(name, *args) - ::File.open(::File.join(fixture_dir, name), *args) + f = ::File.open(::File.join(fixture_dir, name), *args) + self._opened_files << f + f end def fixture_dir ::File.join(::File.dirname(__FILE__), 'fixtures') end + +def check_exception(exception_class) + e = nil + assert_raise(exception_class) do + begin + yield + rescue exception_class => e + raise e + end + end + + e +end