SECURITY: Safely decompress backups when restoring. (#8166)

* SECURITY: Safely decompress backups when restoring.

* Fix tests and update theme_controller_spec to work with zip files instead of .tar.gz

lib/backup_restore/restorer.rb

@@ -57,7 +57,7 @@ module BackupRestore
       ensure_directory_exists(@tmp_directory)
 
       copy_archive_to_tmp_directory
-      unzip_archive
+      decompress_archive
 
       extract_metadata
       validate_metadata
@@ -109,6 +109,70 @@ module BackupRestore
       @success ? log("[SUCCESS]") : log("[FAILED]")
     end
 
+    ### The methods listed below are public just for testing purposes.
+    ### This is not a good practice, but we need to be sure that our new compression API will work.
+
+    attr_reader :tmp_directory
+
+    def ensure_directory_exists(directory)
+      log "Making sure #{directory} exists..."
+      FileUtils.mkdir_p(directory)
+    end
+
+    def copy_archive_to_tmp_directory
+      if @store.remote?
+        log "Downloading archive to tmp directory..."
+        failure_message = "Failed to download archive to tmp directory."
+      else
+        log "Copying archive to tmp directory..."
+        failure_message = "Failed to copy archive to tmp directory."
+      end
+
+      @store.download_file(@filename, @archive_filename, failure_message)
+    end
+
+    def decompress_archive
+      return unless @is_archive
+
+      log "Unzipping archive, this may take a while..."
+
+      pipeline = Compression::Pipeline.new([Compression::Tar.new, Compression::Gzip.new])
+
+      unzipped_path = pipeline.decompress(@tmp_directory, @archive_filename)
+      pipeline.strip_directory(unzipped_path, @tmp_directory)
+    end
+
+    def extract_metadata
+      metadata_path = File.join(@tmp_directory, BackupRestore::METADATA_FILE)
+      @metadata = if File.exists?(metadata_path)
+        data = Oj.load_file(@meta_filename)
+        raise "Failed to load metadata file." if !data
+        data
+      else
+        log "No metadata file to extract."
+        if @filename =~ /-#{BackupRestore::VERSION_PREFIX}(\d{14})/
+          { "version" => Regexp.last_match[1].to_i }
+        else
+          raise "Migration version is missing from the filename."
+        end
+      end
+    end
+
+    def extract_dump
+      @dump_filename =
+        if @is_archive
+          # For backwards compatibility
+          old_dump_path = File.join(@tmp_directory, BackupRestore::OLD_DUMP_FILE)
+          File.exists?(old_dump_path) ? old_dump_path : File.join(@tmp_directory, BackupRestore::DUMP_FILE)
+        else
+          File.join(@tmp_directory, @filename)
+        end
+
+      log "Extracting dump file..."
+
+      Compression::Gzip.new.decompress(@tmp_directory, @dump_filename)
+    end
+
     protected
 
     def ensure_restore_is_enabled
@@ -140,7 +204,6 @@ module BackupRestore
       @tmp_directory = File.join(Rails.root, "tmp", "restores", @current_db, @timestamp)
       @archive_filename = File.join(@tmp_directory, @filename)
       @tar_filename = @archive_filename[0...-3]
-      @meta_filename = File.join(@tmp_directory, BackupRestore::METADATA_FILE)
       @is_archive = !(@filename =~ /.sql.gz$/)
 
       @logs = []
@@ -194,52 +257,6 @@ module BackupRestore
       false
     end
 
-    def copy_archive_to_tmp_directory
-      if @store.remote?
-        log "Downloading archive to tmp directory..."
-        failure_message = "Failed to download archive to tmp directory."
-      else
-        log "Copying archive to tmp directory..."
-        failure_message = "Failed to copy archive to tmp directory."
-      end
-
-      @store.download_file(@filename, @archive_filename, failure_message)
-    end
-
-    def unzip_archive
-      return unless @is_archive
-
-      log "Unzipping archive, this may take a while..."
-
-      FileUtils.cd(@tmp_directory) do
-        Discourse::Utils.execute_command('gzip', '--decompress', @archive_filename, failure_message: "Failed to unzip archive.")
-      end
-    end
-
-    def extract_metadata
-      @metadata =
-        if system('tar', '--list', '--file', @tar_filename, BackupRestore::METADATA_FILE)
-          log "Extracting metadata file..."
-          FileUtils.cd(@tmp_directory) do
-            Discourse::Utils.execute_command(
-              'tar', '--extract', '--file', @tar_filename, BackupRestore::METADATA_FILE,
-              failure_message: "Failed to extract metadata file."
-            )
-          end
-
-          data = Oj.load_file(@meta_filename)
-          raise "Failed to load metadata file." if !data
-          data
-        else
-          log "No metadata file to extract."
-          if @filename =~ /-#{BackupRestore::VERSION_PREFIX}(\d{14})/
-            { "version" => Regexp.last_match[1].to_i }
-          else
-            raise "Migration version is missing from the filename."
-          end
-        end
-    end
-
     def validate_metadata
       log "Validating metadata..."
       log "  Current version: #{@current_version}"
@@ -252,31 +269,6 @@ module BackupRestore
       raise error if @metadata["version"] > @current_version
     end
 
-    def extract_dump
-      @dump_filename =
-        if @is_archive
-          # For backwards compatibility
-          if system('tar', '--list', '--file', @tar_filename, BackupRestore::OLD_DUMP_FILE)
-            File.join(@tmp_directory, BackupRestore::OLD_DUMP_FILE)
-          else
-            File.join(@tmp_directory, BackupRestore::DUMP_FILE)
-          end
-        else
-          File.join(@tmp_directory, @filename)
-        end
-
-      return unless @is_archive
-
-      log "Extracting dump file..."
-
-      FileUtils.cd(@tmp_directory) do
-        Discourse::Utils.execute_command(
-          'tar', '--extract', '--file', @tar_filename, File.basename(@dump_filename),
-          failure_message: "Failed to extract dump file."
-        )
-      end
-    end
-
     def get_dumped_by_version
       output = Discourse::Utils.execute_command(
         File.extname(@dump_filename) == '.gz' ? 'zgrep' : 'grep',
@@ -293,7 +285,7 @@ module BackupRestore
 
     def restore_dump_command
       if File.extname(@dump_filename) == '.gz'
-        "gzip -d < #{@dump_filename} | #{sed_command} | #{psql_command} 2>&1"
+        "#{sed_command} #{@dump_filename.gsub('.gz', '')} | #{psql_command} 2>&1"
       else
         "#{psql_command} 2>&1 < #{@dump_filename}"
       end
@@ -427,40 +419,32 @@ module BackupRestore
     end
 
     def extract_uploads
-      if system('tar', '--exclude=*/*', '--list', '--file', @tar_filename, 'uploads')
+      return unless File.exists?(File.join(@tmp_directory, 'uploads'))
-        log "Extracting uploads..."
+      log "Extracting uploads..."
 
-        FileUtils.cd(@tmp_directory) do
+      public_uploads_path = File.join(Rails.root, "public")
-          Discourse::Utils.execute_command(
+
-            'tar', '--extract', '--keep-newer-files', '--file', @tar_filename, 'uploads/',
+      FileUtils.cd(public_uploads_path) do
-            failure_message: "Failed to extract uploads."
+        FileUtils.mkdir_p("uploads")
-          )
+
+        tmp_uploads_path = Dir.glob(File.join(@tmp_directory, "uploads", "*")).first
+        previous_db_name = BackupMetadata.value_for("db_name") || File.basename(tmp_uploads_path)
+        current_db_name = RailsMultisite::ConnectionManagement.current_db
+        optimized_images_exist = File.exist?(File.join(tmp_uploads_path, 'optimized'))
+
+        Discourse::Utils.execute_command(
+          'rsync', '-avp', '--safe-links', "#{tmp_uploads_path}/", "uploads/#{current_db_name}/",
+          failure_message: "Failed to restore uploads."
+        )
+
+        remap_uploads(previous_db_name, current_db_name)
+
+        if SiteSetting.Upload.enable_s3_uploads
+          migrate_to_s3
+          remove_local_uploads(File.join(public_uploads_path, "uploads/#{current_db_name}"))
         end
 
-        public_uploads_path = File.join(Rails.root, "public")
+        generate_optimized_images unless optimized_images_exist
-
-        FileUtils.cd(public_uploads_path) do
-          FileUtils.mkdir_p("uploads")
-
-          tmp_uploads_path = Dir.glob(File.join(@tmp_directory, "uploads", "*")).first
-          previous_db_name = BackupMetadata.value_for("db_name") || File.basename(tmp_uploads_path)
-          current_db_name = RailsMultisite::ConnectionManagement.current_db
-          optimized_images_exist = File.exist?(File.join(tmp_uploads_path, 'optimized'))
-
-          Discourse::Utils.execute_command(
-            'rsync', '-avp', '--safe-links', "#{tmp_uploads_path}/", "uploads/#{current_db_name}/",
-            failure_message: "Failed to restore uploads."
-          )
-
-          remap_uploads(previous_db_name, current_db_name)
-
-          if SiteSetting.Upload.enable_s3_uploads
-            migrate_to_s3
-            remove_local_uploads(File.join(public_uploads_path, "uploads/#{current_db_name}"))
-          end
-
-          generate_optimized_images unless optimized_images_exist
-        end
       end
     end
 
@@ -661,17 +645,14 @@ module BackupRestore
       log "Something went wrong while marking restore as finished.", ex
     end
 
-    def ensure_directory_exists(directory)
-      log "Making sure #{directory} exists..."
-      FileUtils.mkdir_p(directory)
-    end
-
     def after_restore_hook
       log "Executing the after_restore_hook..."
       DiscourseEvent.trigger(:restore_complete)
     end
 
     def log(message, ex = nil)
+      return if Rails.env.test?
+
       timestamp = Time.now.strftime("%Y-%m-%d %H:%M:%S")
       puts(message)
       publish_log(message, timestamp)

lib/compression/engine.rb

@@ -22,12 +22,6 @@ module Compression
       @strategy = strategy
     end
 
-    def decompress(dest_path, compressed_file_path, allow_non_root_folder: false)
+    delegate :extension, :decompress, :compress, :strip_directory, to: :@strategy
-      @strategy.decompress(dest_path, compressed_file_path, allow_non_root_folder: false)
-    end
-
-    def compress(path, target_name)
-      @strategy.compress(path, target_name)
-    end
   end
 end

lib/compression/pipeline.rb

@@ -21,11 +21,10 @@ module Compression
     end
 
     def decompress(dest_path, compressed_file_path, allow_non_root_folder: false)
-      to_decompress = compressed_file_path
+      @strategies.reverse.reduce(compressed_file_path) do |to_decompress, strategy|
-      @strategies.reverse.each do |strategy|
         last_extension = strategy.extension
         strategy.decompress(dest_path, to_decompress, allow_non_root_folder: allow_non_root_folder)
-        to_decompress = compressed_file_path.gsub(last_extension, '')
+        to_decompress.gsub(last_extension, '')
       end
     end
   end

lib/compression/strategy.rb

@@ -32,6 +32,13 @@ module Compression
       end
     end
 
+    def strip_directory(from, to, relative: false)
+      sanitized_from = sanitize_path(from)
+      sanitized_to = sanitize_path(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)

lib/theme_store/zip_importer.rb

@@ -20,10 +20,8 @@ class ThemeStore::ZipImporter
     Dir.chdir(@temp_folder) do
       Compression::Engine.engine_for(@original_filename).tap do |engine|
         engine.decompress(@temp_folder, @filename)
+        engine.strip_directory(@temp_folder, @temp_folder, relative: true)
       end
-
-      # --strip 1 equivalent
-      FileUtils.mv(Dir.glob("#{@temp_folder}/*/*"), @temp_folder)
     end
   rescue RuntimeError
     raise RemoteTheme::ImportError, I18n.t("themes.import_error.unpack_failed")

spec/lib/backup_restore/restorer_spec.rb

@@ -19,4 +19,85 @@ describe BackupRestore::Restorer do
       expect(described_class.pg_produces_portable_dump?(key)).to eq(value)
     end
   end
+
+  describe 'Decompressing a backup' do
+    fab!(:admin) { Fabricate(:admin) }
+
+    before do
+      SiteSetting.allow_restore = true
+      @restore_path = File.join(Rails.root, "public", "backups", RailsMultisite::ConnectionManagement.current_db)
+    end
+
+    after do
+      FileUtils.rm_rf @restore_path
+      FileUtils.rm_rf @restorer.tmp_directory
+    end
+
+    context 'When there are uploads' do
+      before do
+        @restore_folder = "backup-#{SecureRandom.hex}"
+        @temp_folder = "#{@restore_path}/#{@restore_folder}"
+        FileUtils.mkdir_p("#{@temp_folder}/uploads")
+
+        Dir.chdir(@restore_path) do
+          File.write("#{@restore_folder}/dump.sql", 'This is a dump')
+          Compression::Gzip.new.compress(@restore_folder, 'dump.sql')
+          FileUtils.rm_rf("#{@restore_folder}/dump.sql")
+          File.write("#{@restore_folder}/uploads/upload.txt", 'This is an upload')
+
+          Compression::Tar.new.compress(@restore_path, @restore_folder)
+        end
+
+        Compression::Gzip.new.compress(@restore_path, "#{@restore_folder}.tar")
+        FileUtils.rm_rf @temp_folder
+
+        build_restorer("#{@restore_folder}.tar.gz")
+      end
+
+      it '#decompress_archive works correctly' do
+        @restorer.decompress_archive
+
+        expect(exists?("dump.sql.gz")).to eq(true)
+        expect(exists?("uploads", directory: true)).to eq(true)
+      end
+
+      it '#extract_dump works correctly' do
+        @restorer.decompress_archive
+        @restorer.extract_dump
+
+        expect(exists?('dump.sql')).to eq(true)
+      end
+    end
+
+    context 'When restoring a single file' do
+      before do
+        FileUtils.mkdir_p(@restore_path)
+
+        Dir.chdir(@restore_path) do
+          File.write('dump.sql', 'This is a dump')
+          Compression::Gzip.new.compress(@restore_path, 'dump.sql')
+          FileUtils.rm_rf('dump.sql')
+        end
+
+        build_restorer('dump.sql.gz')
+      end
+
+      it '#extract_dump works correctly with a single file' do
+        @restorer.extract_dump
+
+        expect(exists?("dump.sql")).to eq(true)
+      end
+    end
+
+    def exists?(relative_path, directory: false)
+      full_path = "#{@restorer.tmp_directory}/#{relative_path}"
+      directory ? File.directory?(full_path) : File.exists?(full_path)
+    end
+
+    def build_restorer(filename)
+      @restorer = described_class.new(admin.id, filename: filename)
+      @restorer.ensure_directory_exists(@restorer.tmp_directory)
+      @restorer.copy_archive_to_tmp_directory
+    end
+  end
 end

spec/requests/admin/themes_controller_spec.rb

@@ -74,7 +74,7 @@ describe Admin::ThemesController do
     end
 
     let(:theme_archive) do
-      Rack::Test::UploadedFile.new(file_from_fixtures("discourse-test-theme.tar.gz", "themes"), "application/x-gzip")
+      Rack::Test::UploadedFile.new(file_from_fixtures("discourse-test-theme.zip", "themes"), "application/zip")
     end
 
     let(:image) do