From 5dbe3b7b5558e8026476f9dc17110261202797ff Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 22 Aug 2023 21:30:33 +0300 Subject: [PATCH] SECURITY: Add limits for themes and theme assets This commit adds limits to themes and theme components on the: - file size of about.json and .discourse-compatibility - file size of theme assets - number of files in a theme --- app/models/remote_theme.rb | 64 ++++++++++++++++++++++++--- config/locales/server.en.yml | 4 ++ lib/discourse.rb | 1 + lib/theme_store.rb | 4 ++ lib/theme_store/git_importer.rb | 32 ++------------ lib/theme_store/importer.rb | 37 ++++++++++++++++ lib/theme_store/zip_importer.rb | 32 ++------------ lib/version.rb | 33 ++++++++++++-- spec/lib/version_spec.rb | 9 ++++ spec/models/remote_theme_spec.rb | 76 ++++++++++++++++++++++++++++++++ 10 files changed, 224 insertions(+), 68 deletions(-) create mode 100644 lib/theme_store.rb create mode 100644 lib/theme_store/importer.rb diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index f80bbf26980..9c020c4f6a4 100644 --- a/app/models/remote_theme.rb +++ b/app/models/remote_theme.rb @@ -27,6 +27,11 @@ class RemoteTheme < ActiveRecord::Base GITHUB_REGEXP = %r{\Ahttps?://github\.com/} GITHUB_SSH_REGEXP = %r{\Assh://git@github\.com:} + MAX_METADATA_FILE_SIZE = Discourse::MAX_METADATA_FILE_SIZE + MAX_ASSET_FILE_SIZE = 8.megabytes + MAX_THEME_FILE_COUNT = 1024 + MAX_THEME_SIZE = 256.megabytes + has_one :theme, autosave: false scope :joined_remotes, -> { @@ -41,11 +46,23 @@ class RemoteTheme < ActiveRecord::Base allow_nil: true def self.extract_theme_info(importer) - json = JSON.parse(importer["about.json"]) - json.fetch("name") - json - rescue TypeError, JSON::ParserError, KeyError - raise ImportError.new I18n.t("themes.import_error.about_json") + if importer.file_size("about.json") > MAX_METADATA_FILE_SIZE + raise ImportError.new I18n.t( + "themes.import_error.about_json_too_big", + limit: + ActiveSupport::NumberHelper.number_to_human_size( + MAX_METADATA_FILE_SIZE, + ), + ) + end + + begin + json = JSON.parse(importer["about.json"]) + json.fetch("name") + json + rescue TypeError, JSON::ParserError, KeyError + raise ImportError.new I18n.t("themes.import_error.about_json") + end end def self.update_zipped_theme( @@ -254,8 +271,43 @@ class RemoteTheme < ActiveRecord::Base ) end - importer.all_files.each do |filename| + all_files = importer.all_files + + if all_files.size > MAX_THEME_FILE_COUNT + raise ImportError, + I18n.t( + "themes.import_error.too_many_files", + count: all_files.size, + limit: MAX_THEME_FILE_COUNT, + ) + end + + theme_size = 0 + + all_files.each do |filename| next unless opts = ThemeField.opts_from_file_path(filename) + + file_size = importer.file_size(filename) + + if file_size > MAX_ASSET_FILE_SIZE + raise ImportError, + I18n.t( + "themes.import_error.asset_too_big", + filename: filename, + limit: ActiveSupport::NumberHelper.number_to_human_size(MAX_ASSET_FILE_SIZE), + ) + end + + theme_size += file_size + + if theme_size > MAX_THEME_SIZE + raise ImportError, + I18n.t( + "themes.import_error.theme_too_big", + limit: ActiveSupport::NumberHelper.number_to_human_size(MAX_THEME_SIZE), + ) + end + value = importer[filename] updated_fields << theme.set_field(**opts.merge(value: value)) end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 4e1d89668b8..51d0ebb1974 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -78,8 +78,11 @@ en: generic: An error occurred while importing that theme upload: "Error creating upload asset: %{name}. %{errors}" about_json: "Import Error: about.json does not exist, or is invalid. Are you sure this is a Discourse Theme?" + about_json_too_big: "Import Error: about.json is bigger than the %{limit} limit." about_json_values: "about.json contains invalid values: %{errors}" modifier_values: "about.json modifiers contain invalid values: %{errors}" + asset_too_big: "Asset %{filename} is bigger than the %{limit} limit" + theme_too_big: "Theme is bigger than the %{limit} limit" git: "Error cloning git repository, access is denied or repository is not found" git_ref_not_found: "Unable to checkout git reference: %{ref}" git_unsupported_scheme: "Unable to clone git repo: scheme unsupported" @@ -88,6 +91,7 @@ en: unknown_file_type: "The file you uploaded does not appear to be a valid Discourse theme." not_allowed_theme: "`%{repo}` is not in the list of allowed themes (check `allowed_theme_repos` global setting)." ssh_key_gone: "You waited too long to install the theme and SSH key expired. Please try again." + too_many_files: "The number of files (%{count}) in the theme has exceeded the maximum allowed number of files (%{limit})" errors: component_no_user_selectable: "Theme components can't be user-selectable" component_no_default: "Theme components can't be default theme" diff --git a/lib/discourse.rb b/lib/discourse.rb index 521b3ec2186..dc17e18f9c9 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -8,6 +8,7 @@ require "version" module Discourse DB_POST_MIGRATE_PATH ||= "db/post_migrate" REQUESTED_HOSTNAME ||= "REQUESTED_HOSTNAME" + MAX_METADATA_FILE_SIZE = 64.kilobytes class Utils URI_REGEXP ||= URI.regexp(%w[http https]) diff --git a/lib/theme_store.rb b/lib/theme_store.rb new file mode 100644 index 00000000000..09d92bed045 --- /dev/null +++ b/lib/theme_store.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +module ThemeStore +end diff --git a/lib/theme_store/git_importer.rb b/lib/theme_store/git_importer.rb index f23127a02c2..f354038d161 100644 --- a/lib/theme_store/git_importer.rb +++ b/lib/theme_store/git_importer.rb @@ -1,16 +1,14 @@ # frozen_string_literal: true -module ThemeStore -end - -class ThemeStore::GitImporter +class ThemeStore::GitImporter < ThemeStore::Importer COMMAND_TIMEOUT_SECONDS = 20 attr_reader :url def initialize(url, private_key: nil, branch: nil) + super + @url = GitUrl.normalize(url) - @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}" @private_key = private_key @branch = branch end @@ -60,30 +58,6 @@ class ThemeStore::GitImporter FileUtils.rm_rf(@temp_folder) end - def real_path(relative) - fullpath = "#{@temp_folder}/#{relative}" - return nil unless File.exist?(fullpath) - - # careful to handle symlinks here, don't want to expose random data - fullpath = Pathname.new(fullpath).realpath.to_s - - if fullpath && fullpath.start_with?(@temp_folder) - fullpath - else - nil - end - end - - def all_files - Dir.glob("**/*", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) } - end - - def [](value) - fullpath = real_path(value) - return nil unless fullpath - File.read(fullpath) - end - protected def redirected_uri diff --git a/lib/theme_store/importer.rb b/lib/theme_store/importer.rb new file mode 100644 index 00000000000..0797b1900e4 --- /dev/null +++ b/lib/theme_store/importer.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +class ThemeStore::Importer + def initialize(filename, original_filename) + @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}" + end + + def all_files + Dir.glob("**/*", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) } + end + + def [](value) + fullpath = real_path(value) + return nil unless fullpath + File.read(fullpath) + end + + def file_size(path) + fullpath = real_path(path) + return -1 unless fullpath + File.size(fullpath) + end + + def real_path(relative) + fullpath = "#{@temp_folder}/#{relative}" + return nil unless File.exist?(fullpath) + + # careful to handle symlinks here, don't want to expose random data + fullpath = Pathname.new(fullpath).realpath.to_s + + if fullpath && fullpath.start_with?(@temp_folder) + fullpath + else + nil + end + end +end diff --git a/lib/theme_store/zip_importer.rb b/lib/theme_store/zip_importer.rb index aed20bb83e2..95df290cc0a 100644 --- a/lib/theme_store/zip_importer.rb +++ b/lib/theme_store/zip_importer.rb @@ -2,14 +2,12 @@ require "compression/engine" -module ThemeStore -end - -class ThemeStore::ZipImporter +class ThemeStore::ZipImporter < ThemeStore::Importer attr_reader :url def initialize(filename, original_filename) - @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}" + super + @filename = filename @original_filename = original_filename end @@ -44,28 +42,4 @@ class ThemeStore::ZipImporter FileUtils.mv(Dir.glob("#{@temp_folder}/*/*"), @temp_folder) end end - - def real_path(relative) - fullpath = "#{@temp_folder}/#{relative}" - return nil unless File.exist?(fullpath) - - # careful to handle symlinks here, don't want to expose random data - fullpath = Pathname.new(fullpath).realpath.to_s - - if fullpath && fullpath.start_with?(@temp_folder) - fullpath - else - nil - end - end - - def all_files - Dir.glob("**/**", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) } - end - - def [](value) - fullpath = real_path(value) - return nil unless fullpath - File.read(fullpath) - end end diff --git a/lib/version.rb b/lib/version.rb index 89a9296e191..4f0634753ea 100644 --- a/lib/version.rb +++ b/lib/version.rb @@ -92,12 +92,37 @@ module Discourse # Find a compatible resource from a git repo def self.find_compatible_git_resource(path) return unless File.directory?("#{path}/.git") - compat_resource, std_error, s = - Open3.capture3( - "git -C '#{path}' show HEAD@{upstream}:#{Discourse::VERSION_COMPATIBILITY_FILENAME}", + + tree_info = + Discourse::Utils.execute_command( + "git", + "-C", + path, + "ls-tree", + "-l", + "HEAD", + Discourse::VERSION_COMPATIBILITY_FILENAME, ) - Discourse.find_compatible_resource(compat_resource) if s.success? + blob_size = tree_info.split[3].to_i + + if blob_size > Discourse::MAX_METADATA_FILE_SIZE + $stderr.puts "#{Discourse::VERSION_COMPATIBILITY_FILENAME} file in #{path} too big" + return + end + + compat_resource = + Discourse::Utils.execute_command( + "git", + "-C", + path, + "show", + "HEAD@{upstream}:#{Discourse::VERSION_COMPATIBILITY_FILENAME}", + ) + + Discourse.find_compatible_resource(compat_resource) rescue InvalidVersionListError => e $stderr.puts "Invalid version list in #{path}" + rescue Discourse::Utils::CommandError => e + nil end end diff --git a/spec/lib/version_spec.rb b/spec/lib/version_spec.rb index 6e54d1315ba..c2bacf3f056 100644 --- a/spec/lib/version_spec.rb +++ b/spec/lib/version_spec.rb @@ -177,5 +177,14 @@ RSpec.describe Discourse::VERSION do expect(output).to include("Invalid version list") end + + it "gracefully handles large .discourse-compatibility files" do + stub_const(Discourse, "MAX_METADATA_FILE_SIZE", 1) do + output = + capture_stderr { expect(Discourse.find_compatible_git_resource(git_directory)).to be_nil } + + expect(output).to include(Discourse::VERSION_COMPATIBILITY_FILENAME) + end + end end end diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 659bd5a3324..7eb772885ea 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -195,6 +195,40 @@ RSpec.describe RemoteTheme do expect(remote.reload.remote_version).to eq(new_version) expect(remote.reload.commits_behind).to eq(-1) end + + it "fails if theme has too many files" do + stub_const(RemoteTheme, "MAX_THEME_FILE_COUNT", 1) do + expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error( + RemoteTheme::ImportError, + I18n.t("themes.import_error.too_many_files", count: 14, limit: 1), + ) + end + end + + it "fails if files are too large" do + stub_const(RemoteTheme, "MAX_ASSET_FILE_SIZE", 1.byte) do + expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error( + RemoteTheme::ImportError, + I18n.t( + "themes.import_error.asset_too_big", + filename: "common/color_definitions.scss", + limit: ActiveSupport::NumberHelper.number_to_human_size(1), + ), + ) + end + end + + it "fails if theme is too large" do + stub_const(RemoteTheme, "MAX_THEME_SIZE", 1.byte) do + expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error( + RemoteTheme::ImportError, + I18n.t( + "themes.import_error.theme_too_big", + limit: ActiveSupport::NumberHelper.number_to_human_size(1), + ), + ) + end + end end let(:github_repo) do @@ -232,6 +266,48 @@ RSpec.describe RemoteTheme do end end + describe ".extract_theme_info" do + let(:importer) { mock } + + let(:theme_info) do + { + "name" => "My Theme", + "about_url" => "https://example.com/about", + "license_url" => "https://example.com/license", + } + end + + it "raises an error if about.json is too big" do + importer.stubs(:file_size).with("about.json").returns(100_000_000) + + expect { RemoteTheme.extract_theme_info(importer) }.to raise_error( + RemoteTheme::ImportError, + I18n.t( + "themes.import_error.about_json_too_big", + limit: + ActiveSupport::NumberHelper.number_to_human_size((RemoteTheme::MAX_METADATA_FILE_SIZE)), + ), + ) + end + + it "raises an error if about.json is invalid" do + importer.stubs(:file_size).with("about.json").returns(123) + importer.stubs(:[]).with("about.json").returns("{") + + expect { RemoteTheme.extract_theme_info(importer) }.to raise_error( + RemoteTheme::ImportError, + I18n.t("themes.import_error.about_json"), + ) + end + + it "returns extracted theme info" do + importer.stubs(:file_size).with("about.json").returns(123) + importer.stubs(:[]).with("about.json").returns(theme_info.to_json) + + expect(RemoteTheme.extract_theme_info(importer)).to eq(theme_info) + end + end + describe ".joined_remotes" do it "finds records that are associated with themes" do github_repo