[PATCH 0/1] Help wanted fixing a ruby-minitar bug

[PATCH 0/1] Help wanted fixing a ruby-minitar bug

[Leo Famulari]

I tried using this patch to fix a bug in ruby-minitar, but the build
fails as shown below. Any ideas?

$ ./pre-inst-env guix build ruby-minitar
The following derivations will be built:
   /gnu/store/naprvm320mvzlhv4ciax0436qqm3r8zj-ruby-minitar-0.5.4.drv
   /gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv
@ build-started /gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv - x86_64-linux /var/log/guix/drvs/vz//70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv.bz2
data.tar.gz
metadata.gz
source is under 'data.tar.gz'
Backtrace:
In ice-9/boot-9.scm:
 160: 10 [catch #t #<catch-closure 8cac60> ...]
In unknown file:
   ?: 9 [apply-smob/1 #<catch-closure 8cac60>]
In ice-9/boot-9.scm:
  66: 8 [call-with-prompt prompt0 ...]
In ice-9/eval.scm:
 432: 7 [eval # #]
In ice-9/boot-9.scm:
2404: 6 [save-module-excursion #<procedure 8ea7c0 at ice-9/boot-9.scm:4051:3 ()>]
4056: 5 [#<procedure 8ea7c0 at ice-9/boot-9.scm:4051:3 ()>]
1727: 4 [%start-stack load-stack #<procedure 8fcae0 at ice-9/boot-9.scm:4047:10 ()>]
1732: 3 [#<procedure 8fd6f0 ()>]
In unknown file:
   ?: 2 [primitive-load "/gnu/store/l936dy9y0hz8wxkp4n6y9birnl3fblbb-minitar-0.5.4.tar.xz-builder"]
In ice-9/eval.scm:
 432: 1 [eval # #]
In unknown file:
   ?: 0 [chdir "data.tar.gz"]

ERROR: In procedure chdir:
ERROR: In procedure chdir: Not a directory
builder for `/gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv' failed with exit code 1
@ build-failed /gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv - 1 builder for `/gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv' failed with exit code 1
cannot build derivation `/gnu/store/naprvm320mvzlhv4ciax0436qqm3r8zj-ruby-minitar-0.5.4.drv': 1 dependencies couldn't be built
guix build: error: build failed: build of `/gnu/store/naprvm320mvzlhv4ciax0436qqm3r8zj-ruby-minitar-0.5.4.drv' failed

Leo Famulari (1):
  gnu: ruby-minitar: Fix an arbitrary file overwrite bug.

 gnu/local.mk                                       |   1 +
 .../minitar-fix-arbitrary-file-overwrite.patch     | 253 +++++++++++++++++++++
 gnu/packages/ruby.scm                              |   1 +
 3 files changed, 255 insertions(+)
 create mode 100644 gnu/packages/patches/minitar-fix-arbitrary-file-overwrite.patch

-- 
2.11.0

[PATCH 1/1] gnu: ruby-minitar: Fix an arbitrary file overwrite bug.

[Leo Famulari]

* gnu/packages/patches/minitar-fix-arbitrary-file-overwrite.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/ruby.scm (ruby-minitar)[source]: Use it.
---
 gnu/local.mk                                       |   1 +
 .../minitar-fix-arbitrary-file-overwrite.patch     | 253 +++++++++++++++++++++
 gnu/packages/ruby.scm                              |   1 +
 3 files changed, 255 insertions(+)
 create mode 100644 gnu/packages/patches/minitar-fix-arbitrary-file-overwrite.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 3963b97b7..14aa56a75 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -865,6 +865,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/rpm-CVE-2014-8118.patch			\
   %D%/packages/patches/rsem-makefile.patch			\
   %D%/packages/patches/ruby-concurrent-ignore-broken-test.patch	\
+  %D%/packages/patches/minitar-fix-arbitrary-file-overwrite.patch	\
   %D%/packages/patches/ruby-puma-ignore-broken-test.patch       \
   %D%/packages/patches/ruby-rack-ignore-failing-test.patch      \
   %D%/packages/patches/ruby-tzinfo-data-ignore-broken-test.patch\
diff --git a/gnu/packages/patches/minitar-fix-arbitrary-file-overwrite.patch b/gnu/packages/patches/minitar-fix-arbitrary-file-overwrite.patch
new file mode 100644
index 000000000..5d1836a09
--- /dev/null
+++ b/gnu/packages/patches/minitar-fix-arbitrary-file-overwrite.patch
@@ -0,0 +1,253 @@
+Fix a bug allowing arbitrary file overwrite during archive extraction
+via '..' directory traversal:
+
+http://seclists.org/oss-sec/2017/q1/178
+https://github.com/halostatue/minitar/issues/16
+
+Patch copied from upstream source repository:
+
+https://github.com/halostatue/minitar/commit/e25205ecbb6277ae8a3df1e6a306d7ed4458b6e4
+
+From e25205ecbb6277ae8a3df1e6a306d7ed4458b6e4 Mon Sep 17 00:00:00 2001
+From: Austin Ziegler <austin@zieglers.ca>
+Date: Sun, 13 Nov 2016 23:25:21 -0500
+Subject: [PATCH] Resolve relative path vulnerability
+
+Fixes #16
+---
+ History.md                       | 37 +++++++++++++++++++++++++------------
+ 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, 93 insertions(+), 23 deletions(-)
+
+diff --git a/History.md b/History.md
+index 6fd68cc..adfd992 100644
+--- a/History.md
++++ b/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
+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
diff --git a/gnu/packages/ruby.scm b/gnu/packages/ruby.scm
index 0f1ecd29d..e2a7a88f3 100644
--- a/gnu/packages/ruby.scm
+++ b/gnu/packages/ruby.scm
@@ -1866,6 +1866,7 @@ generation of complex SQL queries and is compatible with various RDBMSes.")
      (origin
        (method url-fetch)
        (uri (rubygems-uri "minitar" version))
+       (patches (search-patches "minitar-fix-arbitrary-file-overwrite.patch"))
        (sha256
         (base32
          "1vpdjfmdq1yc4i620frfp9af02ia435dnpj8ybsd7dc3rypkvbka"))))
-- 
2.11.0

Re: [PATCH 0/1] Help wanted fixing a ruby-minitar bug

[Ludovic Courtès]

Leo Famulari <leo@famulari.name> skribis:

> I tried using this patch to fix a bug in ruby-minitar, but the build
> fails as shown below. Any ideas?
>
> $ ./pre-inst-env guix build ruby-minitar
> The following derivations will be built:
>    /gnu/store/naprvm320mvzlhv4ciax0436qqm3r8zj-ruby-minitar-0.5.4.drv
>    /gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv
> @ build-started /gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv - x86_64-linux /var/log/guix/drvs/vz//70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv.bz2
> data.tar.gz
> metadata.gz
> source is under 'data.tar.gz'

[...]

>    ?: 0 [chdir "data.tar.gz"]

This is apparently a “tarbomb”: it unpacks more than one file in $PWD.
The ‘first-subdirectory’ procedure in gnu-build-system.scm is a bit
stupid and takes the first file in $PWD as the directory to cd to.

To work around that, use ‘url-fetch/tarbomb’.

HTH!

Ludo’.

Re: [PATCH 1/1] gnu: ruby-minitar: Fix an arbitrary file overwrite bug.

[Ludovic Courtès]

Leo Famulari <leo@famulari.name> skribis:

> * gnu/packages/patches/minitar-fix-arbitrary-file-overwrite.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/packages/ruby.scm (ruby-minitar)[source]: Use it.

Maybe exclude the History.md changes from the patch, but otherwise LGTM.

Thank you!

Ludo’.

Re: [PATCH 0/1] Help wanted fixing a ruby-minitar bug

[Ben Woodcroft]

Hi Leo,


On 25/01/17 07:40, Ludovic Courtès wrote:
> Leo Famulari <leo@famulari.name> skribis:
>
>> I tried using this patch to fix a bug in ruby-minitar, but the build
>> fails as shown below. Any ideas?
>>
>> $ ./pre-inst-env guix build ruby-minitar
>> The following derivations will be built:
>>     /gnu/store/naprvm320mvzlhv4ciax0436qqm3r8zj-ruby-minitar-0.5.4.drv
>>     /gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv
>> @ build-started /gnu/store/vz70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv - x86_64-linux /var/log/guix/drvs/vz//70g01wln7aflljz65yf425pppcgpjd-minitar-0.5.4.tar.xz.drv.bz2
>> data.tar.gz
>> metadata.gz
>> source is under 'data.tar.gz'
> [...]
>
>>     ?: 0 [chdir "data.tar.gz"]
> This is apparently a “tarbomb”: it unpacks more than one file in $PWD.
> The ‘first-subdirectory’ procedure in gnu-build-system.scm is a bit
> stupid and takes the first file in $PWD as the directory to cd to.

Well, it is unpacking a .gem file rather than a tar.gz, and patching 
rubygem sources has never worked. It might be worth fixing this in 
general, or perhaps packaging minitar from GitHub rather than rubygems.

As usual, thanks for keeping on top of all this.
ben

Re: [PATCH 0/1] Help wanted fixing a ruby-minitar bug

[Leo Famulari]

On Wed, Jan 25, 2017 at 09:32:22AM +1000, Ben Woodcroft wrote:
> Well, it is unpacking a .gem file rather than a tar.gz, and patching rubygem
> sources has never worked. It might be worth fixing this in general, or
> perhaps packaging minitar from GitHub rather than rubygems.

Would you like to try packaging minitar from the Git repo so we can get
this bug-fix?

Re: [PATCH 0/1] Help wanted fixing a ruby-minitar bug

[Ben Woodcroft]

qOn 26/01/17 03:42, Leo Famulari wrote:
> On Wed, Jan 25, 2017 at 09:32:22AM +1000, Ben Woodcroft wrote:
>> Well, it is unpacking a .gem file rather than a tar.gz, and patching rubygem
>> sources has never worked. It might be worth fixing this in general, or
>> perhaps packaging minitar from GitHub rather than rubygems.
> Would you like to try packaging minitar from the Git repo so we can get
> this bug-fix?
Sure, pushed as bfaf8efd74e88d99f743845357e3f1ff191e23c2.
ben

Re: [PATCH 0/1] Help wanted fixing a ruby-minitar bug

[Leo Famulari]

On Thu, Jan 26, 2017 at 08:30:01AM +1000, Ben Woodcroft wrote:
> qOn 26/01/17 03:42, Leo Famulari wrote:
> > On Wed, Jan 25, 2017 at 09:32:22AM +1000, Ben Woodcroft wrote:
> > > Well, it is unpacking a .gem file rather than a tar.gz, and patching rubygem
> > > sources has never worked. It might be worth fixing this in general, or
> > > perhaps packaging minitar from GitHub rather than rubygems.
> > Would you like to try packaging minitar from the Git repo so we can get
> > this bug-fix?
> Sure, pushed as bfaf8efd74e88d99f743845357e3f1ff191e23c2.

Thank you!