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.

app/models/user.rb

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

config/locales/server.en.yml

@@ -190,6 +190,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}"

lib/site_settings/validations.rb

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

spec/lib/site_settings/validations_spec.rb

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

spec/models/user_spec.rb

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