diff --git a/.travis.yml b/.travis.yml index ad59d61e..b197c86b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,6 +24,7 @@ matrix: - rvm: ruby-head - rvm: rbx-3 - rvm: jruby-head + - rvm: jruby before_install: - gem update --system - gem install bundler diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb index 4d8e1751..fddab51e 100644 --- a/lib/zip/entry.rb +++ b/lib/zip/entry.rb @@ -109,6 +109,17 @@ def name_is_directory? #:nodoc:all @name.end_with?('/') end + # Is the name a relative path, free of `..` patterns that could lead to + # path traversal attacks? This does NOT handle symlinks; if the path + # contains symlinks, this check is NOT enough to guarantee safety. + def name_safe? + cleanpath = Pathname.new(@name).cleanpath + return false unless cleanpath.relative? + root = ::File::SEPARATOR + naive_expanded_path = ::File.join(root, cleanpath.to_s) + cleanpath.expand_path(root).to_s == naive_expanded_path + end + def local_entry_offset #:nodoc:all local_header_offset + @local_header_size end @@ -147,14 +158,17 @@ def next_header_offset #:nodoc:all end # Extracts entry to file dest_path (defaults to @name). - def extract(dest_path = @name, &block) - block ||= proc { ::Zip.on_exists_proc } - - if @name.squeeze('/') =~ /\.{2}(?:\/|\z)/ - puts "WARNING: skipped \"../\" path component(s) in #{@name}" + # NB: The caller is responsible for making sure dest_path is safe, if it + # is passed. + def extract(dest_path = nil, &block) + if dest_path.nil? && !name_safe? + puts "WARNING: skipped #{@name} as unsafe" return self end + dest_path ||= @name + block ||= proc { ::Zip.on_exists_proc } + if directory? || file? || symlink? __send__("create_#{@ftype}", dest_path, &block) else @@ -613,32 +627,9 @@ def create_directory(dest_path) # BUG: create_symlink() does not use &block def create_symlink(dest_path) - stat = nil - begin - stat = ::File.lstat(dest_path) - rescue Errno::ENOENT - end - - io = get_input_stream - linkto = io.read - - if stat - if stat.symlink? - if ::File.readlink(dest_path) == linkto - return - else - raise ::Zip::DestinationFileExistsError, - "Cannot create symlink '#{dest_path}'. " \ - 'A symlink already exists with that name' - end - else - raise ::Zip::DestinationFileExistsError, - "Cannot create symlink '#{dest_path}'. " \ - 'A file already exists with that name' - end - end - - ::File.symlink(linkto, dest_path) + # TODO: Symlinks pose security challenges. Symlink support temporarily + # removed in view of https://github.com/rubyzip/rubyzip/issues/369 . + puts "WARNING: skipped symlink #{dest_path}" end # apply missing data from the zip64 extra information field, if present diff --git a/lib/zip/version.rb b/lib/zip/version.rb index f1dc85f4..14a9f99e 100644 --- a/lib/zip/version.rb +++ b/lib/zip/version.rb @@ -1,3 +1,3 @@ module Zip - VERSION = '1.2.1' + VERSION = '1.2.2' end diff --git a/test/data/path_traversal/Makefile b/test/data/path_traversal/Makefile new file mode 100644 index 00000000..9ff4d816 --- /dev/null +++ b/test/data/path_traversal/Makefile @@ -0,0 +1,10 @@ +# Based on 'relative2' in https://github.com/jwilk/path-traversal-samples, +# but create the local `tmp` folder before adding the symlink. Otherwise +# we may bail out before we get to trying to create the file. +all: relative1.zip +relative1.zip: + rm -f $(@) + mkdir -p -m 755 tmp/tmp + umask 022 && echo moo > moo + cd tmp && zip -X ../$(@) tmp tmp/../../moo + rm -rf tmp moo diff --git a/test/data/path_traversal/jwilk/README.md b/test/data/path_traversal/jwilk/README.md new file mode 100644 index 00000000..2ecceb23 --- /dev/null +++ b/test/data/path_traversal/jwilk/README.md @@ -0,0 +1,5 @@ +# Path Traversal Samples + +Copied from https://github.com/jwilk/path-traversal-samples on 2018-08-26. + +License: MIT diff --git a/test/data/path_traversal/jwilk/absolute1.zip b/test/data/path_traversal/jwilk/absolute1.zip new file mode 100644 index 00000000..27c615d9 Binary files /dev/null and b/test/data/path_traversal/jwilk/absolute1.zip differ diff --git a/test/data/path_traversal/jwilk/absolute2.zip b/test/data/path_traversal/jwilk/absolute2.zip new file mode 100644 index 00000000..c82c14ea Binary files /dev/null and b/test/data/path_traversal/jwilk/absolute2.zip differ diff --git a/test/data/path_traversal/jwilk/dirsymlink.zip b/test/data/path_traversal/jwilk/dirsymlink.zip new file mode 100644 index 00000000..978b5d8a Binary files /dev/null and b/test/data/path_traversal/jwilk/dirsymlink.zip differ diff --git a/test/data/path_traversal/jwilk/dirsymlink2a.zip b/test/data/path_traversal/jwilk/dirsymlink2a.zip new file mode 100644 index 00000000..443deede Binary files /dev/null and b/test/data/path_traversal/jwilk/dirsymlink2a.zip differ diff --git a/test/data/path_traversal/jwilk/dirsymlink2b.zip b/test/data/path_traversal/jwilk/dirsymlink2b.zip new file mode 100644 index 00000000..5a5a12b4 Binary files /dev/null and b/test/data/path_traversal/jwilk/dirsymlink2b.zip differ diff --git a/test/data/path_traversal/jwilk/relative0.zip b/test/data/path_traversal/jwilk/relative0.zip new file mode 100644 index 00000000..d27a0d08 Binary files /dev/null and b/test/data/path_traversal/jwilk/relative0.zip differ diff --git a/test/data/path_traversal/jwilk/relative2.zip b/test/data/path_traversal/jwilk/relative2.zip new file mode 100644 index 00000000..8957028d Binary files /dev/null and b/test/data/path_traversal/jwilk/relative2.zip differ diff --git a/test/data/path_traversal/jwilk/symlink.zip b/test/data/path_traversal/jwilk/symlink.zip new file mode 100644 index 00000000..edaa7526 Binary files /dev/null and b/test/data/path_traversal/jwilk/symlink.zip differ diff --git a/test/data/path_traversal/relative1.zip b/test/data/path_traversal/relative1.zip new file mode 100644 index 00000000..bfcb9def Binary files /dev/null and b/test/data/path_traversal/relative1.zip differ diff --git a/test/data/path_traversal/tuzovakaoff/README.md b/test/data/path_traversal/tuzovakaoff/README.md new file mode 100644 index 00000000..f599810e --- /dev/null +++ b/test/data/path_traversal/tuzovakaoff/README.md @@ -0,0 +1,3 @@ +# Path Traversal Samples + +Copied from https://github.com/tuzovakaoff/zip_path_traversal on 2018-08-25. diff --git a/test/data/path_traversal/tuzovakaoff/absolutepath.zip b/test/data/path_traversal/tuzovakaoff/absolutepath.zip new file mode 100644 index 00000000..59fceed7 Binary files /dev/null and b/test/data/path_traversal/tuzovakaoff/absolutepath.zip differ diff --git a/test/data/path_traversal/tuzovakaoff/symlink.zip b/test/data/path_traversal/tuzovakaoff/symlink.zip new file mode 100644 index 00000000..e74ee19a Binary files /dev/null and b/test/data/path_traversal/tuzovakaoff/symlink.zip differ diff --git a/test/data/rubycode.zip b/test/data/rubycode.zip index 8a68560e..06134bbc 100644 Binary files a/test/data/rubycode.zip and b/test/data/rubycode.zip differ diff --git a/test/path_traversal_test.rb b/test/path_traversal_test.rb new file mode 100644 index 00000000..9a361a59 --- /dev/null +++ b/test/path_traversal_test.rb @@ -0,0 +1,134 @@ +class PathTraversalTest < MiniTest::Test + TEST_FILE_ROOT = File.absolute_path('test/data/path_traversal') + + def setup + # With apologies to anyone using these files... but they are the files in + # the sample zips, so we don't have much choice here. + FileUtils.rm_f '/tmp/moo' + FileUtils.rm_f '/tmp/file.txt' + end + + def extract_path_traversal_zip(name) + Zip::File.open(File.join(TEST_FILE_ROOT, name)) do |zip_file| + zip_file.each do |entry| + entry.extract + end + end + end + + def in_tmpdir + Dir.mktmpdir do |tmp| + test_path = File.join(tmp, 'test') + Dir.mkdir test_path + Dir.chdir test_path do + yield test_path + end + end + end + + def test_leading_slash + in_tmpdir do + extract_path_traversal_zip 'jwilk/absolute1.zip' + refute File.exist?('/tmp/moo') + end + end + + def test_multiple_leading_slashes + in_tmpdir do + extract_path_traversal_zip 'jwilk/absolute2.zip' + refute File.exist?('/tmp/moo') + end + end + + def test_leading_dot_dot + in_tmpdir do + extract_path_traversal_zip 'jwilk/relative0.zip' + refute File.exist?('../moo') + end + end + + def test_non_leading_dot_dot_with_existing_folder + in_tmpdir do + extract_path_traversal_zip 'relative1.zip' + assert Dir.exist?('tmp') + refute File.exist?('../moo') + end + end + + def test_non_leading_dot_dot_without_existing_folder + in_tmpdir do + extract_path_traversal_zip 'jwilk/relative2.zip' + refute File.exist?('../moo') + end + end + + def test_file_symlink + in_tmpdir do + extract_path_traversal_zip 'jwilk/symlink.zip' + assert File.exist?('moo') + refute File.exist?('/tmp/moo') + end + end + + def test_directory_symlink + in_tmpdir do + # Can't create tmp/moo, because the tmp symlink is skipped. + assert_raises Errno::ENOENT do + extract_path_traversal_zip 'jwilk/dirsymlink.zip' + end + refute File.exist?('/tmp/moo') + end + end + + def test_two_directory_symlinks_a + in_tmpdir do + # Can't create par/moo because the symlinks are skipped. + assert_raises Errno::ENOENT do + extract_path_traversal_zip 'jwilk/dirsymlink2a.zip' + end + refute File.exist?('cur') + refute File.exist?('par') + refute File.exist?('par/moo') + end + end + + def test_two_directory_symlinks_b + in_tmpdir do + # Can't create par/moo, because the symlinks are skipped. + assert_raises Errno::ENOENT do + extract_path_traversal_zip 'jwilk/dirsymlink2b.zip' + end + refute File.exist?('cur') + refute File.exist?('../moo') + end + end + + def test_entry_name_with_absolute_path_does_not_extract + in_tmpdir do + extract_path_traversal_zip 'tuzovakaoff/absolutepath.zip' + refute File.exist?('/tmp/file.txt') + end + end + + def test_entry_name_with_absolute_path_extract_when_given_different_path + in_tmpdir do |test_path| + zip_path = File.join(TEST_FILE_ROOT, 'tuzovakaoff/absolutepath.zip') + Zip::File.open(zip_path) do |zip_file| + zip_file.each do |entry| + entry.extract(File.join(test_path, entry.name)) + end + end + refute File.exist?('/tmp/file.txt') + end + end + + def test_entry_name_with_relative_symlink + in_tmpdir do + # Doesn't create the symlink path, so can't create path/file.txt. + assert_raises Errno::ENOENT do + extract_path_traversal_zip 'tuzovakaoff/symlink.zip' + end + refute File.exist?('/tmp/file.txt') + end + end +end