SECURITY: Safely decompress files. (#8124)

* FEATURE: Adds an extra protection layer when decompressing files.

* Rename exporter/importer to zip importer. Update old locale

* Added a new composite class to decompress a file with multiple strategies

* Set max file size inside a site setting

* Ensure that file is deleted after compression

* Sanitize path and files before compressing/decompressing
This commit is contained in:
Roman Rizzi 2019-10-03 10:19:35 -03:00 committed by GitHub
parent aaf15944f8
commit 10565e4623
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
20 changed files with 466 additions and 101 deletions

View File

@ -92,7 +92,7 @@ class Admin::ThemesController < Admin::AdminController
theme_id = params[:theme_id]
match_theme_by_name = !!params[:bundle] && !params.key?(:theme_id) # Old theme CLI behavior, match by name. Remove Jan 2020
begin
@theme = RemoteTheme.update_tgz_theme(bundle.path, match_theme: match_theme_by_name, user: theme_user, theme_id: theme_id)
@theme = RemoteTheme.update_zipped_theme(bundle.path, bundle.original_filename, match_theme: match_theme_by_name, user: theme_user, theme_id: theme_id)
log_theme_change(nil, @theme)
render json: @theme, status: :created
rescue RemoteTheme::ImportError => e
@ -242,7 +242,7 @@ class Admin::ThemesController < Admin::AdminController
@theme = Theme.find_by(id: params[:id])
raise Discourse::InvalidParameters.new(:id) unless @theme
exporter = ThemeStore::TgzExporter.new(@theme)
exporter = ThemeStore::ZipExporter.new(@theme)
file_path = exporter.package_filename
headers['Content-Length'] = File.size(file_path).to_s

View File

@ -1,7 +1,6 @@
# frozen_string_literal: true
require 'csv'
require 'zip'
module Jobs
@ -51,14 +50,14 @@ module Jobs
FileUtils.mkdir_p(UserExport.base_directory) unless Dir.exists?(UserExport.base_directory)
# Generate a compressed CSV file
csv_to_export = CSV.generate do |csv|
csv << get_header if @entity != "report"
public_send(export_method).each { |d| csv << d }
end
compressed_file_path = "#{absolute_path}.zip"
Zip::File.open(compressed_file_path, Zip::File::CREATE) do |zipfile|
zipfile.get_output_stream(file_name) { |f| f.puts csv_to_export }
begin
CSV.open(absolute_path, "w") do |csv|
csv << get_header if @entity != "report"
public_send(export_method).each { |d| csv << d }
end
compressed_file_path = Compression::Zip.new.compress(UserExport.base_directory, file_name)
ensure
File.delete(absolute_path)
end
# create upload
@ -76,7 +75,7 @@ module Jobs
if upload.persisted?
user_export.update_columns(upload_id: upload.id)
else
Rails.logger.warn("Failed to upload the file #{Discourse.base_uri}/export_csv/#{file_name}.gz")
Rails.logger.warn("Failed to upload the file #{compressed_file_path}")
end
end

View File

@ -30,8 +30,8 @@ class RemoteTheme < ActiveRecord::Base
raise ImportError.new I18n.t("themes.import_error.about_json")
end
def self.update_tgz_theme(filename, match_theme: false, user: Discourse.system_user, theme_id: nil)
importer = ThemeStore::TgzImporter.new(filename)
def self.update_zipped_theme(filename, original_filename, match_theme: false, user: Discourse.system_user, theme_id: nil)
importer = ThemeStore::ZipImporter.new(filename, original_filename)
importer.import!
theme_info = RemoteTheme.extract_theme_info(importer)

View File

@ -3635,7 +3635,7 @@ en:
other: "Theme is {{count}} commits behind!"
compare_commits: "(See new commits)"
repo_unreachable: "Couldn't contact the Git repository of this theme. Error message:"
imported_from_archive: "This theme was imported from a .tar.gz file"
imported_from_archive: "This theme was imported from a .zip file"
scss:
text: "CSS"
title: "Enter custom CSS, we accept all valid CSS and SCSS styles"

View File

@ -85,6 +85,7 @@ en:
about_json_values: "about.json contains invalid values: %{errors}"
git: "Error cloning git repository, access is denied or repository is not found"
unpack_failed: "Failed to unpack file"
file_too_big: "The uncompressed file is too big."
unknown_file_type: "The file you uploaded does not appear to be a valid Discourse theme."
errors:
component_no_user_selectable: "Theme components can't be user-selectable"

View File

@ -1222,6 +1222,9 @@ files:
default: 5
min: 0
max: 20
decompressed_file_max_size_mb:
default: 1000
hidden: true
trust:
default_trust_level:

33
lib/compression/engine.rb Normal file
View File

@ -0,0 +1,33 @@
# frozen_string_literal: true
module Compression
class Engine
UnsupportedFileExtension = Class.new(StandardError)
def self.default_strategies
[
Compression::Zip.new,
Compression::Pipeline.new([Compression::Tar.new, Compression::Gzip.new]),
Compression::Gzip.new,
Compression::Tar.new
]
end
def self.engine_for(filename, strategies: default_strategies)
strategy = strategies.detect(-> { raise UnsupportedFileExtension }) { |e| e.can_handle?(filename) }
new(strategy)
end
def initialize(strategy)
@strategy = strategy
end
def decompress(dest_path, compressed_file_path, allow_non_root_folder: false)
@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

57
lib/compression/gzip.rb Normal file
View File

@ -0,0 +1,57 @@
# frozen_string_literal: true
module Compression
class Gzip < Strategy
def extension
'.gz'
end
def compress(path, target_name)
gzip_target = sanitize_path("#{path}/#{target_name}")
Discourse::Utils.execute_command('gzip', '-5', gzip_target, failure_message: "Failed to gzip file.")
"#{gzip_target}.gz"
end
private
def entries_of(compressed_file)
[compressed_file]
end
def is_file?(_)
true
end
def extract_folder(_entry, _entry_path); end
def get_compressed_file_stream(compressed_file_path)
gzip = Zlib::GzipReader.open(compressed_file_path)
yield(gzip)
end
def build_entry_path(_compressed_file, dest_path, compressed_file_path, entry, _allow_non_root_folder)
compressed_file_path.gsub(extension, '')
end
def extract_file(entry, entry_path, available_size)
remaining_size = available_size
if ::File.exist?(entry_path)
raise ::Zip::DestinationFileExistsError,
"Destination '#{entry_path}' already exists"
end # Change this later.
::File.open(entry_path, 'wb') do |os|
buf = ''.dup
while (buf = entry.read(chunk_size))
remaining_size -= chunk_size
raise ExtractFailed if remaining_size.negative?
os << buf
end
end
remaining_size
end
end
end

View File

@ -0,0 +1,32 @@
# frozen_string_literal: true
module Compression
class Pipeline < Strategy
def initialize(strategies)
@strategies = strategies
end
def extension
@strategies.reduce('') { |ext, strategy| ext += strategy.extension }
end
def compress(path, target_name)
current_target = target_name
@strategies.reduce('') do |compressed_path, strategy|
compressed_path = strategy.compress(path, current_target)
current_target = compressed_path.split('/').last
compressed_path
end
end
def decompress(dest_path, compressed_file_path, allow_non_root_folder: false)
to_decompress = compressed_file_path
@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, '')
end
end
end
end

View File

@ -0,0 +1,88 @@
# frozen_string_literal: true
module Compression
class Strategy
ExtractFailed = Class.new(StandardError)
DestinationFileExistsError = Class.new(StandardError)
def can_handle?(file_name)
file_name.include?(extension)
end
def decompress(dest_path, compressed_file_path, allow_non_root_folder: false)
sanitized_compressed_file_path = sanitize_path(compressed_file_path)
get_compressed_file_stream(sanitized_compressed_file_path) do |compressed_file|
available_size = calculate_available_size
entries_of(compressed_file).each do |entry|
entry_path = build_entry_path(
compressed_file, sanitize_path(dest_path),
sanitized_compressed_file_path, entry,
allow_non_root_folder
)
if is_file?(entry)
remaining_size = extract_file(entry, entry_path, available_size)
available_size = remaining_size
else
extract_folder(entry, entry_path)
end
end
end
end
private
def sanitize_path(filename)
Pathname.new(filename).realpath.to_s
end
# https://guides.rubyonrails.org/security.html#file-uploads
def sanitize_filename(filename)
filename.strip.tap do |name|
# NOTE: File.basename doesn't work right with Windows paths on Unix
# get only the filename, not the whole path
name.sub! /\A.*(\\|\/)/, ''
# Finally, replace all non alphanumeric, underscore
# or periods with underscore
name.gsub! /[^\w\.\-]/, '_'
end
end
def calculate_available_size
1024**2 * (SiteSetting.decompressed_file_max_size_mb / 1.049) # Mb to Mib
end
def entries_of(compressed_file)
compressed_file
end
def is_file?(entry)
entry.file?
end
def chunk_size
@chunk_size ||= 1024**2 * 2 # 2MiB
end
def extract_file(entry, entry_path, available_size)
remaining_size = available_size
if ::File.exist?(entry_path)
raise DestinationFileExistsError, "Destination '#{entry_path}' already exists"
end
::File.open(entry_path, 'wb') do |os|
buf = ''.dup
while (buf = entry.read(chunk_size))
remaining_size -= chunk_size
raise ExtractFailed if remaining_size.negative?
os << buf
end
end
remaining_size
end
end
end

35
lib/compression/tar.rb Normal file
View File

@ -0,0 +1,35 @@
# frozen_string_literal: true
require 'rubygems/package'
module Compression
class Tar < Strategy
def extension
'.tar'
end
def compress(path, target_name)
tar_filename = sanitize_filename("#{target_name}.tar")
Discourse::Utils.execute_command('tar', '--create', '--file', tar_filename, target_name, failure_message: "Failed to tar file.")
sanitize_path("#{path}/#{tar_filename}")
end
private
def extract_folder(_entry, _entry_path); end
def get_compressed_file_stream(compressed_file_path)
file_stream = IO.new(IO.sysopen(compressed_file_path))
tar_extract = Gem::Package::TarReader.new(file_stream)
tar_extract.rewind
yield(tar_extract)
end
def build_entry_path(_compressed_file, dest_path, compressed_file_path, entry, _allow_non_root_folder)
File.join(dest_path, entry.full_name).tap do |entry_path|
FileUtils.mkdir_p(File.dirname(entry_path))
end
end
end
end

99
lib/compression/zip.rb Normal file
View File

@ -0,0 +1,99 @@
# frozen_string_literal: true
require 'zip'
module Compression
class Zip < Strategy
def extension
'.zip'
end
def compress(path, target_name)
absolute_path = sanitize_path("#{path}/#{target_name}")
zip_filename = "#{absolute_path}.zip"
::Zip::File.open(zip_filename, ::Zip::File::CREATE) do |zipfile|
if File.directory?(absolute_path)
entries = Dir.entries(absolute_path) - %w[. ..]
write_entries(entries, absolute_path, '', zipfile)
else
put_into_archive(absolute_path, zipfile, target_name)
end
end
zip_filename
end
private
def extract_folder(entry, entry_path)
entry.extract(entry_path)
end
def get_compressed_file_stream(compressed_file_path)
zip_file = ::Zip::File.open(compressed_file_path)
yield(zip_file)
end
def build_entry_path(compressed_file, dest_path, compressed_file_path, entry, allow_non_root_folder)
folder_name = compressed_file_path.split('/').last.gsub('.zip', '')
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)
filenames.map { |p| p.name.split('/').first }.uniq.size == 1 || allow_non_root_folder
end
def extract_file(entry, entry_path, available_size)
remaining_size = available_size
if ::File.exist?(entry_path)
raise ::Zip::DestinationFileExistsError,
"Destination '#{entry_path}' already exists"
end
::File.open(entry_path, 'wb') do |os|
entry.get_input_stream do |is|
entry.set_extra_attributes_on_path(entry_path)
buf = ''.dup
while (buf = is.sysread(chunk_size, buf))
remaining_size -= chunk_size
raise ExtractFailed if remaining_size.negative?
os << buf
end
end
end
remaining_size
end
# A helper method to make the recursion work.
def write_entries(entries, base_path, path, zipfile)
entries.each do |e|
zipfile_path = path == '' ? e : File.join(path, e)
disk_file_path = File.join(base_path, zipfile_path)
if File.directory? disk_file_path
recursively_deflate_directory(disk_file_path, zipfile, base_path, zipfile_path)
else
put_into_archive(disk_file_path, zipfile, zipfile_path)
end
end
end
def recursively_deflate_directory(disk_file_path, zipfile, base_path, zipfile_path)
zipfile.mkdir zipfile_path
subdir = Dir.entries(disk_file_path) - %w[. ..]
write_entries subdir, base_path, zipfile_path, zipfile
end
def put_into_archive(disk_file_path, zipfile, zipfile_path)
zipfile.add(zipfile_path, disk_file_path)
end
end
end

View File

@ -1,60 +0,0 @@
# frozen_string_literal: true
require 'zip'
module ImportExport
class ZipUtils
def zip_directory(path, export_name)
zip_filename = "#{export_name}.zip"
absolute_path = "#{path}/#{export_name}"
entries = Dir.entries(absolute_path) - %w[. ..]
Zip::File.open(zip_filename, Zip::File::CREATE) do |zipfile|
write_entries(entries, absolute_path, '', zipfile)
end
"#{absolute_path}.zip"
end
def unzip_directory(path, zip_filename, allow_non_root_folder: false)
Zip::File.open(zip_filename) do |zip_file|
root = root_folder_present?(zip_file, allow_non_root_folder) ? '' : 'unzipped/'
zip_file.each do |entry|
entry_path = File.join(path, "#{root}#{entry.name}")
FileUtils.mkdir_p(File.dirname(entry_path))
entry.extract(entry_path)
end
end
end
private
def root_folder_present?(filenames, allow_non_root_folder)
filenames.map { |p| p.name.split('/').first }.uniq.size == 1 || allow_non_root_folder
end
# A helper method to make the recursion work.
def write_entries(entries, base_path, path, zipfile)
entries.each do |e|
zipfile_path = path == '' ? e : File.join(path, e)
disk_file_path = File.join(base_path, zipfile_path)
if File.directory? disk_file_path
recursively_deflate_directory(disk_file_path, zipfile, base_path, zipfile_path)
else
put_into_archive(disk_file_path, zipfile, zipfile_path)
end
end
end
def recursively_deflate_directory(disk_file_path, zipfile, base_path, zipfile_path)
zipfile.mkdir zipfile_path
subdir = Dir.entries(disk_file_path) - %w[. ..]
write_entries subdir, base_path, zipfile_path, zipfile
end
def put_into_archive(disk_file_path, zipfile, zipfile_path)
zipfile.add(zipfile_path, disk_file_path)
end
end
end

View File

@ -30,7 +30,7 @@ class ThemeStore::GitImporter
raise Discourse::InvalidParameters.new(:id) unless theme
local_version = theme.remote_theme&.local_version
exporter = ThemeStore::TgzExporter.new(theme)
exporter = ThemeStore::ZipExporter.new(theme)
local_temp_folder = exporter.export_to_folder
Dir.chdir(@temp_folder) do

View File

@ -1,10 +1,10 @@
# frozen_string_literal: true
require 'import_export/zip_utils'
require_dependency 'compression/zip'
module ThemeStore; end
class ThemeStore::TgzExporter
class ThemeStore::ZipExporter
def initialize(theme)
@theme = theme
@ -62,6 +62,6 @@ class ThemeStore::TgzExporter
def export_package
export_to_folder
Dir.chdir(@temp_folder) { ImportExport::ZipUtils.new.zip_directory(@temp_folder, @export_name) }
Dir.chdir(@temp_folder) { Compression::Zip.new.compress(@temp_folder, @export_name) }
end
end

View File

@ -1,33 +1,34 @@
# frozen_string_literal: true
require 'import_export/zip_utils'
require_dependency 'compression/engine'
module ThemeStore; end
class ThemeStore::TgzImporter
class ThemeStore::ZipImporter
attr_reader :url
def initialize(filename)
def initialize(filename, original_filename)
@temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}"
@filename = filename
@original_filename = original_filename
end
def import!
FileUtils.mkdir(@temp_folder)
Dir.chdir(@temp_folder) do
if @filename.include?('.zip')
ImportExport::ZipUtils.new.unzip_directory(@temp_folder, @filename)
# --strip 1 equivalent
FileUtils.mv(Dir.glob("#{@temp_folder}/*/*"), @temp_folder)
else
Discourse::Utils.execute_command("tar", "-xzvf", @filename, "--strip", "1")
Compression::Engine.engine_for(@original_filename).tap do |engine|
engine.decompress(@temp_folder, @filename)
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")
rescue Compression::Zip::ExtractFailed
raise RemoteTheme::ImportError, I18n.t("themes.import_error.file_too_big")
end
def cleanup!

View File

@ -1,9 +1,9 @@
# frozen_string_literal: true
require 'rails_helper'
require 'theme_store/tgz_exporter'
require 'theme_store/zip_exporter'
describe ThemeStore::TgzExporter do
describe ThemeStore::ZipExporter do
let!(:theme) do
Fabricate(:theme, name: "Header Icons").tap do |theme|
theme.set_field(target: :common, name: :body_tag, value: "<b>testtheme1</b>")
@ -51,7 +51,7 @@ describe ThemeStore::TgzExporter do
end
let(:package) do
exporter = ThemeStore::TgzExporter.new(theme)
exporter = ThemeStore::ZipExporter.new(theme)
filename = exporter.package_filename
FileUtils.cp(filename, dir)
exporter.cleanup!
@ -63,7 +63,7 @@ describe ThemeStore::TgzExporter do
file = 'discourse-header-icons.zip'
dest = 'discourse-header-icons'
Dir.chdir(dir) do
ImportExport::ZipUtils.new.unzip_directory(dir, file, allow_non_root_folder: true)
Compression::Zip.new.decompress(dir, file, allow_non_root_folder: true)
`rm #{file}`
folders = Dir.glob("**/*").reject { |f| File.file?(f) }
@ -120,7 +120,7 @@ describe ThemeStore::TgzExporter do
it "doesn't prepend 'discourse' to filename if already there" do
theme.update!(name: "Discourse Header Icons")
exporter = ThemeStore::TgzExporter.new(theme)
exporter = ThemeStore::ZipExporter.new(theme)
filename = exporter.package_filename
exporter.cleanup!
expect(filename).to end_with "/discourse-header-icons.zip"

View File

@ -3,10 +3,9 @@
# frozen_string_literal: true
require 'rails_helper'
require 'theme_store/tgz_importer'
require 'import_export/zip_utils'
require 'theme_store/zip_importer'
describe ThemeStore::TgzImporter do
describe ThemeStore::ZipImporter do
before do
@temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}"
@ -24,11 +23,12 @@ describe ThemeStore::TgzImporter do
it "can import a simple zipped theme" do
Dir.chdir(@temp_folder) do
ImportExport::ZipUtils.new.zip_directory(@temp_folder, 'test')
Compression::Zip.new.compress(@temp_folder, 'test')
FileUtils.rm_rf('test/')
end
importer = ThemeStore::TgzImporter.new("#{@temp_folder}/test.zip")
file_name = 'test.zip'
importer = ThemeStore::ZipImporter.new("#{@temp_folder}/#{file_name}", file_name)
importer.import!
expect(importer["hello.txt"]).to eq("hello world")
@ -42,7 +42,8 @@ describe ThemeStore::TgzImporter do
`tar -cvzf test.tar.gz test/* 2> /dev/null`
end
importer = ThemeStore::TgzImporter.new("#{@temp_folder}/test.tar.gz")
file_name = 'test.tar.gz'
importer = ThemeStore::ZipImporter.new("#{@temp_folder}/#{file_name}", file_name)
importer.import!
expect(importer["hello.txt"]).to eq("hello world")

View File

@ -0,0 +1,76 @@
# frozen_string_literal: true
require 'rails_helper'
describe Compression::Engine do
before do
@temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/#{SecureRandom.hex}"
@folder_name = 'test'
FileUtils.mkdir(@temp_folder)
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 }
it 'raises an exception when the file is not supported' do
unknown_extension = 'a_file.crazyext'
expect { described_class.engine_for(unknown_extension) }.to raise_error Compression::Engine::UnsupportedFileExtension
end
describe 'compressing and decompressing files' do
before do
Dir.chdir(@temp_folder) do
@compressed_path = Compression::Engine.engine_for("#{@folder_name}#{extension}").compress(@temp_folder, @folder_name)
FileUtils.rm_rf("#{@folder_name}/")
end
end
context 'working with zip files' do
let(:extension) { '.zip' }
it 'decompress the folder and inspect files correctly' do
engine = described_class.engine_for(@compressed_path)
engine.decompress(@temp_folder, "#{@temp_folder}/#{@folder_name}.zip")
expect(read_file("test/hello.txt")).to eq("hello world")
expect(read_file("test/a/inner")).to eq("hello world inner")
end
end
context 'working with .tar.gz files' do
let(:extension) { '.tar.gz' }
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.gz")
expect(read_file("test/hello.txt")).to eq("hello world")
expect(read_file("test/a/inner")).to eq("hello world inner")
end
end
context 'working with .tar files' do
let(:extension) { '.tar' }
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")
expect(read_file("test/hello.txt")).to eq("hello world")
expect(read_file("test/a/inner")).to eq("hello world inner")
end
end
end
def read_file(relative_path)
File.read("#{@temp_folder}/#{relative_path}")
end
end

View File

@ -50,7 +50,7 @@ describe Admin::ThemesController do
expect(response.status).to eq(200)
# Save the output in a temp file (automatically cleaned up)
file = Tempfile.new('archive.tar.zip')
file = Tempfile.new('archive.zip')
file.write(response.body)
file.rewind
uploaded_file = Rack::Test::UploadedFile.new(file.path, "application/zip")