From b99c072e115d7ef84ae496cd8c87aade7bdac93e Mon Sep 17 00:00:00 2001 From: Austin Ziegler Date: Sun, 13 Nov 2016 23:25:21 -0500 Subject: [PATCH] Resolve relative path vulnerability Fixes #16 --- History.md | 36 +++++++++++++++++++++----------- lib/archive/tar/minitar.rb | 11 +++++++--- lib/archive/tar/minitar/input.rb | 34 +++++++++++++++++++++++------- minitar.gemspec | 2 +- test/test_tar_input.rb | 32 ++++++++++++++++++++++++++++ 5 files changed, 92 insertions(+), 23 deletions(-) diff --git a/History.md b/History.md index 6fd68cc..39b55de 100644 --- a/History.md +++ b/History.md @@ -8,6 +8,13 @@ `archive-tar-minitar` will install both `minitar` and `minitar-cli`, at least until version 1.0.) + * Minitar extraction before 0.6 traverses directories if the tarball + includes a relative directory reference, as reported in [#16][] by + @ecneladis. This has been disallowed entirely and will throw a + SecureRelativePathError when found. Additionally, if the final + destination of a file is a symbolic link, the symbolic link will be + removed and the file will be written correctly. + * Enhancements: * Licence change. After speaking with Mauricio Fernández, we have changed @@ -51,18 +58,16 @@ * Bugs: - * Fix [#2](https://github.com/halostatue/minitar/issues/2) to handle IO - streams that are not seekable, such as pipes, STDIN, or STDOUT. - * Fix [#3](https://github.com/halostatue/minitar/issues/3) to make the - test timezone resilient. - * Fix [#4](https://github.com/halostatue/minitar/issues/4) for supporting - the reading of tar files with filenames in the GNU long filename - extension format. Ported from @atoulme’s fork, originally provided by - Curtis Sampson. - * Fix [#6](https://github.com/halostatue/minitar/issues/6) by making it - raise the correct error for a long filename with no path components. - * Fix [#14](https://github.com/halostatue/minitar/pull/6) provided by - @kzys should fix Windows detection issues. + * Fix [#2][] to handle IO streams that are not seekable, such as pipes, + STDIN, or STDOUT. + * Fix [#3][] to make the test timezone resilient. + * Fix [#4][] for supporting the reading of tar files with filenames in + the GNU long filename extension format. Ported from @atoulme’s fork, + originally provided by Curtis Sampson. + * Fix [#6][] by making it raise the correct error for a long filename + with no path components. + * Fix [#14][] provided by @kzys should fix Windows detection issues. + * Fix [#16][] as specified above. * Development: @@ -83,3 +88,10 @@ * Initial release. Does files and directories. Command does create, extract, and list. + +[#2]: https://github.com/halostatue/minitar/issues/2 +[#3]: https://github.com/halostatue/minitar/issues/3 +[#4]: https://github.com/halostatue/minitar/issues/4 +[#6]: https://github.com/halostatue/minitar/issues/6 +[#14]: https://github.com/halostatue/minitar/issues/14 +[#16]: https://github.com/halostatue/minitar/issues/16 diff --git a/lib/archive/tar/minitar.rb b/lib/archive/tar/minitar.rb index b69a3a1..b38bc7a 100644 --- a/lib/archive/tar/minitar.rb +++ b/lib/archive/tar/minitar.rb @@ -73,17 +73,22 @@ def modules module Archive::Tar::Minitar VERSION = '0.6' # :nodoc: + # The base class for any minitar error. + Error = Class.new(StandardError) # Raised when a wrapped data stream class is not seekable. - NonSeekableStream = Class.new(StandardError) + NonSeekableStream = Class.new(Error) # The exception raised when operations are performed on a stream that has # previously been closed. - ClosedStream = Class.new(StandardError) + ClosedStream = Class.new(Error) # The exception raised when a filename exceeds 256 bytes in length, the # maximum supported by the standard Tar format. - FileNameTooLong = Class.new(StandardError) + FileNameTooLong = Class.new(Error) # The exception raised when a data stream ends before the amount of data # expected in the archive's PosixHeader. UnexpectedEOF = Class.new(StandardError) + # The exception raised when a file contains a relative path in secure mode + # (the default for this version). + SecureRelativePathError = Class.new(Error) class << self # Tests if +path+ refers to a directory. Fixes an apparently diff --git a/lib/archive/tar/minitar/input.rb b/lib/archive/tar/minitar/input.rb index 599948e..4ba27fe 100644 --- a/lib/archive/tar/minitar/input.rb +++ b/lib/archive/tar/minitar/input.rb @@ -97,10 +97,25 @@ def extract_entry(destdir, entry) # :yields action, name, stats: :entry => entry } + # extract_entry is not vulnerable to prefix '/' vulnerabilities, but it + # is vulnerable to relative path directories. This code will break this + # vulnerability. For this version, we are breaking relative paths HARD by + # throwing an exception. + # + # Future versions may permit relative paths as long as the file does not + # leave +destdir+. + # + # However, squeeze consecutive '/' characters together. + full_name = entry.full_name.squeeze('/') + + if full_name =~ /\.{2}(?:\/|\z)/ + raise SecureRelativePathError, %q(Path contains '..') + end + if entry.directory? - dest = File.join(destdir, entry.full_name) + dest = File.join(destdir, full_name) - yield :dir, entry.full_name, stats if block_given? + yield :dir, full_name, stats if block_given? if Archive::Tar::Minitar.dir?(dest) begin @@ -109,6 +124,8 @@ def extract_entry(destdir, entry) # :yields action, name, stats: nil end else + File.unlink(dest.chomp('/')) if File.symlink?(dest.chomp('/')) + FileUtils.mkdir_p(dest, :mode => entry.mode) FileUtils.chmod(entry.mode, dest) end @@ -117,13 +134,16 @@ def extract_entry(destdir, entry) # :yields action, name, stats: fsync_dir(File.join(dest, "..")) return else # it's a file - destdir = File.join(destdir, File.dirname(entry.full_name)) + destdir = File.join(destdir, File.dirname(full_name)) FileUtils.mkdir_p(destdir, :mode => 0755) - destfile = File.join(destdir, File.basename(entry.full_name)) + destfile = File.join(destdir, File.basename(full_name)) + + File.unlink(destfile) if File.symlink?(destfile) + FileUtils.chmod(0600, destfile) rescue nil # Errno::ENOENT - yield :file_start, entry.full_name, stats if block_given? + yield :file_start, full_name, stats if block_given? File.open(destfile, "wb", entry.mode) do |os| loop do @@ -133,7 +153,7 @@ def extract_entry(destdir, entry) # :yields action, name, stats: stats[:currinc] = os.write(data) stats[:current] += stats[:currinc] - yield :file_progress, entry.full_name, stats if block_given? + yield :file_progress, full_name, stats if block_given? end os.fsync end @@ -142,7 +162,7 @@ def extract_entry(destdir, entry) # :yields action, name, stats: fsync_dir(File.dirname(destfile)) fsync_dir(File.join(File.dirname(destfile), "..")) - yield :file_done, entry.full_name, stats if block_given? + yield :file_done, full_name, stats if block_given? end end diff --git a/minitar.gemspec b/minitar.gemspec index f0674c5..344606b 100644 --- a/minitar.gemspec +++ b/minitar.gemspec @@ -8,7 +8,7 @@ Gem::Specification.new do |s| s.required_rubygems_version = Gem::Requirement.new(">= 0") if s.respond_to? :required_rubygems_version= s.require_paths = ["lib"] s.authors = ["Austin Ziegler"] - s.date = "2016-11-08" + s.date = "2016-11-14" s.description = "The minitar library is a pure-Ruby library that provides the ability to deal\nwith POSIX tar(1) archive files.\n\nThis is release 0.6, \u{2026}\n\nminitar (previously called Archive::Tar::Minitar) is based heavily on code\noriginally written by Mauricio Julio Fern\u{e1}ndez Pradier for the rpa-base\nproject." s.email = ["halostatue@gmail.com"] s.extra_rdoc_files = ["Code-of-Conduct.md", "Contributing.md", "History.md", "Licence.md", "Manifest.txt", "README.rdoc", "docs/bsdl.txt", "docs/ruby.txt"] diff --git a/test/test_tar_input.rb b/test/test_tar_input.rb index 87dc4da..5e46ffb 100644 --- a/test/test_tar_input.rb +++ b/test/test_tar_input.rb @@ -73,6 +73,7 @@ def test_each_works outer += 1 end + assert_equal(2, outer) end end @@ -131,4 +132,35 @@ def test_extract_entry_works assert_equal(2, outer_count) end end + + def test_extract_entry_breaks_symlinks + return if Minitar.windows? + + IO.write('data__/file4', '') + File.symlink('data__/file4', 'data__/file3') + File.symlink('data__/file4', 'data__/data') + + Minitar.unpack(Zlib::GzipReader.new(StringIO.new(TEST_TGZ)), 'data__') + Minitar.unpack(Zlib::GzipReader.new(File.open('data__/data.tar.gz', 'rb')), + 'data__') + + refute File.symlink?('data__/file3') + refute File.symlink?('data__/data') + end + + RELATIVE_DIRECTORY_TGZ = Base64.decode64 <<-EOS +H4sICIIoKVgCA2JhZC1kaXIudGFyANPT0y8sTy0qqWSgHTAwMDAzMVEA0eZmpmDawAjChwEFQ2MDQyMg +MDUzVDAwNDY0N2VQMGCgAygtLkksAjolEcjIzMOtDqgsLQ2/J0H+gNOjYBSMglEwyAEA2LchrwAGAAA= + EOS + + def test_extract_entry_fails_with_relative_directory + reader = Zlib::GzipReader.new(StringIO.new(RELATIVE_DIRECTORY_TGZ)) + Minitar::Input.open(reader) do |stream| + stream.each do |entry| + assert_raises Archive::Tar::Minitar::SecureRelativePathError do + stream.extract_entry("data__", entry) + end + end + end + end end