SECURITY: Safely decompress files. (#8124)

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

diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb
index 89c7ef8..9c16a0a 100644
--- a/app/controllers/admin/themes_controller.rb
+++ b/app/controllers/admin/themes_controller.rb
@@ -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
diff --git a/app/jobs/regular/export_csv_file.rb b/app/jobs/regular/export_csv_file.rb
index afd32e8..2ab9472 100644
--- a/app/jobs/regular/export_csv_file.rb
+++ b/app/jobs/regular/export_csv_file.rb
@@ -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
 
diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb
index 3f3cdec..c2cd50f 100644
--- a/app/models/remote_theme.rb
+++ b/app/models/remote_theme.rb
@@ -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)
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index e60a4d1..d352d9f 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -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"
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index dcf1d7c..8ef26ec 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -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"
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 3b09020..ba2fed7 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1222,6 +1222,9 @@ files:
     default: 5
     min: 0
     max: 20
+  decompressed_file_max_size_mb:
+    default: 1000
+    hidden: true
 
 trust:
   default_trust_level:
diff --git a/lib/compression/engine.rb b/lib/compression/engine.rb
new file mode 100644
index 0000000..2cbfef4
--- /dev/null
+++ b/lib/compression/engine.rb
@@ -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
diff --git a/lib/compression/gzip.rb b/lib/compression/gzip.rb
new file mode 100644
index 0000000..536c6c6
--- /dev/null
+++ b/lib/compression/gzip.rb
@@ -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

[... diff too long, it was truncated ...]

GitHub sha: 10565e46

will this be backported to stable?

@romanrizzi once you’ve confirmed it’s good in master we should backport it if it can be done cleanly.

This SECURITY issue is much less worrisome than most though, since you already need to be an admin.

We have discussed this internally and decided that we are not going to backport this to stable. The risk is mitigated by this being an admin-only feature.

1 Like