Skip to content

Fix CVE-2018-1000544 #371

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 31, 2018
Merged

Fix CVE-2018-1000544 #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e0d231
Fix CVE-2018-1000544 absolute path traversal
bdewater Jul 1, 2018
8e78311
Fix CVE-2018-1000544 symlink path traversal
bdewater Jul 1, 2018
cf71583
Move jruby to allow failures matrix till crc uint 32 issues are resolved
vipulnsward Aug 26, 2018
0586329
Trigger CI again
bdewater Aug 26, 2018
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
1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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

Copy link

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
View file
Open in desktop
Binary file not shown.
Binary file added test/data/symlink.zip
View file
Open in desktop
Binary file not shown.
36 changes: 36 additions & 0 deletions test/entry_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
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