diff --git a/lib/compression/engine.rb b/lib/compression/engine.rb index 39ee9e98a62..c098c0b6687 100644 --- a/lib/compression/engine.rb +++ b/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 diff --git a/lib/compression/gzip.rb b/lib/compression/gzip.rb index 536c6c64e4b..4d0b7c51e15 100644 --- a/lib/compression/gzip.rb +++ b/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) - compressed_file_path.gsub(extension, '') + def build_entry_path(dest_path, _, compressed_file_path) + 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) diff --git a/lib/compression/pipeline.rb b/lib/compression/pipeline.rb index 98a93cce414..1ea9f17ddb2 100644 --- a/lib/compression/pipeline.rb +++ b/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 - strategy.decompress(dest_path, to_decompress, max_size, allow_non_root_folder: allow_non_root_folder) - to_decompress.gsub(last_extension, '') + next_compressed_file = strategy.decompress(dest_path, to_decompress, max_size) + FileUtils.rm_rf(to_decompress) + next_compressed_file end end end diff --git a/lib/compression/strategy.rb b/lib/compression/strategy.rb index 5e7088cc6c5..8bb54f87d4f 100644 --- a/lib/compression/strategy.rb +++ b/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( - compressed_file, sanitize_path(dest_path), - sanitized_compressed_file_path, entry, - allow_non_root_folder - ) + entry_path = build_entry_path(sanitized_dest_path, entry, sanitized_compressed_file_path) + if !is_safe_path_for_extraction?(entry_path, sanitized_dest_path) + next + 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 diff --git a/lib/compression/tar.rb b/lib/compression/tar.rb index 6e857949f71..580a8ebdd5d 100644 --- a/lib/compression/tar.rb +++ b/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) - File.join(dest_path, entry.full_name).tap do |entry_path| - FileUtils.mkdir_p(File.dirname(entry_path)) - end + def build_entry_path(dest_path, entry, _) + File.join(dest_path, entry.full_name) + end + + def decompression_results_path(dest_path, _) + dest_path end end end diff --git a/lib/compression/zip.rb b/lib/compression/zip.rb index 2fc953545a9..63c6b927299 100644 --- a/lib/compression/zip.rb +++ b/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) - 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 + def build_entry_path(dest_path, entry, _) + File.join(dest_path, entry.name) end - def root_folder_present?(filenames, allow_non_root_folder) - filenames.map { |p| p.name.split('/').first }.uniq.size == 1 || allow_non_root_folder + def decompression_results_path(dest_path, _) + dest_path end def extract_file(entry, entry_path, available_size) diff --git a/lib/theme_store/zip_importer.rb b/lib/theme_store/zip_importer.rb index ff98ea49019..f8bed2349bf 100644 --- a/lib/theme_store/zip_importer.rb +++ b/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) diff --git a/spec/lib/compression/engine_spec.rb b/spec/lib/compression/engine_spec.rb index 6e96e5a226f..6ca9a969ed1 100644 --- a/spec/lib/compression/engine_spec.rb +++ b/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}" - @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') + 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 - @compressed_path = Compression::Engine.engine_for("#{@folder_name}#{extension}").compress(@temp_folder, @folder_name) - FileUtils.rm_rf("#{@folder_name}/") + 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 '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("test/a/inner")).to eq("hello world inner") + expect(read_file("extract_location/hello.txt")).to eq("hello world") + 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 diff --git a/spec/lib/theme_store/zip_exporter_spec.rb b/spec/lib/theme_store/zip_exporter_spec.rb index 3efdd7f6afe..05d468bb6b1 100644 --- a/spec/lib/theme_store/zip_exporter_spec.rb +++ b/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) }