CVE-2023-28107: Allocation of Resources Without Limits or Throttling (CWE-770)
value: Discourse is an open-source discussion platform. Prior to version 3.0.2 of the `stable` branch and version 3.1.0.beta3 of the `beta` and `tests-passed` branches, a user logged as an administrator can request backups multiple times, which will eat up all the connections to the DB. If this is done on a site using multisite, then it can affect the whole cluster. The vulnerability is patched in version 3.0.2 of the `stable` branch and version 3.1.0.beta3 of the `beta` and `tests-passed` branches. There are no known workarounds.
CWE description: The product allocates a reusable resource or group of resources on behalf of an actor without imposing any restrictions on the size or number of resources that can be allocated, in violation of the intended security policy for that actor.
CVSS v3 base score: 4.9
Published: 2023-03-17T17:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/controllers/admin/backups_controller.rb, lib/backup_restore/backuper.rb, spec/lib/backup_restore/backuper_spec.rb, spec/requests/admin/backups_controller_spec.rb
Commit date: 2023-03-16 16:09:22 +0100
Commit message: SECURITY: Rate limit the creation of backups

Diff: diff --git a/app/controllers/admin/backups_controller.rb b/app/controllers/admin/backups_controller.rb
index 99bc0ca2d350..7a86b2ef457d 100644
--- a/app/controllers/admin/backups_controller.rb
+++ b/app/controllers/admin/backups_controller.rb
@@ -32,10 +32,18 @@ class Admin::BackupsController < Admin::AdminController
   def status
     render_json_dump(BackupRestore.operations_status)
   end
 
   def create
+    RateLimiter.new(
+      current_user,
+      "max-backups-per-minute",
+      1,
+      1.minute,
+      apply_limit_to_staff: true,
+    ).performed!
+
     opts = {
       publish_to_message_bus: true,
       with_uploads: params.fetch(:with_uploads) == "true",
       client_id: params[:client_id],
     }
diff --git a/lib/backup_restore/backuper.rb b/lib/backup_restore/backuper.rb
index ef8d019886df..a0b6c923afaa 100644
--- a/lib/backup_restore/backuper.rb
+++ b/lib/backup_restore/backuper.rb
@@ -3,11 +3,11 @@
 require "mini_mime"
 require "file_store/s3_store"
 
 module BackupRestore
   class Backuper
-    attr_reader :success
+    attr_reader :success, :store
 
     def initialize(user_id, opts = {})
       @user_id = user_id
       @client_id = opts[:client_id]
       @publish_to_message_bus = opts[:publish_to_message_bus] || false
@@ -44,20 +44,19 @@ module BackupRestore
     rescue SystemExit
       log "Backup process was cancelled!"
     rescue Exception => ex
       log "EXCEPTION: " + ex.message
       log ex.backtrace.join("\n")
-      @success = false
     else
       @success = true
       @backup_filename
     ensure
       delete_old
       clean_up
       notify_user
       log "Finished!"
-      publish_completion(@success)
+      publish_completion
     end
 
     protected
 
     def ensure_no_operation_is_running
@@ -335,16 +334,16 @@ module BackupRestore
 
       log "No uploads found on S3. Skipping archiving of uploads stored on S3..." if count == 0
     end
 
     def upload_archive
-      return unless @store.remote?
+      return unless store.remote?
 
       log "Uploading archive..."
       content_type = MiniMime.lookup_by_filename(@backup_filename).content_type
       archive_path = File.join(@archive_directory, @backup_filename)
-      @store.upload_file(@backup_filename, archive_path, content_type)
+      store.upload_file(@backup_filename, archive_path, content_type)
     end
 
     def after_create_hook
       log "Executing the after_create_hook for the backup..."
       DiscourseEvent.trigger(:backup_created)
@@ -352,20 +351,20 @@ module BackupRestore
 
     def delete_old
       return if Rails.env.development?
 
       log "Deleting old backups..."
-      @store.delete_old
+      store.delete_old
     rescue => ex
       log "Something went wrong while deleting old backups.", ex
     end
 
     def notify_user
-      return if @success && @user.id == Discourse::SYSTEM_USER_ID
+      return if success && @user.id == Discourse::SYSTEM_USER_ID
 
       log "Notifying '#{@user.username}' of the end of the backup..."
-      status = @success ? :backup_succeeded : :backup_failed
+      status = success ? :backup_succeeded : :backup_failed
 
       logs = Discourse::Utils.logs_markdown(@logs, user: @user)
       post = SystemMessage.create_from_system_user(@user, status, logs: logs)
 
       post.topic.invite_group(@user, Group[:admins]) if @user.id == Discourse::SYSTEM_USER_ID
@@ -376,15 +375,15 @@ module BackupRestore
     def clean_up
       log "Cleaning stuff up..."
       delete_uploaded_archive
       remove_tar_leftovers
       mark_backup_as_not_running
-      refresh_disk_space
+      refresh_disk_space if success
     end
 
     def delete_uploaded_archive
-      return unless @store.remote?
+      return unless store.remote?
 
       archive_path = File.join(@archive_directory, @backup_filename)
 
       if File.exist?(archive_path)
         log "Removing archive from local storage..."
@@ -394,11 +393,11 @@ module BackupRestore
       log "Something went wrong while deleting uploaded archive from local storage.", ex
     end
 
     def refresh_disk_space
       log "Refreshing disk stats..."
-      @store.reset_cache
+      store.reset_cache
     rescue => ex
       log "Something went wrong while refreshing disk stats.", ex
     end
 
     def remove_tar_leftovers
@@ -448,11 +447,11 @@ module BackupRestore
 
     def save_log(message, timestamp)
       @logs << "[#{timestamp}] #{message}"
     end
 
-    def publish_completion(success)
+    def publish_completion
       if success
         log("[SUCCESS]")
         DiscourseEvent.trigger(:backup_complete, logs: @logs, ticket: @ticket)
       else
         log("[FAILED]")
diff --git a/spec/lib/backup_restore/backuper_spec.rb b/spec/lib/backup_restore/backuper_spec.rb
index 1578a44f446d..258eedfec42d 100644
--- a/spec/lib/backup_restore/backuper_spec.rb
+++ b/spec/lib/backup_restore/backuper_spec.rb
@@ -1,20 +1,22 @@
 # frozen_string_literal: true
 
 RSpec.describe BackupRestore::Backuper do
-  it "returns a non-empty parameterized title when site title contains unicode" do
-    SiteSetting.title = "Ɣ"
-    backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
+  describe "#get_parameterized_title" do
+    it "returns a non-empty parameterized title when site title contains unicode" do
+      SiteSetting.title = "Ɣ"
+      backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
 
-    expect(backuper.send(:get_parameterized_title)).to eq("discourse")
-  end
+      expect(backuper.send(:get_parameterized_title)).to eq("discourse")
+    end
 
-  it "returns a valid parameterized site title" do
-    SiteSetting.title = "Coding Horror"
-    backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
+    it "returns a valid parameterized site title" do
+      SiteSetting.title = "Coding Horror"
+      backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
 
-    expect(backuper.send(:get_parameterized_title)).to eq("coding-horror")
+      expect(backuper.send(:get_parameterized_title)).to eq("coding-horror")
+    end
   end
 
   describe "#notify_user" do
     before { freeze_time Time.zone.parse("2010-01-01 12:00") }
 
@@ -67,6 +69,34 @@ RSpec.describe BackupRestore::Backuper do
       expect(Topic.last.first_post.raw).to include(
         "```text\n...\n[2010-01-01 12:00:00] Line 10\n[2010-01-01 12:00:00] Notifying 'system' of the end of the backup...\n```",
       )
     end
   end
+
+  describe "#run" do
+    subject(:run) { backup.run }
+
+    let(:backup) { described_class.new(user.id) }
+    let(:user) { Discourse.system_user }
+    let(:store) { backup.store }
+
+    before { backup.stubs(:success).returns(success) }
+
+    context "when the result isn't successful" do
+      let(:success) { false }
+
+      it "doesn't refresh disk stats" do
+        store.expects(:reset_cache).never
+        run
+      end
+    end
+
+    context "when the result is successful" do
+      let(:success) { true }
+
+      it "refreshes disk stats" do
+        store.expects(:reset_cache)
+        run
+      end
+    end
+  end
 end
diff --git a/spec/requests/admin/backups_controller_spec.rb b/spec/requests/admin/backups_controller_spec.rb
index b52278832c02..6bb5519c5f05 100644
--- a/spec/requests/admin/backups_controller_spec.rb
+++ b/spec/requests/admin/backups_controller_spec.rb
@@ -135,11 +135,14 @@ RSpec.describe Admin::BackupsController do
     end
   end
 
   describe "#create" do
     context "when logged in as an admin" do
-      before { sign_in(admin) }
+      before do
+        sign_in(admin)
+        BackupRestore.stubs(:backup!)
+      end
 
       it "starts a backup" do
         BackupRestore.expects(:backup!).with(
           admin.id,
           { publish_to_message_bus: true, with_uploads: false, client_id: "foo" },
@@ -147,10 +150,26 @@ RSpec.describe Admin::BackupsController do
 
         post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }
 
         expect(response.status).to eq(200)
       end
+
+      context "with rate limiting enabled" do
+        before do
+          RateLimiter.clear_all!
+          RateLimiter.enable
+        end
+
+        after { RateLimiter.disable }
+
+        it "is rate limited" do
+          post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }
+          post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }
+
+          expect(response).to have_http_status :too_many_requests
+        end
+      end
     end
 
     shared_examples "backups creation not allowed" do
       it "prevents backups creation with a 404 response" do
         post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }

Vulnerable code:     attr_reader :success
      @success = false
      publish_completion(@success)
      return unless @store.remote?
      @store.upload_file(@backup_filename, archive_path, content_type)
      @store.delete_old
      return if @success && @user.id == Discourse::SYSTEM_USER_ID
      status = @success ? :backup_succeeded : :backup_failed
      refresh_disk_space
      return unless @store.remote?
      @store.reset_cache
    def publish_completion(success)
  it "returns a non-empty parameterized title when site title contains unicode" do
    SiteSetting.title = "Ɣ"
    backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
    expect(backuper.send(:get_parameterized_title)).to eq("discourse")
  end
  it "returns a valid parameterized site title" do
    SiteSetting.title = "Coding Horror"
    backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
    expect(backuper.send(:get_parameterized_title)).to eq("coding-horror")
      before { sign_in(admin) }
Fixed code:     RateLimiter.new(
      current_user,
      "max-backups-per-minute",
      1,
      1.minute,
      apply_limit_to_staff: true,
    ).performed!

    attr_reader :success, :store
      publish_completion
      return unless store.remote?
      store.upload_file(@backup_filename, archive_path, content_type)
      store.delete_old
      return if success && @user.id == Discourse::SYSTEM_USER_ID
      status = success ? :backup_succeeded : :backup_failed
      refresh_disk_space if success
      return unless store.remote?
      store.reset_cache
    def publish_completion
  describe "#get_parameterized_title" do
    it "returns a non-empty parameterized title when site title contains unicode" do
      SiteSetting.title = "Ɣ"
      backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
      expect(backuper.send(:get_parameterized_title)).to eq("discourse")
    end
    it "returns a valid parameterized site title" do
      SiteSetting.title = "Coding Horror"
      backuper = BackupRestore::Backuper.new(Discourse.system_user.id)
      expect(backuper.send(:get_parameterized_title)).to eq("coding-horror")
    end

  describe "#run" do
    subject(:run) { backup.run }

    let(:backup) { described_class.new(user.id) }
    let(:user) { Discourse.system_user }
    let(:store) { backup.store }

    before { backup.stubs(:success).returns(success) }

    context "when the result isn't successful" do
      let(:success) { false }

      it "doesn't refresh disk stats" do
        store.expects(:reset_cache).never
        run
      end
    end

    context "when the result is successful" do
      let(:success) { true }

      it "refreshes disk stats" do
        store.expects(:reset_cache)
        run
      end
    end
  end
      before do
        sign_in(admin)
        BackupRestore.stubs(:backup!)
      end

      context "with rate limiting enabled" do
        before do
          RateLimiter.clear_all!
          RateLimiter.enable
        end

        after { RateLimiter.disable }

        it "is rate limited" do
          post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }
          post "/admin/backups.json", params: { with_uploads: false, client_id: "foo" }

          expect(response).to have_http_status :too_many_requests
        end
      end