Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CVE-2018-1000544 #371

Merged
merged 4 commits into from Aug 31, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
Expand Up @@ -24,6 +24,7 @@ matrix:
- rvm: ruby-head
- rvm: rbx-3
- rvm: jruby-head
- rvm: jruby
before_install:
- gem update --system
- gem install bundler
Expand Down
15 changes: 11 additions & 4 deletions lib/zip/entry.rb
Expand Up @@ -147,14 +147,21 @@ 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)/
def extract(dest_path = nil, &block)
if dest_path.nil? && Pathname.new(@name).absolute?
puts "WARNING: skipped absolute path in #{@name}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is putsing a warning the best thing (most extensible) way to do things? I recognize that's what the previous code did, but it seems off to me. Maybe we should raise, or is there some kind of error reporting mechanism that we can surface to the user that's zipfile-wide?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way feel free to say that this is out of scope :)

return self
elsif @name.squeeze('/') =~ /\.{2}(?:\/|\z)/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not also gain the dest_path.nil? check?

puts "WARNING: skipped \"../\" path component(s) in #{@name}"
return self
elsif symlink? && get_input_stream.read =~ %r{../..}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test that this still works for non-malicious symlinks? It doesn't appear there is one.

The comment on get_input_stream's definition seems to also say that it doesn't work too great for symlinks. We should make sure we're not adding any undefined behaviour: https://github.com/rubyzip/rubyzip/blob/master/lib/zip/entry.rb#L484

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symlink could also links to absolute path
You can find examples of such zip files in https://github.com/jwilk/path-traversal-samples

puts "WARNING: skipped \"#{get_input_stream.read}\" symlink path in #{@name}"
return self
end

dest_path ||= @name
block ||= proc { ::Zip.on_exists_proc }

if directory? || file? || symlink?
__send__("create_#{@ftype}", dest_path, &block)
else
Expand Down
Binary file added test/data/absolutepath.zip
Binary file not shown.
Binary file added test/data/symlink.zip
Binary file not shown.
36 changes: 36 additions & 0 deletions test/entry_test.rb
Expand Up @@ -151,4 +151,40 @@ def test_store_file_without_compression

assert_match(/mimetypeapplication\/epub\+zip/, first_100_bytes)
end

def test_entry_name_with_absolute_path_does_not_extract
path = '/tmp/file.txt'
File.delete(path) if File.exist?(path)

Zip::File.open('test/data/absolutepath.zip') do |zip_file|
zip_file.each do |entry|
entry.extract
end
end

refute File.exist?(path)
end

def test_entry_name_with_absolute_path_extract_when_given_different_path
path = '/tmp/CVE-2018-1000544'
FileUtils.rm_rf(path) if Dir.exist?(path)

Zip::File.open('test/data/absolutepath.zip') do |zip_file|
zip_file.each do |entry|
entry.extract("#{path}/#{entry.name}")
end
end

assert File.exist?("#{path}/tmp/file.txt")
end

def test_entry_name_with_relative_symlink
assert_raises Errno::ENOENT do
Zip::File.open('test/data/symlink.zip') do |zip_file|
zip_file.each do |entry|
entry.extract
end
end
end
end
end