Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SECURITY: Reduce maximum size of SVG sprite cache to prevent DoS 

Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>
This commit is contained in:
Daniel Waterworth 2023-09-01 12:22:58 -05:00 committed by Roman Rizzi
parent e3a2446874
commit 290306a932
No known key found for this signature in database
GPG Key ID: 64024A71CE7330D3
6 changed files with 137 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
app/models/theme_field.rb
View File

@ -204,7 +204,11 @@ class ThemeField < ActiveRecord::Base
begin
content = File.read(path)
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",
)

28
lib/svg_sprite.rb
View File

@ -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,9 +300,11 @@ module SvgSprite
def self.theme_svgs(theme_id)
if theme_id.present?
theme_ids = Theme.transform_ids(theme_id)
get_set_cache("theme_svg_sprites_#{theme_ids.join(",")}") do
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],
@ -308,8 +312,9 @@ module SvgSprite
theme_id: theme_ids,
).pluck(:upload_id)
theme_sprites = ThemeSvgSprite.where(theme_id: theme_ids).pluck(:upload_id, :sprite)
missing_sprites = (theme_field_uploads - theme_sprites.map(&:first))
theme_sprites =
ThemeSvgSprite.where(theme_id: theme_ids).pluck(:theme_id, :upload_id, :sprite)
missing_sprites = (theme_field_uploads - theme_sprites.map(&:second))
if missing_sprites.present?
Rails.logger.warn(
@ -317,18 +322,23 @@ module SvgSprite
)
end
theme_sprites.reduce({}) do |symbols, (upload_id, sprite)|
theme_sprites
.map do |(theme_id, upload_id, sprite)|
begin
symbols.merge!(symbols_for("theme_#{theme_id}_#{upload_id}.svg", sprite, strict: false))
[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
symbols
end
.compact
.to_h
.values_at(*theme_ids)
end
.values
.compact
.reduce({}) { |a, b| a.merge!(b) }
else
{}
end

9
spec/fixtures/images/custom-theme-component-icon-sprite.svg vendored Normal file
View File

@ -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>
Side by Side

After

Width:  |  Height:  |  Size: 1007 B

58
spec/lib/svg_sprite/svg_sprite_spec.rb
View File

@ -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

24
spec/models/theme_field_spec.rb
View File

@ -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

6
spec/rails_helper.rb
View File

@ -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")