Skip to content
Permalink
Browse files

SECURITY: SQL injection with default categories

This is a low severity security fix because it requires a logged in
admin user to update a site setting via the API directly to an invalid
value.

The fix adds validation for the affected site settings, as well as a
secondary fix to prevent injection in the event of bad data somehow
already exists.
  • Loading branch information...
eviltrout committed Jul 11, 2019
1 parent afe922c commit 1d380405792adee7b9406b3bd6c3498cf30b5dca
@@ -1375,8 +1375,9 @@ def set_default_categories_preferences
values = []

%w{watching watching_first_post tracking muted}.each do |s|
category_ids = SiteSetting.get("default_categories_#{s}").split("|")
category_ids = SiteSetting.get("default_categories_#{s}").split("|").map(&:to_i)
category_ids.each do |category_id|
next if category_id == 0
values << "(#{self.id}, #{category_id}, #{CategoryUser.notification_levels[s.to_sym]})"
end
end
@@ -191,6 +191,7 @@ en:
embed:
load_from_remote: "There was an error loading that post."
site_settings:
invalid_category_id: "You specified a category that does not exist"
invalid_choice:
one: "You specified the invalid choice %{name}"
other: "You specified the invalid choices %{name}"
@@ -7,48 +7,62 @@ def validate_error(key, opts = {})
raise Discourse::InvalidParameters.new(I18n.t("errors.site_settings.#{key}", opts))
end

def validate_default_categories(new_val, default_categories_selected)
validate_error :default_categories_already_selected if (new_val.split("|").to_set & default_categories_selected).size > 0
def validate_category_ids(category_ids)
category_ids = category_ids.split('|').map(&:to_i).to_set
validate_error :invalid_category_id if Category.where(id: category_ids).count != category_ids.size
category_ids
end

def validate_default_categories(category_ids, default_categories_selected)
validate_error :default_categories_already_selected if (category_ids & default_categories_selected).size > 0
end

def validate_default_categories_watching(new_val)
category_ids = validate_category_ids(new_val)

default_categories_selected = [
SiteSetting.default_categories_tracking.split("|"),
SiteSetting.default_categories_muted.split("|"),
SiteSetting.default_categories_watching_first_post.split("|")
].flatten.to_set

validate_default_categories(new_val, default_categories_selected)
validate_default_categories(category_ids, default_categories_selected)
end

def validate_default_categories_tracking(new_val)
category_ids = validate_category_ids(new_val)

default_categories_selected = [
SiteSetting.default_categories_watching.split("|"),
SiteSetting.default_categories_muted.split("|"),
SiteSetting.default_categories_watching_first_post.split("|")
].flatten.to_set

validate_default_categories(new_val, default_categories_selected)
validate_default_categories(category_ids, default_categories_selected)
end

def validate_default_categories_muted(new_val)
category_ids = validate_category_ids(new_val)

default_categories_selected = [
SiteSetting.default_categories_watching.split("|"),
SiteSetting.default_categories_tracking.split("|"),
SiteSetting.default_categories_watching_first_post.split("|")
].flatten.to_set

validate_default_categories(new_val, default_categories_selected)
validate_default_categories(category_ids, default_categories_selected)
end

def validate_default_categories_watching_first_post(new_val)
category_ids = validate_category_ids(new_val)

default_categories_selected = [
SiteSetting.default_categories_watching.split("|"),
SiteSetting.default_categories_tracking.split("|"),
SiteSetting.default_categories_muted.split("|")
].flatten.to_set

validate_default_categories(new_val, default_categories_selected)
validate_default_categories(category_ids, default_categories_selected)
end

def validate_enable_s3_uploads(new_val)
@@ -6,6 +6,24 @@
describe SiteSettings::Validations do
subject { Class.new.include(described_class).new }

context "default_categories" do
fab!(:category) { Fabricate(:category) }

it "supports valid categories" do
expect { subject.validate_default_categories_watching("#{category.id}") }.not_to raise_error
end

it "won't allow you to input junk categories" do
expect {
subject.validate_default_categories_watching("junk")
}.to raise_error(Discourse::InvalidParameters)

expect {
subject.validate_default_categories_watching("#{category.id}|12312323")
}.to raise_error(Discourse::InvalidParameters)
end
end

context "s3 buckets reusage" do
let(:error_message) { I18n.t("errors.site_settings.s3_bucket_reused") }

@@ -1581,6 +1581,11 @@ def hash(password, salt)

context "when user preferences are overriden" do

fab!(:category0) { Fabricate(:category) }
fab!(:category1) { Fabricate(:category) }
fab!(:category2) { Fabricate(:category) }
fab!(:category3) { Fabricate(:category) }

before do
SiteSetting.default_email_digest_frequency = 1440 # daily
SiteSetting.default_email_level = UserOption.email_level_types[:never]
@@ -1596,10 +1601,10 @@ def hash(password, salt)

SiteSetting.default_topics_automatic_unpin = false

SiteSetting.default_categories_watching = "1"
SiteSetting.default_categories_tracking = "2"
SiteSetting.default_categories_muted = "3"
SiteSetting.default_categories_watching_first_post = "4"
SiteSetting.default_categories_watching = category0.id.to_s
SiteSetting.default_categories_tracking = category1.id.to_s
SiteSetting.default_categories_muted = category2.id.to_s
SiteSetting.default_categories_watching_first_post = category3.id.to_s
end

it "has overriden preferences" do
@@ -1617,10 +1622,10 @@ def hash(password, salt)
expect(options.auto_track_topics_after_msecs).to eq(0)
expect(options.notification_level_when_replying).to eq(3)

expect(CategoryUser.lookup(user, :watching).pluck(:category_id)).to eq([1])
expect(CategoryUser.lookup(user, :tracking).pluck(:category_id)).to eq([2])
expect(CategoryUser.lookup(user, :muted).pluck(:category_id)).to eq([3])
expect(CategoryUser.lookup(user, :watching_first_post).pluck(:category_id)).to eq([4])
expect(CategoryUser.lookup(user, :watching).pluck(:category_id)).to eq([category0.id])
expect(CategoryUser.lookup(user, :tracking).pluck(:category_id)).to eq([category1.id])
expect(CategoryUser.lookup(user, :muted).pluck(:category_id)).to eq([category2.id])
expect(CategoryUser.lookup(user, :watching_first_post).pluck(:category_id)).to eq([category3.id])
end

it "does not set category preferences for staged users" do

0 comments on commit 1d38040

Please sign in to comment.
You can’t perform that action at this time.