Merge pull request #4443 from tgxworld/security_escape_input

SECUIRTY: Escape input made to system calls.

app/controllers/admin/backups_controller.rb

@@ -119,6 +119,7 @@ class Admin::BackupsController < Admin::AdminController
 
     return render status: 415, text: I18n.t("backup.backup_file_should_be_tar_gz") unless /\.(tar\.gz|t?gz)$/i =~ filename
     return render status: 415, text: I18n.t("backup.not_enough_space_on_disk")     unless has_enough_space_on_disk?(total_size)
+    return render status: 415, text: I18n.t("backup.invalid_filename") unless !!(/^[a-zA-Z0-9\.-_]+$/ =~ filename)
 
     file               = params.fetch(:file)
     identifier         = params.fetch(:resumableIdentifier)

config/locales/server.en.yml

@@ -140,6 +140,7 @@ en:
     operation_already_running: "An operation is currently running. Can't start a new job right now."
     backup_file_should_be_tar_gz: "The backup file should be a .tar.gz archive."
     not_enough_space_on_disk: "There is not enough space on disk to upload this backup."
+    invalid_filename: "The backup filename contains invalid characters. Valid characters are a-z 0-9 . - _."
 
   not_logged_in: "You need to be logged in to do that."
   not_found: "The requested URL or resource could not be found."

lib/backup_restore/backuper.rb

@@ -199,8 +199,8 @@ module BackupRestore
       log "Finalizing database dump file: #{@backup_filename}"
 
       execute_command(
-        "mv #{@dump_filename} #{File.join(@archive_directory, @backup_filename)}",
+        'mv', @dump_filename, File.join(@archive_directory, @backup_filename),
-        "Failed to move database dump file."
+        failure_message: "Failed to move database dump file."
       )
 
       remove_tmp_directory
@@ -212,17 +212,17 @@ module BackupRestore
       tar_filename = "#{@archive_basename}.tar"
 
       log "Making sure archive does not already exist..."
-      execute_command("rm -f #{tar_filename}")
+      execute_command('rm', '-f', tar_filename)
-      execute_command("rm -f #{tar_filename}.gz")
+      execute_command('rm', '-f', "#{tar_filename}.gz")
 
       log "Creating empty archive..."
-      execute_command("tar --create --file #{tar_filename} --files-from /dev/null")
+      execute_command('tar', '--create', '--file', tar_filename, '--files-from', '/dev/null')
 
       log "Archiving data dump..."
-      FileUtils.cd(File.dirname("#{@dump_filename}")) do
+      FileUtils.cd(File.dirname(@dump_filename)) do
         execute_command(
-          "tar --append --dereference --file #{tar_filename} #{File.basename(@dump_filename)}",
+          'tar', '--append', '--dereference', '--file', tar_filename, File.basename(@dump_filename),
-          "Failed to archive data dump."
+          failure_message: "Failed to archive data dump."
         )
       end
 
@@ -232,8 +232,8 @@ module BackupRestore
       FileUtils.cd(File.join(Rails.root, "public")) do
         if File.directory?(upload_directory)
           execute_command(
-            "tar --append --dereference --file #{tar_filename} #{upload_directory}",
+            'tar', '--append', '--dereference', '--file', tar_filename, upload_directory,
-            "Failed to archive uploads."
+            failure_message: "Failed to archive uploads."
           )
         else
           log "No uploads found, skipping archiving uploads..."
@@ -243,7 +243,7 @@ module BackupRestore
       remove_tmp_directory
 
       log "Gzipping archive, this may take a while..."
-      execute_command("gzip -5 #{tar_filename}", "Failed to gzip archive.")
+      execute_command('gzip', '-5', tar_filename, failure_message: "Failed to gzip archive.")
     end
 
     def after_create_hook
@@ -277,7 +277,7 @@ module BackupRestore
 
     def remove_tar_leftovers
       log "Removing '.tar' leftovers..."
-      `rm -f #{@archive_directory}/*.tar`
+      system('rm', '-f', "#{@archive_directory}/*.tar")
     end
 
     def remove_tmp_directory

lib/backup_restore/restorer.rb

@@ -115,7 +115,7 @@ module BackupRestore
       # For backwards compatibility
       @dump_filename =
         if @is_archive
-          if system("tar --list --file #{@source_filename} #{BackupRestore::OLD_DUMP_FILE}")
+          if system('tar', '--list', '--file', @source_filename, BackupRestore::OLD_DUMP_FILE)
             File.join(@tmp_directory, BackupRestore::OLD_DUMP_FILE)
           else
             File.join(@tmp_directory, BackupRestore::DUMP_FILE)
@@ -176,7 +176,7 @@ module BackupRestore
 
     def copy_archive_to_tmp_directory
       log "Copying archive to tmp directory..."
-      execute_command("cp '#{@source_filename}' '#{@archive_filename}'", "Failed to copy archive to tmp directory.")
+      execute_command('cp', @source_filename, @archive_filename, failure_message: "Failed to copy archive to tmp directory.")
     end
 
     def unzip_archive
@@ -185,7 +185,7 @@ module BackupRestore
       log "Unzipping archive, this may take a while..."
 
       FileUtils.cd(@tmp_directory) do
-        execute_command("gzip --decompress '#{@archive_filename}'", "Failed to unzip archive.")
+        execute_command('gzip', '--decompress', @archive_filename, failure_message: "Failed to unzip archive.")
       end
     end
 
@@ -193,11 +193,11 @@ module BackupRestore
       log "Extracting metadata file..."
 
       @metadata =
-        if system("tar --list --file #{@source_filename} #{BackupRestore::METADATA_FILE}")
+        if system('tar', '--list', '--file', @source_filename, BackupRestore::METADATA_FILE)
           FileUtils.cd(@tmp_directory) do
             execute_command(
-              "tar --extract --file '#{@tar_filename}' #{BackupRestore::METADATA_FILE}",
+              'tar', '--extract', '--file', @tar_filename, BackupRestore::METADATA_FILE,
-              "Failed to extract metadata file."
+              failure_message: "Failed to extract metadata file."
             )
           end
 
@@ -232,8 +232,8 @@ module BackupRestore
 
       FileUtils.cd(@tmp_directory) do
         execute_command(
-          "tar --extract --file '#{@tar_filename}' #{File.basename(@dump_filename)}",
+          'tar', '--extract', '--file', @tar_filename, File.basename(@dump_filename),
-          "Failed to extract dump file."
+          failure_message: "Failed to extract dump file."
         )
       end
     end
@@ -292,7 +292,7 @@ module BackupRestore
         "--dbname='#{db_conf.database}'", # connect to database *dbname*
         "--single-transaction",           # all or nothing (also runs COPY commands faster)
         host_argument,                    # the hostname to connect to (if any)
-        port_argument,                # the port to connect to (if any)
+        port_argument,                    # the port to connect to (if any)
         username_argument                 # the username to connect as (if any)
       ].join(" ")
     end
@@ -362,8 +362,8 @@ module BackupRestore
         log "Extracting uploads..."
         FileUtils.cd(File.join(Rails.root, "public")) do
           execute_command(
-            "tar --extract --keep-newer-files --file '#{@tar_filename}' uploads/",
+            'tar', '--extract', '--keep-newer-files', '--file', @tar_filename, 'uploads/',
-            "Failed to extract uploads."
+            failure_message: "Failed to extract uploadsd."
           )
         end
       end

lib/backup_restore/utils.rb

@@ -1,14 +1,16 @@
+require 'open3'
+
 module BackupRestore
   module Utils
-    def execute_command(command, failure_message = "")
+    def execute_command(*command, failure_message: "")
-      output = `#{command} 2>&1`
+      stdout, stderr, status = Open3.capture3(*command)
 
-      if !$?.success?
+      if !status.success?
         failure_message = "#{failure_message}\n" if !failure_message.blank?
-        raise "#{failure_message}#{output}"
+        raise "#{failure_message}#{stderr}"
       end
 
-      output
+      stdout
     end
 
     def pretty_logs(logs)

spec/controllers/admin/backups_controller_spec.rb

@@ -194,6 +194,35 @@ describe Admin::BackupsController do
 
     end
 
+    describe "#upload_backup_chunk" do
+      describe "when filename contains invalid characters" do
+        it "should raise an error" do
+          ['灰色.tar.gz', '; echo \'haha\'.tar.gz'].each do |invalid_filename|
+            xhr :post, :upload_backup_chunk, resumableFilename: invalid_filename, resumableTotalSize: '1'
+
+            expect(response.status).to eq(415)
+            expect(response.body).to eq(I18n.t('backup.invalid_filename'))
+          end
+        end
+      end
+
+      describe "when filename is valid" do
+        it "should upload the file successfully" do
+          xhr :post, :upload_backup_chunk,
+            resumableFilename: 'test.tar.gz',
+            resumableTotalSize: '1',
+            resumableIdentifier: 'test',
+            resumableChunkNumber: '1',
+            resumableChunkSize: '1',
+            resumableCurrentChunkSize: '1',
+            file: fixture_file_upload(Tempfile.new)
+
+          expect(response.status).to eq(200)
+          expect(response.body).to eq("")
+        end
+      end
+    end
+
   end
 
 end