CVE-2022-46177: Insufficient Session Expiration (CWE-613)
value: Discourse is an option source discussion platform. Prior to version 2.8.14 on the `stable` branch and version 3.0.0.beta16 on the `beta` and `tests-passed` branches, when a user requests for a password reset link email, then changes their primary email, the old reset email is still valid. When the old reset email is used to reset the password, the Discourse account's primary email would be re-linked to the old email. If the old email address is compromised or has transferred ownership, this leads to an account takeover. This is however mitigated by the SiteSetting `email_token_valid_hours` which is currently 48 hours. Users should upgrade to versions 2.8.14 or 3.0.0.beta15 to receive a patch. As a workaround, lower `email_token_valid_hours ` as needed.
CWE description: According to WASC, "Insufficient Session Expiration is when a web site permits an attacker to reuse old session credentials or session IDs for authorization."
CVSS v3 base score: 8.1
Published: 2023-01-05T20:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/models/user_email.rb, spec/requests/users_controller_spec.rb, spec/requests/users_email_controller_spec.rb
Commit date: 2023-01-05 06:08:55 +0800
Commit message: SECURITY: Delete email tokens when a user's email is changed or deleted (#19735)

Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>
Diff: diff --git a/app/models/user_email.rb b/app/models/user_email.rb
index 4bb2e64e9abc..50ee27318b24 100644
--- a/app/models/user_email.rb
+++ b/app/models/user_email.rb
@@ -16,10 +16,14 @@ class UserEmail < ActiveRecord::Base
   validate :user_id_not_changed, if: :primary
   validate :unique_email, if: :validate_unique_email?
 
   scope :secondary, -> { where(primary: false) }
 
+  before_save ->() { destroy_email_tokens(self.email_was) }, if: :will_save_change_to_email?
+
+  after_destroy { destroy_email_tokens(self.email) }
+
   def normalize_email
     self.normalized_email = if self.email.present?
       username, domain = self.email.split('@', 2)
       username = username.gsub('.', '').gsub(/\+.*/, '')
       "#{username}@#{domain}"
@@ -60,10 +64,14 @@ class UserEmail < ActiveRecord::Base
       self.errors.add(:user_id, I18n.t(
         'active_record.errors.model.user_email.attributes.user_id.reassigning_primary_email')
       )
     end
   end
+
+  def destroy_email_tokens(email)
+    EmailToken.where(email: email).destroy_all
+  end
 end
 
 # == Schema Information
 #
 # Table name: user_emails
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 238aa6bed26b..3ee6bc1fe892 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -3654,21 +3654,22 @@ RSpec.describe UsersController do
 
       expect(user1.user_emails.pluck(:email)).to contain_exactly(user_email.email, other_email.email)
       expect(user1.email_change_requests).to contain_exactly(request_1)
     end
 
-    it "can destroy associated email tokens" do
+    it "destroys associated email tokens and email change requests" do
       new_email = 'new.n.cool@example.com'
       updater = EmailUpdater.new(guardian: user1.guardian, user: user1)
+      updater.change_to(new_email)
 
-      expect { updater.change_to(new_email) }
-        .to change { user1.email_tokens.count }.by(1)
+      email_token = updater.change_req.new_email_token
+      expect(email_token).to be_present
 
-      expect { delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email } }
-        .to change { user1.email_tokens.count }.by(-1)
+      delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email }
 
-      expect(user1.email_tokens.first.email).to eq(user1.email)
+      expect(EmailToken.find_by(id: email_token.id)).to eq(nil)
+      expect(EmailChangeRequest.find_by(id: updater.change_req.id)).to eq(nil)
     end
   end
 
   describe '#topic_tracking_state' do
     context 'when anon' do
@@ -3920,12 +3921,11 @@ RSpec.describe UsersController do
 
         user.reload
         expect(user.email).to eq('updatedemail@example.com')
         expect(user.email_tokens.where(email: 'updatedemail@example.com', expired: false)).to be_present
 
-        token.reload
-        expect(token.expired?).to eq(true)
+        expect(EmailToken.find_by(id: token.id)).to eq(nil)
       end
 
       it 'tells the user to slow down after many requests' do
         RateLimiter.enable
         RateLimiter.clear_all!
@@ -4011,12 +4011,11 @@ RSpec.describe UsersController do
 
         user.reload
         expect(user.email).to eq('updatedemail@example.com')
         expect(user.email_tokens.where(email: 'updatedemail@example.com', expired: false)).to be_present
 
-        token.reload
-        expect(token.expired?).to eq(true)
+        expect(EmailToken.find_by(id: token.id)).to eq(nil)
       end
 
       it 'tells the user to slow down after many requests' do
         RateLimiter.enable
         RateLimiter.clear_all!
diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb
index 655d302e177a..7c65d18f5a87 100644
--- a/spec/requests/users_email_controller_spec.rb
+++ b/spec/requests/users_email_controller_spec.rb
@@ -240,10 +240,32 @@ RSpec.describe UsersEmailController do
             expect(response.status).to eq(400)
           end
         end
       end
     end
+
+    it "destroys email tokens associated with the old email after the new email is confirmed" do
+      SiteSetting.enable_secondary_emails = true
+
+      email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset])
+
+      updater = EmailUpdater.new(guardian: user.guardian, user: user)
+      updater.change_to('bubblegum@adventuretime.ooo')
+
+      sign_in(user)
+      put "/u/confirm-new-email", params: {
+        token: "#{updater.change_req.new_email_token.token}"
+      }
+
+      new_password = SecureRandom.hex
+      put "/u/password-reset/#{email_token.token}.json", params: {
+        password: new_password
+      }
+      expect(response.parsed_body["success"]).to eq(false)
+      expect(response.parsed_body["message"]).to eq(I18n.t("password_reset.no_token", base_url: Discourse.base_url))
+      expect(user.reload.confirm_password?(new_password)).to eq(false)
+    end
   end
 
   describe '#confirm-old-email' do
     it 'redirects to login for signed out accounts' do
       get "/u/confirm-old-email/invalidtoken"

Vulnerable code:     it "can destroy associated email tokens" do
      expect { updater.change_to(new_email) }
        .to change { user1.email_tokens.count }.by(1)
      expect { delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email } }
        .to change { user1.email_tokens.count }.by(-1)
      expect(user1.email_tokens.first.email).to eq(user1.email)
        token.reload
        expect(token.expired?).to eq(true)
        token.reload
        expect(token.expired?).to eq(true)
Fixed code:   before_save ->() { destroy_email_tokens(self.email_was) }, if: :will_save_change_to_email?

  after_destroy { destroy_email_tokens(self.email) }


  def destroy_email_tokens(email)
    EmailToken.where(email: email).destroy_all
  end
    it "destroys associated email tokens and email change requests" do
      updater.change_to(new_email)
      email_token = updater.change_req.new_email_token
      expect(email_token).to be_present
      delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email }
      expect(EmailToken.find_by(id: email_token.id)).to eq(nil)
      expect(EmailChangeRequest.find_by(id: updater.change_req.id)).to eq(nil)
        expect(EmailToken.find_by(id: token.id)).to eq(nil)
        expect(EmailToken.find_by(id: token.id)).to eq(nil)

    it "destroys email tokens associated with the old email after the new email is confirmed" do
      SiteSetting.enable_secondary_emails = true

      email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset])

      updater = EmailUpdater.new(guardian: user.guardian, user: user)
      updater.change_to('bubblegum@adventuretime.ooo')

      sign_in(user)
      put "/u/confirm-new-email", params: {
        token: "#{updater.change_req.new_email_token.token}"
      }

      new_password = SecureRandom.hex
      put "/u/password-reset/#{email_token.token}.json", params: {
        password: new_password
      }
      expect(response.parsed_body["success"]).to eq(false)
      expect(response.parsed_body["message"]).to eq(I18n.t("password_reset.no_token", base_url: Discourse.base_url))
      expect(user.reload.confirm_password?(new_password)).to eq(false)
    end