SECURITY: Prevent arbitrary file write when decompressing files (#18421)

* SECURITY: Prevent arbitrary file write when decompressing files
* FIX: Allow decompressing files into symlinked directories

Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>
Co-authored-by: Gerhard Schlager <gerhard.schlager@discourse.org>

lib/compression/engine.rb

@@ -22,6 +22,6 @@ module Compression
       @strategy = strategy
     end
 
-    delegate :extension, :decompress, :compress, :strip_directory, to: :@strategy
+    delegate :extension, :decompress, :compress, to: :@strategy
   end
 end

lib/compression/gzip.rb

@@ -30,8 +30,14 @@ module Compression
       yield(gzip)
     end
 
-    def build_entry_path(_compressed_file, dest_path, compressed_file_path, entry, _allow_non_root_folder)
+    def build_entry_path(dest_path, _, compressed_file_path)
-      compressed_file_path.gsub(extension, '')
+      basename = File.basename(compressed_file_path)
+      basename.gsub!(/#{Regexp.escape(extension)}$/, '')
+      File.join(dest_path, basename)
+    end
+
+    def decompression_results_path(dest_path, compressed_file_path)
+      build_entry_path(dest_path, nil, compressed_file_path)
     end
 
     def extract_file(entry, entry_path, available_size)

lib/compression/pipeline.rb

@@ -20,11 +20,11 @@ module Compression
       end
     end
 
-    def decompress(dest_path, compressed_file_path, max_size, allow_non_root_folder: false)
+    def decompress(dest_path, compressed_file_path, max_size)
       @strategies.reverse.reduce(compressed_file_path) do |to_decompress, strategy|
-        last_extension = strategy.extension
+        next_compressed_file = strategy.decompress(dest_path, to_decompress, max_size)
-        strategy.decompress(dest_path, to_decompress, max_size, allow_non_root_folder: allow_non_root_folder)
+        FileUtils.rm_rf(to_decompress)
-        to_decompress.gsub(last_extension, '')
+        next_compressed_file
       end
     end
   end

lib/compression/strategy.rb

@@ -9,19 +9,20 @@ module Compression
       file_name.include?(extension)
     end
 
-    def decompress(dest_path, compressed_file_path, max_size, allow_non_root_folder: false)
+    def decompress(dest_path, compressed_file_path, max_size)
       sanitized_compressed_file_path = sanitize_path(compressed_file_path)
+      sanitized_dest_path = sanitize_path(dest_path)
 
       get_compressed_file_stream(sanitized_compressed_file_path) do |compressed_file|
         available_size = calculate_available_size(max_size)
 
         entries_of(compressed_file).each do |entry|
-          entry_path = build_entry_path(
+          entry_path = build_entry_path(sanitized_dest_path, entry, sanitized_compressed_file_path)
-            compressed_file, sanitize_path(dest_path),
+          if !is_safe_path_for_extraction?(entry_path, sanitized_dest_path)
-            sanitized_compressed_file_path, entry,
+            next
-            allow_non_root_folder
+          end
-          )
 
+          FileUtils.mkdir_p(File.dirname(entry_path))
           if is_file?(entry)
             remaining_size = extract_file(entry, entry_path, available_size)
             available_size = remaining_size
@@ -29,18 +30,10 @@ module Compression
             extract_folder(entry, entry_path)
           end
         end
+        decompression_results_path(sanitized_dest_path, sanitized_compressed_file_path)
       end
     end
 
-    def strip_directory(from, to, relative: false)
-      sanitized_from = sanitize_path(from) rescue nil
-      sanitized_to = sanitize_path(to) rescue nil
-      return unless sanitized_from && sanitized_to
-
-      glob_path = relative ? "#{sanitized_from}/*/*" : "#{sanitized_from}/**"
-      FileUtils.mv(Dir.glob(glob_path), sanitized_to) if File.directory?(sanitized_from)
-    end
-
     private
 
     def sanitize_path(filename)
@@ -92,5 +85,9 @@ module Compression
 
       remaining_size
     end
+
+    def is_safe_path_for_extraction?(path, dest_directory)
+      File.expand_path(path).start_with?(dest_directory)
+    end
   end
 end

lib/compression/tar.rb

@@ -26,10 +26,12 @@ module Compression
       yield(tar_extract)
     end
 
-    def build_entry_path(_compressed_file, dest_path, compressed_file_path, entry, _allow_non_root_folder)
+    def build_entry_path(dest_path, entry, _)
-      File.join(dest_path, entry.full_name).tap do |entry_path|
+      File.join(dest_path, entry.full_name)
-        FileUtils.mkdir_p(File.dirname(entry_path))
+    end
-      end
+
+    def decompression_results_path(dest_path, _)
+      dest_path
     end
   end
 end

lib/compression/zip.rb

@@ -35,17 +35,12 @@ module Compression
       yield(zip_file)
     end
 
-    def build_entry_path(compressed_file, dest_path, compressed_file_path, entry, allow_non_root_folder)
+    def build_entry_path(dest_path, entry, _)
-      folder_name = compressed_file_path.split('/').last.gsub('.zip', '')
+      File.join(dest_path, entry.name)
-      root = root_folder_present?(compressed_file, allow_non_root_folder) ? '' : "#{folder_name}/"
-
-      File.join(dest_path, "#{root}#{entry.name}").tap do |entry_path|
-        FileUtils.mkdir_p(File.dirname(entry_path))
-      end
     end
 
-    def root_folder_present?(filenames, allow_non_root_folder)
+    def decompression_results_path(dest_path, _)
-      filenames.map { |p| p.name.split('/').first }.uniq.size == 1 || allow_non_root_folder
+      dest_path
     end
 
     def extract_file(entry, entry_path, available_size)

lib/theme_store/zip_importer.rb

@@ -20,7 +20,7 @@ class ThemeStore::ZipImporter
     available_size = SiteSetting.decompressed_theme_max_file_size_mb
     Compression::Engine.engine_for(@original_filename).tap do |engine|
       engine.decompress(@temp_folder, @filename, available_size)
-      engine.strip_directory(@temp_folder, @temp_folder, relative: true)
+      strip_root_directory
     end
   rescue RuntimeError
     raise RemoteTheme::ImportError, I18n.t("themes.import_error.unpack_failed")
@@ -36,6 +36,13 @@ class ThemeStore::ZipImporter
     ""
   end
 
+  def strip_root_directory
+    root_files = Dir.glob("#{@temp_folder}/*")
+    if root_files.size == 1 && File.directory?(root_files[0])
+      FileUtils.mv(Dir.glob("#{@temp_folder}/*/*"), @temp_folder)
+    end
+  end
+
   def real_path(relative)
     fullpath = "#{@temp_folder}/#{relative}"
     return nil unless File.exist?(fullpath)

spec/lib/compression/engine_spec.rb

@@ -2,20 +2,22 @@
 
 RSpec.describe Compression::Engine do
   let(:available_size) { SiteSetting.decompressed_theme_max_file_size_mb }
+  let(:folder_name) { 'test' }
+  let(:temp_folder) do
+    path = "#{Pathname.new(Dir.tmpdir).realpath}/#{SecureRandom.hex}"
+    FileUtils.mkdir(path)
+    path
+  end
 
   before do
-    @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/#{SecureRandom.hex}"
+    Dir.chdir(temp_folder) do
-    @folder_name = 'test'
+      FileUtils.mkdir_p("#{folder_name}/a")
-
+      File.write("#{folder_name}/hello.txt", 'hello world')
-    FileUtils.mkdir(@temp_folder)
+      File.write("#{folder_name}/a/inner", 'hello world inner')
-    Dir.chdir(@temp_folder) do
-      FileUtils.mkdir_p("#{@folder_name}/a")
-      File.write("#{@folder_name}/hello.txt", 'hello world')
-      File.write("#{@folder_name}/a/inner", 'hello world inner')
     end
   end
 
-  after { FileUtils.rm_rf @temp_folder }
+  after { FileUtils.rm_rf(temp_folder) }
 
   it 'raises an exception when the file is not supported' do
     unknown_extension = 'a_file.crazyext'
@@ -24,36 +26,145 @@ RSpec.describe Compression::Engine do
 
   describe 'compressing and decompressing files' do
     before do
-      Dir.chdir(@temp_folder) do
+      Dir.chdir(temp_folder) do
-        @compressed_path = Compression::Engine.engine_for("#{@folder_name}#{extension}").compress(@temp_folder, @folder_name)
+        @compressed_path = Compression::Engine.engine_for("#{folder_name}#{extension}").compress(temp_folder, folder_name)
-        FileUtils.rm_rf("#{@folder_name}/")
+        FileUtils.rm_rf("#{folder_name}/")
       end
     end
 
     context 'when working with zip files' do
       let(:extension) { '.zip' }
 
-      it 'decompress the folder and inspect files correctly' do
+      it 'decompresses the folder and inspects files correctly' do
         engine = described_class.engine_for(@compressed_path)
 
-        engine.decompress(@temp_folder, "#{@temp_folder}/#{@folder_name}.zip", available_size)
+        extract_location = "#{temp_folder}/extract_location"
+        FileUtils.mkdir(extract_location)
+        engine.decompress(extract_location, "#{temp_folder}/#{folder_name}.zip", available_size)
 
-        expect(read_file("test/hello.txt")).to eq("hello world")
+        expect(read_file("extract_location/hello.txt")).to eq("hello world")
-        expect(read_file("test/a/inner")).to eq("hello world inner")
+        expect(read_file("extract_location/a/inner")).to eq("hello world inner")
+      end
+
+      it "doesn't allow files to be extracted outside the target directory" do
+        FileUtils.rm_rf(temp_folder)
+        FileUtils.mkdir(temp_folder)
+
+        zip_file = "#{temp_folder}/theme.zip"
+        Zip::File.open(zip_file, create: true) do |zipfile|
+          zipfile.get_output_stream("child-file") do |f|
+            f.puts("child file")
+          end
+          zipfile.get_output_stream("../escape-decompression-folder.txt") do |f|
+            f.puts("file that attempts to escape the decompression destination directory")
+          end
+          zipfile.mkdir("child-dir")
+          zipfile.get_output_stream("child-dir/grandchild-file") do |f|
+            f.puts("grandchild file")
+          end
+        end
+
+        extract_location = "#{temp_folder}/extract_location"
+        FileUtils.mkdir(extract_location)
+        engine = described_class.engine_for(zip_file)
+        engine.decompress(extract_location, zip_file, available_size)
+        Dir.chdir(temp_folder) do
+          expect(Dir.glob("**/*")).to contain_exactly(
+            "extract_location",
+            "extract_location/child-file",
+            "extract_location/child-dir",
+            "extract_location/child-dir/grandchild-file",
+            "theme.zip"
+          )
+        end
+      end
+
+      it "decompresses into symlinked directory" do
+        real_location = "#{temp_folder}/extract_location"
+        extract_location = "#{temp_folder}/is/symlinked"
+
+        FileUtils.mkdir(real_location)
+        FileUtils.mkdir_p(extract_location)
+        extract_location = "#{extract_location}/extract_location"
+        FileUtils.symlink(real_location, extract_location)
+
+        engine = described_class.engine_for(@compressed_path)
+        engine.decompress(extract_location, "#{temp_folder}/#{folder_name}.zip", available_size)
+
+        expect(File.realpath(extract_location)).to eq(real_location)
+        expect(read_file("is/symlinked/extract_location/hello.txt")).to eq("hello world")
+        expect(read_file("is/symlinked/extract_location/a/inner")).to eq("hello world inner")
       end
     end
 
     context 'when working with .tar.gz files' do
       let(:extension) { '.tar.gz' }
 
-      it 'decompress the folder and inspect files correctly' do
+      it 'decompresses the folder and inspects files correctly' do
         engine = described_class.engine_for(@compressed_path)
 
-        engine.decompress(@temp_folder, "#{@temp_folder}/#{@folder_name}.tar.gz", available_size)
+        engine.decompress(temp_folder, "#{temp_folder}/#{folder_name}.tar.gz", available_size)
 
         expect(read_file("test/hello.txt")).to eq("hello world")
         expect(read_file("test/a/inner")).to eq("hello world inner")
       end
+
+      it "doesn't allow files to be extracted outside the target directory" do
+        FileUtils.rm_rf(temp_folder)
+        FileUtils.mkdir(temp_folder)
+
+        tar_file = "#{temp_folder}/theme.tar"
+        File.open(tar_file, "wb") do |file|
+          Gem::Package::TarWriter.new(file) do |tar|
+            tar.add_file("child-file", 644) do |tf|
+              tf.write("child file")
+            end
+            tar.add_file("../escape-extraction-folder", 644) do |tf|
+              tf.write("file that attempts to escape the decompression destination directory")
+            end
+            tar.mkdir("child-dir", 755)
+            tar.add_file("child-dir/grandchild-file", 644) do |tf|
+              tf.write("grandchild file")
+            end
+          end
+        end
+        tar_gz_file = "#{temp_folder}/theme.tar.gz"
+        Zlib::GzipWriter.open(tar_gz_file) do |gz|
+          gz.orig_name = tar_file
+          gz.write(File.binread(tar_file))
+        end
+        FileUtils.rm(tar_file)
+
+        extract_location = "#{temp_folder}/extract_location"
+        FileUtils.mkdir(extract_location)
+        engine = described_class.engine_for(tar_gz_file)
+        engine.decompress(extract_location, tar_gz_file, available_size)
+        Dir.chdir(temp_folder) do
+          expect(Dir.glob("**/*")).to contain_exactly(
+            "extract_location",
+            "extract_location/child-file",
+            "extract_location/child-dir",
+            "extract_location/child-dir/grandchild-file",
+          )
+        end
+      end
+
+      it "decompresses into symlinked directory" do
+        real_location = "#{temp_folder}/extract_location"
+        extract_location = "#{temp_folder}/is/symlinked"
+
+        FileUtils.mkdir(real_location)
+        FileUtils.mkdir_p(extract_location)
+        extract_location = "#{extract_location}/extract_location"
+        FileUtils.symlink(real_location, extract_location)
+
+        engine = described_class.engine_for(@compressed_path)
+        engine.decompress(extract_location, "#{temp_folder}/#{folder_name}.tar.gz", available_size)
+
+        expect(File.realpath(extract_location)).to eq(real_location)
+        expect(read_file("is/symlinked/extract_location/test/hello.txt")).to eq("hello world")
+        expect(read_file("is/symlinked/extract_location/test/a/inner")).to eq("hello world inner")
+      end
     end
 
     context 'when working with .tar files' do
@@ -62,7 +173,7 @@ RSpec.describe Compression::Engine do
       it 'decompress the folder and inspect files correctly' do
         engine = described_class.engine_for(@compressed_path)
 
-        engine.decompress(@temp_folder, "#{@temp_folder}/#{@folder_name}.tar", available_size)
+        engine.decompress(temp_folder, "#{temp_folder}/#{folder_name}.tar", available_size)
 
         expect(read_file("test/hello.txt")).to eq("hello world")
         expect(read_file("test/a/inner")).to eq("hello world inner")
@@ -71,6 +182,6 @@ RSpec.describe Compression::Engine do
   end
 
   def read_file(relative_path)
-    File.read("#{@temp_folder}/#{relative_path}")
+    File.read("#{temp_folder}/#{relative_path}")
   end
 end

spec/lib/theme_store/zip_exporter_spec.rb

@@ -65,7 +65,7 @@ RSpec.describe ThemeStore::ZipExporter do
     file = 'discourse-header-icons.zip'
     Dir.chdir(dir) do
       available_size = SiteSetting.decompressed_theme_max_file_size_mb
-      Compression::Zip.new.decompress(dir, file, available_size, allow_non_root_folder: true)
+      Compression::Zip.new.decompress(dir, file, available_size)
       `rm #{file}`
 
       folders = Dir.glob("**/*").reject { |f| File.file?(f) }