From 8cade1e825e90a66f440e820992d43c6905f4b47 Mon Sep 17 00:00:00 2001 From: Daniel Waterworth Date: Thu, 22 Feb 2024 13:47:15 -0600 Subject: [PATCH] SECURITY: Prevent large staff actions causing DoS This commit operates at three levels of abstraction: 1. We want to prevent user history rows from being unbounded in size. This commit adds rails validations to limit the sizes of columns on user_histories, 2. However, we don't want to prevent certain actions from being completed if these columns are too long. In those cases, we truncate the values that are given and store the truncated versions, 3. For endpoints that perform staff actions, we can further control what is permitted by explicitly validating the params that are given before attempting the action, --- app/controllers/admin/users_controller.rb | 18 ++++++ app/controllers/export_csv_controller.rb | 14 +++- app/models/user_history.rb | 17 +++++ app/services/staff_action_logger.rb | 35 ++++++---- config/locales/server.en.yml | 1 + spec/requests/admin/users_controller_spec.rb | 24 +++++++ spec/requests/export_csv_controller_spec.rb | 10 +++ spec/services/staff_action_logger_spec.rb | 67 ++++++++++++++++++-- 8 files changed, 167 insertions(+), 19 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 779ac6ea54c..01ef6bb0728 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -107,6 +107,11 @@ class Admin::UsersController < Admin::StaffController def suspend guardian.ensure_can_suspend!(@user) + reason = params[:reason] + + if reason && (!reason.is_a?(String) || reason.size > 300) + raise Discourse::InvalidParameters.new(:reason) + end if @user.suspended? suspend_record = @user.suspend_record @@ -128,6 +133,10 @@ class Admin::UsersController < Admin::StaffController all_users = [@user] if Array === params[:other_user_ids] + if params[:other_user_ids].size > MAX_SIMILAR_USERS + raise Discourse::InvalidParameters.new(:other_user_ids) + end + all_users.concat(User.where(id: params[:other_user_ids]).to_a) all_users.uniq! end @@ -360,6 +369,11 @@ class Admin::UsersController < Admin::StaffController def silence guardian.ensure_can_silence_user! @user + reason = params[:reason] + + if reason && (!reason.is_a?(String) || reason.size > 300) + raise Discourse::InvalidParameters.new(:reason) + end if @user.silenced? silenced_record = @user.silenced_record @@ -379,6 +393,10 @@ class Admin::UsersController < Admin::StaffController all_users = [@user] if Array === params[:other_user_ids] + if params[:other_user_ids].size > MAX_SIMILAR_USERS + raise Discourse::InvalidParameters.new(:other_user_ids) + end + all_users.concat(User.where(id: params[:other_user_ids]).to_a) all_users.uniq! end diff --git a/app/controllers/export_csv_controller.rb b/app/controllers/export_csv_controller.rb index 833bd53b303..a053de20b30 100644 --- a/app/controllers/export_csv_controller.rb +++ b/app/controllers/export_csv_controller.rb @@ -5,18 +5,26 @@ class ExportCsvController < ApplicationController def export_entity guardian.ensure_can_export_entity!(export_params[:entity]) + entity = export_params[:entity] + raise Discourse::InvalidParameters.new(:entity) unless entity.is_a?(String) && entity.size < 100 - if export_params[:entity] == "user_archive" + (export_params[:args] || {}).each do |key, value| + unless value.is_a?(String) && value.size < 100 + raise Discourse::InvalidParameters.new("args.#{key}") + end + end + + if entity == "user_archive" Jobs.enqueue(:export_user_archive, user_id: current_user.id, args: export_params[:args]) else Jobs.enqueue( :export_csv_file, - entity: export_params[:entity], + entity: entity, user_id: current_user.id, args: export_params[:args], ) end - StaffActionLogger.new(current_user).log_entity_export(export_params[:entity]) + StaffActionLogger.new(current_user).log_entity_export(entity) render json: success_json rescue Discourse::InvalidAccess render_json_error I18n.t("csv_export.rate_limit_error") diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 796dcd48bcb..5b6c6f006cc 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -11,6 +11,23 @@ class UserHistory < ActiveRecord::Base belongs_to :topic belongs_to :category + # Each value in the context should be shorter than this + MAX_CONTEXT_LENGTH = 50_000 + + # We often store multiple values in details, particularly during post edits + # Let's allow space for 2 values + a little extra for padding + MAX_DETAILS_LENGTH = 110_000 + + MAX_JSON_LENGTH = 300_000 + + validates :details, length: { maximum: MAX_DETAILS_LENGTH } + validates :context, length: { maximum: MAX_CONTEXT_LENGTH } + validates :subject, length: { maximum: MAX_CONTEXT_LENGTH } + validates :ip_address, length: { maximum: MAX_CONTEXT_LENGTH } + validates :email, length: { maximum: MAX_CONTEXT_LENGTH } + validates :previous_value, length: { maximum: MAX_JSON_LENGTH } + validates :new_value, length: { maximum: MAX_JSON_LENGTH } + validates_presence_of :action scope :only_staff_actions, -> { where("action IN (?)", UserHistory.staff_action_ids) } diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index 64a6d283a35..039b3eddf22 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -38,7 +38,7 @@ class StaffActionLogger StaffActionLogger.base_attrs.each do |attr| attrs[attr] = details.delete(attr) if details.has_key?(attr) end - attrs[:details] = details.map { |r| "#{r[0]}: #{r[1]}" }.join("\n") + attrs[:details] = details.map { |r| "#{r[0]}: #{truncate(r[1].to_s)}" }.join("\n") attrs[:acting_user_id] = @admin.id attrs[:action] = UserHistory.actions[:custom_staff] attrs[:custom_type] = custom_type @@ -80,7 +80,7 @@ class StaffActionLogger "user: #{username} (#{name})", "topic: #{topic_title}", "post_number: #{deleted_post.post_number}", - "raw: #{deleted_post.raw}", + "raw: #{truncate(deleted_post.raw)}", ] UserHistory.create!( @@ -105,7 +105,7 @@ class StaffActionLogger ] if first_post = topic.ordered_posts.with_deleted.first - details << "raw: #{first_post.raw}" + details << "raw: #{truncate(first_post.raw)}" end UserHistory.create!( @@ -179,7 +179,7 @@ class StaffActionLogger params(opts).merge( action: UserHistory.actions[:post_edit], post_id: post.id, - details: "#{opts[:old_raw]}\n\n---\n\n#{post.raw}", + details: "#{truncate(opts[:old_raw])}\n\n---\n\n#{truncate(post.raw)}", ), ) end @@ -261,15 +261,12 @@ class StaffActionLogger raise Discourse::InvalidParameters.new(:new_theme) unless new_theme new_json = theme_json(new_theme) - old_json, new_json = strip_duplicates(old_json, new_json) UserHistory.create!( - params(opts).merge( + params(opts).merge(json_params(old_json, new_json)).merge( action: UserHistory.actions[:change_theme], subject: new_theme.name, - previous_value: old_json, - new_value: new_json, ), ) end @@ -277,10 +274,9 @@ class StaffActionLogger def log_theme_destroy(theme, opts = {}) raise Discourse::InvalidParameters.new(:theme) unless theme UserHistory.create!( - params(opts).merge( + params(opts).merge(json_params(theme_json(theme), nil)).merge( action: UserHistory.actions[:delete_theme], subject: theme.name, - previous_value: theme_json(theme), ), ) end @@ -804,7 +800,7 @@ class StaffActionLogger "rejected_at: #{rejected_at}", "user: #{username} (#{name})", "topic: #{topic_title}", - "raw: #{reviewable.payload["raw"]}", + "raw: #{truncate(reviewable.payload["raw"])}", ] UserHistory.create!( @@ -1017,6 +1013,15 @@ class StaffActionLogger private + def json_params(previous_value, new_value) + if (previous_value && previous_value.length > UserHistory::MAX_JSON_LENGTH) || + (new_value && new_value.length > UserHistory::MAX_JSON_LENGTH) + { context: I18n.t("staff_action_logs.json_too_long") } + else + { previous_value: previous_value, new_value: new_value } + end + end + def get_changes(changes) return unless changes @@ -1046,4 +1051,12 @@ class StaffActionLogger urls = section.sidebar_urls.map { |url| "#{url.name} - #{url.value}" } "links: #{urls.join(", ")}" end + + def truncate(s) + if s.size > UserHistory::MAX_CONTEXT_LENGTH + "#{s.slice(..UserHistory::MAX_CONTEXT_LENGTH)}..." + else + s + end + end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 434dbcd9d7a..3a815f4d86d 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -5320,6 +5320,7 @@ en: custom: "Notification from %{username} on %{site_title}" staff_action_logs: + json_too_long: "Values not logged because they exceed the column length limits" not_found: "not found" unknown: "unknown" user_merged: "%{username} was merged into this account" diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index cf8f68c48c0..8114fe9791e 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -334,6 +334,18 @@ RSpec.describe Admin::UsersController do end end + it "fails the request if the reason is too long" do + expect(user).not_to be_suspended + put "/admin/users/#{user.id}/suspend.json", + params: { + reason: "x" * 301, + suspend_until: 5.hours.from_now, + } + expect(response.status).to eq(400) + user.reload + expect(user).not_to be_suspended + end + it "requires suspend_until and reason" do expect(user).not_to be_suspended put "/admin/users/#{user.id}/suspend.json", params: {} @@ -1596,6 +1608,18 @@ RSpec.describe Admin::UsersController do expect(reg_user.reload).to be_silenced expect(other_user.reload).to be_silenced end + + it "fails the request if the reason is too long" do + expect(user).not_to be_silenced + put "/admin/users/#{user.id}/silence.json", + params: { + reason: "x" * 301, + silenced_till: 5.hours.from_now, + } + expect(response.status).to eq(400) + user.reload + expect(user).not_to be_suspended + end end context "when logged in as a moderator" do diff --git a/spec/requests/export_csv_controller_spec.rb b/spec/requests/export_csv_controller_spec.rb index 861d5274b1b..d55106cb60f 100644 --- a/spec/requests/export_csv_controller_spec.rb +++ b/spec/requests/export_csv_controller_spec.rb @@ -73,6 +73,16 @@ RSpec.describe ExportCsvController do expect(log_entry.acting_user_id).to eq(admin.id) expect(log_entry.subject).to eq("user_list") end + + it "fails requests where the entity is too long" do + post "/export_csv/export_entity.json", params: { entity: "x" * 200 } + expect(response.status).to eq(400) + end + + it "fails requests where the name arg is too long" do + post "/export_csv/export_entity.json", params: { entity: "foo", args: { name: "x" * 200 } } + expect(response.status).to eq(400) + end end end diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index 4a60944f802..a6a2d282500 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/spec/services/staff_action_logger_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true RSpec.describe StaffActionLogger do + let(:long_string) { "Na " * 100_000 + "Batman!" } fab!(:admin) let(:logger) { described_class.new(admin) } @@ -69,6 +70,13 @@ RSpec.describe StaffActionLogger do log_post_deletion }.to change { UserHistory.count }.by(1) end + + it "truncates overly long values" do + deleted_post.update!(raw: long_string, skip_validation: true) + expect { log_post_deletion }.to change { UserHistory.count }.by(1) + log = UserHistory.last + expect(log.details.size).to be_between(50_000, 110_000) + end end describe "log_topic_delete_recover" do @@ -90,6 +98,13 @@ RSpec.describe StaffActionLogger do it "creates a new UserHistory record" do expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1) end + + it "truncates overly long values" do + Fabricate(:post, topic: topic, skip_validation: true, raw: long_string) + expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1) + log = UserHistory.last + expect(log.details.size).to be_between(50_000, 110_000) + end end context "when recovering topic" do @@ -112,6 +127,13 @@ RSpec.describe StaffActionLogger do it "creates a new UserHistory record" do expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1) end + + it "truncates overly long values" do + Fabricate(:post, topic: topic, skip_validation: true, raw: long_string) + expect { log_topic_delete_recover }.to change { UserHistory.count }.by(1) + log = UserHistory.last + expect(log.details.size).to be_between(50_000, 110_000) + end end end @@ -187,14 +209,12 @@ RSpec.describe StaffActionLogger do end describe "log_theme_change" do + fab!(:theme) + it "raises an error when params are invalid" do expect { logger.log_theme_change(nil, nil) }.to raise_error(Discourse::InvalidParameters) end - let! :theme do - Fabricate(:theme) - end - it "logs new site customizations" do log_record = logger.log_theme_change(nil, theme) expect(log_record.subject).to eq(theme.name) @@ -226,15 +246,28 @@ RSpec.describe StaffActionLogger do ], ) end + + it "doesn't log values when the json is too large" do + old_json = ThemeSerializer.new(theme, root: false).to_json + + theme.set_field(target: :common, name: :scss, value: long_string) + + log_record = logger.log_theme_change(old_json, theme) + + expect(log_record.previous_value).not_to be_present + expect(log_record.new_value).not_to be_present + expect(log_record.context).to be_present + end end describe "log_theme_destroy" do + fab!(:theme) + it "raises an error when params are invalid" do expect { logger.log_theme_destroy(nil) }.to raise_error(Discourse::InvalidParameters) end it "creates a new UserHistory record" do - theme = Fabricate(:theme) theme.set_field(target: :common, name: :scss, value: "body{margin: 10px;}") log_record = logger.log_theme_destroy(theme) @@ -253,6 +286,15 @@ RSpec.describe StaffActionLogger do ], ) end + + it "doesn't log values when the json is too large" do + theme.set_field(target: :common, name: :scss, value: long_string) + log_record = logger.log_theme_destroy(theme) + + expect(log_record.previous_value).not_to be_present + expect(log_record.new_value).not_to be_present + expect(log_record.context).to be_present + end end describe "log_theme_setting_change" do @@ -398,6 +440,12 @@ RSpec.describe StaffActionLogger do expect(logged.custom_type).to eq("clicked_something") expect(logged.topic_id).to be === 1234 end + + it "truncates overly long values" do + logged = logger.log_custom(:shower_thought, lyrics: long_string) + expect(logged).to be_valid + expect(logged.details.size).to be_between(50_000, 110_000) + end end describe "log_category_settings_change" do @@ -654,6 +702,15 @@ RSpec.describe StaffActionLogger do expect(user_history.action).to eq(UserHistory.actions[:post_rejected]) expect(user_history.details).to include(reviewable.payload["raw"]) end + + it "truncates overly long values" do + reviewable.payload["raw"] = long_string + reviewable.save! + + expect { log_post_rejected }.to change { UserHistory.count }.by(1) + log = UserHistory.last + expect(log.details.size).to be_between(50_000, 110_000) + end end describe "log_topic_closed" do