From 290306a93290c0818372d8dc6ce026046ad48335 Mon Sep 17 00:00:00 2001
From: Daniel Waterworth <me@danielwaterworth.com>
Date: Fri, 1 Sep 2023 12:22:58 -0500
Subject: [PATCH] SECURITY: Reduce maximum size of SVG sprite cache to prevent
DoS
Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>
---
app/models/theme_field.rb | 8 ++-
lib/svg_sprite.rb | 58 +++++++++++--------
.../custom-theme-component-icon-sprite.svg | 9 +++
spec/lib/svg_sprite/svg_sprite_spec.rb | 58 +++++++++++++++++++
spec/models/theme_field_spec.rb | 24 ++++++++
spec/rails_helper.rb | 6 ++
6 files changed, 137 insertions(+), 26 deletions(-)
create mode 100644 spec/fixtures/images/custom-theme-component-icon-sprite.svg
diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb
index 1fff6f85f77..5ed57393014 100644
--- a/app/models/theme_field.rb
+++ b/app/models/theme_field.rb
@@ -204,7 +204,11 @@ class ThemeField < ActiveRecord::Base
begin
content = File.read(path)
- Nokogiri.XML(content) { |config| config.options = Nokogiri::XML::ParseOptions::NOBLANKS }
+ if content.to_s.bytesize > SvgSprite::MAX_THEME_SPRITE_SIZE
+ error = "Error with #{self.name}: Icon sprite file is too large"
+ else
+ Nokogiri.XML(content) { |config| config.options = Nokogiri::XML::ParseOptions::NOBLANKS }
+ end
rescue => e
error = "Error with #{self.name}: #{e.inspect}"
end
@@ -667,7 +671,7 @@ class ThemeField < ActiveRecord::Base
rescue => e
Discourse.warn_exception(e, message: "Failed to fetch svg sprite for theme field #{id}")
else
- if content.length > 4 * 1024**2
+ if content.length > SvgSprite::MAX_THEME_SPRITE_SIZE
Rails.logger.warn(
"can't store theme svg sprite for theme #{theme_id} and upload #{upload_id}, sprite too big",
)
diff --git a/lib/svg_sprite.rb b/lib/svg_sprite.rb
index 1217bfa26f9..ba1c59b7c63 100644
--- a/lib/svg_sprite.rb
+++ b/lib/svg_sprite.rb
@@ -244,6 +244,8 @@ module SvgSprite
THEME_SPRITE_VAR_NAME = "icons-sprite"
+ MAX_THEME_SPRITE_SIZE = 512.kilobytes
+
def self.preload
settings_icons
group_icons
@@ -298,37 +300,45 @@ module SvgSprite
def self.theme_svgs(theme_id)
if theme_id.present?
- theme_ids = Theme.transform_ids(theme_id)
+ cache
+ .defer_get_set_bulk(
+ Theme.transform_ids(theme_id),
+ lambda { |theme_id| "theme_svg_sprites_#{theme_id}" },
+ ) do |theme_ids|
+ theme_field_uploads =
+ ThemeField.where(
+ type_id: ThemeField.types[:theme_upload_var],
+ name: THEME_SPRITE_VAR_NAME,
+ theme_id: theme_ids,
+ ).pluck(:upload_id)
- get_set_cache("theme_svg_sprites_#{theme_ids.join(",")}") do
- theme_field_uploads =
- ThemeField.where(
- type_id: ThemeField.types[:theme_upload_var],
- name: THEME_SPRITE_VAR_NAME,
- theme_id: theme_ids,
- ).pluck(:upload_id)
+ theme_sprites =
+ ThemeSvgSprite.where(theme_id: theme_ids).pluck(:theme_id, :upload_id, :sprite)
+ missing_sprites = (theme_field_uploads - theme_sprites.map(&:second))
- theme_sprites = ThemeSvgSprite.where(theme_id: theme_ids).pluck(:upload_id, :sprite)
- missing_sprites = (theme_field_uploads - theme_sprites.map(&:first))
-
- if missing_sprites.present?
- Rails.logger.warn(
- "Missing ThemeSvgSprites for theme #{theme_id}, uploads #{missing_sprites.join(", ")}",
- )
- end
-
- theme_sprites.reduce({}) do |symbols, (upload_id, sprite)|
- begin
- symbols.merge!(symbols_for("theme_#{theme_id}_#{upload_id}.svg", sprite, strict: false))
- rescue => e
+ if missing_sprites.present?
Rails.logger.warn(
- "Bad XML in custom sprite in theme with ID=#{theme_id}. Error info: #{e.inspect}",
+ "Missing ThemeSvgSprites for theme #{theme_id}, uploads #{missing_sprites.join(", ")}",
)
end
- symbols
+ theme_sprites
+ .map do |(theme_id, upload_id, sprite)|
+ begin
+ [theme_id, symbols_for("theme_#{theme_id}_#{upload_id}.svg", sprite, strict: false)]
+ rescue => e
+ Rails.logger.warn(
+ "Bad XML in custom sprite in theme with ID=#{theme_id}. Error info: #{e.inspect}",
+ )
+ end
+ end
+ .compact
+ .to_h
+ .values_at(*theme_ids)
end
- end
+ .values
+ .compact
+ .reduce({}) { |a, b| a.merge!(b) }
else
{}
end
diff --git a/spec/fixtures/images/custom-theme-component-icon-sprite.svg b/spec/fixtures/images/custom-theme-component-icon-sprite.svg
new file mode 100644
index 00000000000..901894ececa
--- /dev/null
+++ b/spec/fixtures/images/custom-theme-component-icon-sprite.svg
@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
+ <symbol id="my-other-custom-theme-icon" viewBox="0 0 496 512">
+ <path d="M248 8C111.03 8 0 119.03 0 256s111.03 248 248 248 248-111.03 248-248S384.97 8 248 8zm0 376c-17.67 0-32-14.33-32-32s14.33-32 32-32 32 14.33 32 32-14.33 32-32 32zm0-128c-53.02 0-96 42.98-96 96s42.98 96 96 96c-106.04 0-192-85.96-192-192S141.96 64 248 64c53.02 0 96 42.98 96 96s-42.98 96-96 96zm0-128c-17.67 0-32 14.33-32 32s14.33 32 32 32 32-14.33 32-32-14.33-32-32-32z"></path>
+ </symbol>
+ <symbol class="id-missing" viewBox="0 0 64 64">
+ <path d="M18.679,56.392c-0.441,0-0.885-0.097-1.298-0.295c-1.04-0.5-1.702-1.551-1.702-2.705V10.608 c0-1.155,0.663-2.207,1.704-2.706c1.04-0.5,2.276-0.356,3.176,0.368l26.641,21.429c0.708,0.57,1.121,1.431,1.12,2.34 c-0.001,0.91-0.414,1.77-1.124,2.338L20.556,55.733C20.013,56.168,19.349,56.392,18.679,56.392z M21.68,16.871v30.271 l18.849-15.11L21.68,16.871z"/>
+ </symbol>
+</svg>
diff --git a/spec/lib/svg_sprite/svg_sprite_spec.rb b/spec/lib/svg_sprite/svg_sprite_spec.rb
index e928280b7c1..4d7747c8617 100644
--- a/spec/lib/svg_sprite/svg_sprite_spec.rb
+++ b/spec/lib/svg_sprite/svg_sprite_spec.rb
@@ -275,6 +275,30 @@ RSpec.describe SvgSprite do
expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/)
end
+ it "includes custom icons in a theme and an attached theme component" do
+ theme_component = Fabricate(:theme, component: true)
+ theme.add_relative_theme!(:child, theme_component)
+
+ fname1 = "custom-theme-icon-sprite.svg"
+ fname2 = "custom-theme-component-icon-sprite.svg"
+
+ [[theme, fname1], [theme_component, fname2]].each do |t, fname|
+ upload = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1)
+ expect(Upload.exists?(id: upload.id)).to eq(true)
+
+ t.set_field(
+ target: :common,
+ name: SvgSprite.theme_sprite_variable_name,
+ upload_id: upload.id,
+ type: :theme_upload_var,
+ )
+ t.save!
+ end
+
+ expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/)
+ expect(SvgSprite.bundle(theme.id)).to match(/my-other-custom-theme-icon/)
+ end
+
it "does not fail on bad XML in custom icon sprite" do
fname = "bad-xml-icon-sprite.svg"
@@ -310,5 +334,39 @@ RSpec.describe SvgSprite do
expect(Upload.exists?(id: upload.id)).to eq(true)
expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/)
end
+
+ it "does not include theme icons if custom icon sprite is too large" do
+ fname = "theme-icon-sprite.svg"
+ symbols = ""
+
+ 1500.times do |i|
+ id = "icon-id-#{i}"
+ path =
+ "M#{rand(1..100)} 2.18-2.087.277-4.216.42-6.378.42s-4.291-.143-6.378-.42c-1.085-.144-1.872-1.086-1.872-2.18v-4.25m16.5 0a2.18 2.18 0 00.75-1.661V8.706c0-1.081-.768-2.015-1.837-2.175a48.114 48.114 0 00-3.413-.387m4.5 8.006c-.194.165-.42.295-.673.38A23.978 23.978 0 0112 15.75c-2.648 0-5.195-.429-7.577-1.22a2.016 2.016 0 01-.673-.38m0 0A2.18 2.18 0 013 12.489V8.706c0-1.081.768-2.015 1.837-2.175a48.111 48.111 0 013.413-.387m7.5 0V5.25A2.25 2.25 0 0013.5 3h-3a2.25 .008z"
+ symbols += "<symbol id='#{id}' viewBox='0 0 100 100'><path d='#{path}'/></symbol>\n"
+ end
+
+ contents =
+ "<?xml version='1.0' encoding='UTF-8'?><svg><symbol id='customthemeicon' viewBox='0 0 100 100'><path d='M0 0h1ssss00v100H0z'/></symbol>#{symbols}</svg>"
+
+ child_theme = Fabricate(:theme, component: true)
+ theme.add_relative_theme!(:child, child_theme)
+
+ upload =
+ UploadCreator.new(file_from_contents(contents, fname), fname, for_theme: true).create_for(
+ -1,
+ )
+
+ child_theme.set_field(
+ target: :common,
+ name: SvgSprite.theme_sprite_variable_name,
+ upload_id: upload.id,
+ type: :theme_upload_var,
+ )
+ child_theme.save!
+
+ expect(Upload.exists?(id: upload.id)).to eq(true)
+ expect(SvgSprite.bundle(theme.id)).not_to match(/customthemeicon/)
+ end
end
end
diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb
index 8e2260c903e..550de9a6b84 100644
--- a/spec/models/theme_field_spec.rb
+++ b/spec/models/theme_field_spec.rb
@@ -666,6 +666,30 @@ HTML
FileStore::LocalStore.any_instance.stubs(:path_for).returns(nil)
expect(theme_field.validate_svg_sprite_xml).to match("Error with icons-sprite")
end
+
+ it "raises an error when sprite is too big" do
+ fname = "theme-icon-sprite.svg"
+ symbols = ""
+
+ 1500.times do |i|
+ id = "icon-id-#{i}"
+ path =
+ "M#{rand(1..100)} 2.18-2.087.277-4.216.42-6.378.42s-4.291-.143-6.378-.42c-1.085-.144-1.872-1.086-1.872-2.18v-4.25m16.5 0a2.18 2.18 0 00.75-1.661V8.706c0-1.081-.768-2.015-1.837-2.175a48.114 48.114 0 00-3.413-.387m4.5 8.006c-.194.165-.42.295-.673.38A23.978 23.978 0 0112 15.75c-2.648 0-5.195-.429-7.577-1.22a2.016 2.016 0 01-.673-.38m0 0A2.18 2.18 0 013 12.489V8.706c0-1.081.768-2.015 1.837-2.175a48.111 48.111 0 013.413-.387m7.5 0V5.25A2.25 2.25 0 0013.5 3h-3a2.25 .008z"
+ symbols += "<symbol id='#{id}' viewBox='0 0 100 100'><path d='#{path}'/></symbol>\n"
+ end
+
+ contents =
+ "<?xml version='1.0' encoding='UTF-8'?><svg><symbol id='customthemeicon' viewBox='0 0 100 100'><path d='M0 0h1ssss00v100H0z'/></symbol>#{symbols}</svg>"
+
+ sprite =
+ UploadCreator.new(file_from_contents(contents, fname), fname, for_theme: true).create_for(
+ -1,
+ )
+
+ theme_field.update(upload: sprite)
+
+ expect(theme_field.validate_svg_sprite_xml).to match("Error with icons-sprite")
+ end
end
describe "local js assets" do
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index 7dc882cb25b..eb017855a5e 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -687,6 +687,12 @@ def file_from_fixtures(filename, directory = "images")
File.new(tmp_file_path)
end
+def file_from_contents(contents, filename, directory = "images")
+ tmp_file_path = File.join(concurrency_safe_tmp_dir, SecureRandom.hex << filename)
+ File.write(tmp_file_path, contents)
+ File.new(tmp_file_path)
+end
+
def plugin_from_fixtures(plugin_name)
tmp_plugins_dir = File.join(concurrency_safe_tmp_dir, "plugins")