https://git.ruby-lang.org/ruby.git/commit/?id=bbcf8f87ac

From bbcf8f87ac50be423991ccbb2d83ac09ebecf46a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <deivid.rodriguez@r...>
Date: Wed, 6 Oct 2021 18:17:37 +0200
Subject: [ruby/rubygems] Check safety of packaged symlinks

If we explicitly disallow the creation of symlinks that point to files
outside of the destination directory, we can avoid any other safety
checks while creating directories, because we can be sure they will
always fall under the destination directory as well.

https://github.com/rubygems/rubygems/commit/555692b8de
---
 lib/rubygems/package.rb           | 33 ++++++++++++++++-----------------
 test/rubygems/test_gem_package.rb | 11 +++++++----
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/lib/rubygems/package.rb b/lib/rubygems/package.rb
index 7f843f4e0a..0ca272e099 100644
--- a/lib/rubygems/package.rb
+++ b/lib/rubygems/package.rb
@@ -71,6 +71,13 @@ class Gem::Package https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package.rb#L71
     end
   end
 
+  class SymlinkError < Error
+    def initialize(name, destination, destination_dir)
+      super "installing symlink '%s' pointing to parent path %s of %s is not allowed" %
+              [name, destination, destination_dir]
+    end
+  end
+
   class NonSeekableIO < Error; end
 
   class TooLongFileName < Error; end
@@ -407,6 +414,14 @@ EOM https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package.rb#L414
 
         destination = install_location entry.full_name, destination_dir
 
+        if entry.symlink?
+          link_target = entry.header.linkname
+          real_destination = link_target.start_with?("/") ? link_target : File.expand_path(link_target, File.dirname(destination))
+
+          raise Gem::Package::SymlinkError.new(entry.full_name, real_destination, destination_dir) unless
+            normalize_path(real_destination).start_with? normalize_path(destination_dir + '/')
+        end
+
         FileUtils.rm_rf destination
 
         mkdir_options = {}
@@ -419,7 +434,7 @@ EOM https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package.rb#L434
           end
 
         unless directories.include?(mkdir)
-          mkdir_p_safe mkdir, mkdir_options, destination_dir, entry.full_name
+          FileUtils.mkdir_p mkdir, **mkdir_options
           directories << mkdir
         end
 
@@ -495,22 +510,6 @@ EOM https://github.com/ruby/ruby/blob/trunk/lib/rubygems/package.rb#L510
     end
   end
 
-  def mkdir_p_safe(mkdir, mkdir_options, destination_dir, file_name)
-    destination_dir = File.realpath(File.expand_path(destination_dir))
-    parts = mkdir.split(File::SEPARATOR)
-    parts.reduce do |path, basename|
-      path = File.realpath(path) unless path == ""
-      path = File.expand_path(path + File::SEPARATOR + basename)
-      lstat = File.lstat path rescue nil
-      if !lstat || !lstat.directory?
-        unless normalize_path(path).start_with? normalize_path(destination_dir) and (FileUtils.mkdir path, **mkdir_options rescue false)
-          raise Gem::Package::PathError.new(file_name, destination_dir)
-        end
-      end
-      path
-    end
-  end
-
   ##
   # Loads a Gem::Specification from the TarEntry +entry+
 
diff --git a/test/rubygems/test_gem_package.rb b/test/rubygems/test_gem_package.rb
index 5dabcfd47c..27afca1ccb 100644
--- a/test/rubygems/test_gem_package.rb
+++ b/test/rubygems/test_gem_package.rb
@@ -574,7 +574,7 @@ class TestGemPackage < Gem::Package::TarTestCase https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_package.rb#L574
     destination_subdir = File.join @destination, 'subdir'
     FileUtils.mkdir_p destination_subdir
 
-    expected_exceptions = win_platform? ? [Gem::Package::PathError, Errno::EACCES] : [Gem::Package::PathError]
+    expected_exceptions = win_platform? ? [Gem::Package::SymlinkError, Errno::EACCES] : [Gem::Package::SymlinkError]
 
     e = assert_raise(*expected_exceptions) do
       package.extract_tar_gz tgz_io, destination_subdir
@@ -582,10 +582,11 @@ class TestGemPackage < Gem::Package::TarTestCase https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_package.rb#L582
 
     pend "symlink - must be admin with no UAC on Windows" if Errno::EACCES === e
 
-    assert_equal("installing into parent path lib/link/outside.txt of " +
+    assert_equal("installing symlink 'lib/link' pointing to parent path #{@destination} of " +
                 "#{destination_subdir} is not allowed", e.message)
 
     assert_path_not_exist File.join(@destination, "outside.txt")
+    assert_path_not_exist File.join(destination_subdir, "lib/link")
   end
 
   def test_extract_symlink_parent_doesnt_delete_user_dir
@@ -608,7 +609,7 @@ class TestGemPackage < Gem::Package::TarTestCase https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_package.rb#L609
       tar.add_symlink 'link/dir', '.', 16877
     end
 
-    expected_exceptions = win_platform? ? [Gem::Package::PathError, Errno::EACCES] : [Gem::Package::PathError]
+    expected_exceptions = win_platform? ? [Gem::Package::SymlinkError, Errno::EACCES] : [Gem::Package::SymlinkError]
 
     e = assert_raise(*expected_exceptions) do
       package.extract_tar_gz tgz_io, destination_subdir
@@ -616,10 +617,12 @@ class TestGemPackage < Gem::Package::TarTestCase https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_package.rb#L617
 
     pend "symlink - must be admin with no UAC on Windows" if Errno::EACCES === e
 
-    assert_equal("installing into parent path #{destination_user_subdir} of " +
+    assert_equal("installing symlink 'link' pointing to parent path #{destination_user_dir} of " +
                 "#{destination_subdir} is not allowed", e.message)
 
     assert_path_exist destination_user_subdir
+    assert_path_not_exist File.join(destination_subdir, "link/dir")
+    assert_path_not_exist File.join(destination_subdir, "link")
   end
 
   def test_extract_tar_gz_directory
-- 
cgit v1.2.1


--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/