From f63a797e390ed79b643cfad42837a67068549fbf Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 16 Sep 2016 10:32:53 +0800 Subject: [PATCH 1/2] SECUIRTY: Escape input made to system calls. --- lib/backup_restore/backuper.rb | 24 ++++++++++++------------ lib/backup_restore/restorer.rb | 22 +++++++++++----------- lib/backup_restore/utils.rb | 12 +++++++----- 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/lib/backup_restore/backuper.rb b/lib/backup_restore/backuper.rb index 4d2e0c9f09f..bc885e8fe12 100644 --- a/lib/backup_restore/backuper.rb +++ b/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)}", - "Failed to move database dump file." + 'mv', @dump_filename, File.join(@archive_directory, @backup_filename), + 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}.gz") + execute_command('rm', '-f', tar_filename) + 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)}", - "Failed to archive data dump." + 'tar', '--append', '--dereference', '--file', tar_filename, File.basename(@dump_filename), + 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}", - "Failed to archive uploads." + 'tar', '--append', '--dereference', '--file', tar_filename, upload_directory, + 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 diff --git a/lib/backup_restore/restorer.rb b/lib/backup_restore/restorer.rb index bb10f0503ac..cf9ff136798 100644 --- a/lib/backup_restore/restorer.rb +++ b/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}", - "Failed to extract metadata file." + 'tar', '--extract', '--file', @tar_filename, BackupRestore::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)}", - "Failed to extract dump file." + 'tar', '--extract', '--file', @tar_filename, File.basename(@dump_filename), + 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/", - "Failed to extract uploads." + 'tar', '--extract', '--keep-newer-files', '--file', @tar_filename, 'uploads/', + failure_message: "Failed to extract uploadsd." ) end end diff --git a/lib/backup_restore/utils.rb b/lib/backup_restore/utils.rb index f5021f1d3f1..b782f61bee6 100644 --- a/lib/backup_restore/utils.rb +++ b/lib/backup_restore/utils.rb @@ -1,14 +1,16 @@ +require 'open3' + module BackupRestore module Utils - def execute_command(command, failure_message = "") - output = `#{command} 2>&1` + def execute_command(*command, failure_message: "") + 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) From 512922d7767c84615b8b77089e34ede244a88c7f Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 16 Sep 2016 11:56:22 +0800 Subject: [PATCH 2/2] SECURITY: Add filename validation for backup uploads. --- app/controllers/admin/backups_controller.rb | 1 + config/locales/server.en.yml | 1 + .../admin/backups_controller_spec.rb | 29 +++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/app/controllers/admin/backups_controller.rb b/app/controllers/admin/backups_controller.rb index b085dc81f68..6222fc2b088 100644 --- a/app/controllers/admin/backups_controller.rb +++ b/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) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c9ba20b69de..09554bb945a 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/spec/controllers/admin/backups_controller_spec.rb b/spec/controllers/admin/backups_controller_spec.rb index 82b9e69dd53..dc44007ca3e 100644 --- a/spec/controllers/admin/backups_controller_spec.rb +++ b/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