From 17d9e02da3882e44c1876e2805cf9708481715ee Mon Sep 17 00:00:00 2001
From: Scott McClellan <scott.mcclellan@puppetlabs.com>
Date: Fri, 20 Oct 2017 19:23:10 -0500
Subject: [PATCH] (PUP-7866) Reset permissions when unpacking tar in PMT

When using minitar, files are unpacked with whatever permissions
are in the tarball. This is potentially unsafe, as tarballs can be
easily created with weird permissions.

This change sets the permissions upon unpacking the tar.
---
 lib/puppet/module_tool/tar/mini.rb     | 61 +++++++++++++++++++++++++++++++---
 spec/unit/module_tool/tar/mini_spec.rb | 39 +++++++++++++++++++---
 2 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/lib/puppet/module_tool/tar/mini.rb b/lib/puppet/module_tool/tar/mini.rb
index b3bb36bbcc9..00ad3c2b26c 100644
--- a/lib/puppet/module_tool/tar/mini.rb
+++ b/lib/puppet/module_tool/tar/mini.rb
@@ -3,24 +3,77 @@ def unpack(sourcefile, destdir, _)
     Zlib::GzipReader.open(sourcefile) do |reader|
       Archive::Tar::Minitar.unpack(reader, destdir, find_valid_files(reader)) do |action, name, stats|
         case action
-        when :file_done
-          File.chmod(0644, "#{destdir}/#{name}")
-        when :dir, :file_start
+        when :dir
           validate_entry(destdir, name)
+          set_dir_mode!(stats)
+          Puppet.debug("Extracting: #{destdir}/#{name}")
+        when :file_start
+          # Octal string of the old file mode.
+          validate_entry(destdir, name)
+          set_file_mode!(stats)
           Puppet.debug("Extracting: #{destdir}/#{name}")
         end
+        set_default_user_and_group!(stats)
+        stats
       end
     end
   end
 
   def pack(sourcedir, destfile)
     Zlib::GzipWriter.open(destfile) do |writer|
-      Archive::Tar::Minitar.pack(sourcedir, writer)
+      Archive::Tar::Minitar.pack(sourcedir, writer) do |step, name, stats|
+        # TODO smcclellan 2017-10-31 Set permissions here when this yield block
+        # executes before the header is written. As it stands, the `stats`
+        # argument isn't mutable in a way that will effect the desired mode for
+        # the file.
+      end
     end
   end
 
   private
 
+  EXECUTABLE = 0755
+  NOT_EXECUTABLE = 0644
+  USER_EXECUTE = 0100
+
+  def set_dir_mode!(stats)
+    if stats.key?(:mode)
+      # This is only the case for `pack`, so this code will not run.
+      stats[:mode] = EXECUTABLE
+    elsif stats.key?(:entry)
+      old_mode = stats[:entry].instance_variable_get(:@mode)
+      if old_mode.is_a?(Fixnum)
+        stats[:entry].instance_variable_set(:@mode, EXECUTABLE)
+      end
+    end
+  end
+
+  # Sets a file mode to 0755 if the file is executable by the user.
+  # Sets a file mode to 0644 if the file mode is set (non-Windows).
+  def sanitized_mode(old_mode)
+    old_mode & USER_EXECUTE != 0 ? EXECUTABLE : NOT_EXECUTABLE
+  end
+
+  def set_file_mode!(stats)
+    if stats.key?(:mode)
+      # This is only the case for `pack`, so this code will not run.
+      stats[:mode] = sanitized_mode(stats[:mode])
+    elsif stats.key?(:entry)
+      old_mode = stats[:entry].instance_variable_get(:@mode)
+      # If the user can execute the file, set 0755, otherwise 0644.
+      if old_mode.is_a?(Fixnum)
+        new_mode = sanitized_mode(old_mode)
+        stats[:entry].instance_variable_set(:@mode, new_mode)
+      end
+    end
+  end
+
+  # Sets UID and GID to 0 for standardization.
+  def set_default_user_and_group!(stats)
+    stats[:uid] = 0
+    stats[:gid] = 0
+  end
+
   # Find all the valid files in tarfile.
   #
   # This check was mainly added to ignore 'x' and 'g' flags from the PAX
diff --git a/spec/unit/module_tool/tar/mini_spec.rb b/spec/unit/module_tool/tar/mini_spec.rb
index 0237737faf6..cb9d620f117 100644
--- a/spec/unit/module_tool/tar/mini_spec.rb
+++ b/spec/unit/module_tool/tar/mini_spec.rb
@@ -8,10 +8,17 @@ describe Puppet::ModuleTool::Tar::Mini,
   let(:destfile)   { '/the/dest/file.tar.gz' }
   let(:minitar)    { described_class.new }
 
-  it "unpacks a tar file" do
-    unpacks_the_entry(:file_start, 'thefile')
+  class MockFileStatEntry
+    def initialize(mode = 0100)
+      @mode = mode
+    end
+  end
+
+  it "unpacks a tar file with correct permissions" do
+    entry = unpacks_the_entry(:file_start, 'thefile')
 
     minitar.unpack(sourcefile, destdir, 'uid')
+    expect(entry.instance_variable_get(:@mode)).to eq(0755)
   end
 
   it "does not allow an absolute path" do
@@ -41,20 +48,42 @@ describe Puppet::ModuleTool::Tar::Mini,
                      "Attempt to install file into \"#{File.expand_path('/the/thedir')}\" under \"#{destdir}\"")
   end
 
+  it "unpacks on Windows" do
+    unpacks_the_entry(:file_start, 'thefile', nil)
+
+    entry = minitar.unpack(sourcefile, destdir, 'uid')
+    # Windows does not use these permissions.
+    expect(entry.instance_variable_get(:@mode)).to eq(nil)
+  end
+
   it "packs a tar file" do
     writer = stub('GzipWriter')
 
     Zlib::GzipWriter.expects(:open).with(destfile).yields(writer)
-    Archive::Tar::Minitar.expects(:pack).with(sourcedir, writer)
+    stats = {:mode => 0222}
+    Archive::Tar::Minitar.expects(:pack).with(sourcedir, writer).yields(:file_start, 'abc', stats)
+
+    minitar.pack(sourcedir, destfile)
+  end
+
+  it "packs a tar file on Windows" do
+    writer = stub('GzipWriter')
+
+    Zlib::GzipWriter.expects(:open).with(destfile).yields(writer)
+    Archive::Tar::Minitar.expects(:pack).with(sourcedir, writer).
+        yields(:file_start, 'abc', {:entry => MockFileStatEntry.new(nil)})
 
     minitar.pack(sourcedir, destfile)
   end
 
-  def unpacks_the_entry(type, name)
+  def unpacks_the_entry(type, name, mode = 0100)
     reader = stub('GzipReader')
 
     Zlib::GzipReader.expects(:open).with(sourcefile).yields(reader)
     minitar.expects(:find_valid_files).with(reader).returns([name])
-    Archive::Tar::Minitar.expects(:unpack).with(reader, destdir, [name]).yields(type, name, nil)
+    entry = MockFileStatEntry.new(mode)
+    Archive::Tar::Minitar.expects(:unpack).with(reader, destdir, [name]).
+        yields(type, name, {:entry => entry})
+    entry
   end
 end