Resolve relative path vulnerability

Fixes #16

Author: halostatue

History.md

@@ -8,6 +8,14 @@
         `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 an entry is an already-existing symbolic link, the
+        existing symbolic link will be removed and the file will be written
+        correctly (on platforms that support symblic links).
+
 *   Enhancements:
 
     *   Licence change. After speaking with Mauricio Fernández, we have changed
@@ -51,18 +59,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 +89,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

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

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
 

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 = ["[email protected]"]
   s.extra_rdoc_files = ["Code-of-Conduct.md", "Contributing.md", "History.md", "Licence.md", "Manifest.txt", "README.rdoc", "docs/bsdl.txt", "docs/ruby.txt"]

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