CVE-2021-37693: Weak Password Recovery Mechanism for Forgotten Password (CWE-640)
value: Discourse is an open-source platform for community discussion. In Discourse before versions 2.7.8 and 2.8.0.beta4, when adding additional email addresses to an existing account on a Discourse site an email token is generated as part of the email verification process. Deleting the additional email address does not invalidate an unused token which can then be used in other contexts, including reseting a password.
CWE description: The product contains a mechanism for users to recover or change their passwords without knowing the original password, but the mechanism is weak.
CVSS v3 base score: 7.5
Published: 2021-08-13T16:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/models/email_change_request.rb, spec/requests/users_controller_spec.rb
Commit date: 2021-08-04 19:14:56 -0400
Commit message: SECURITY: Destroy `EmailToken` when `EmailChangeRequest` is destroyed (#13950)


Diff: diff --git a/app/models/email_change_request.rb b/app/models/email_change_request.rb
index 60b0d04183..74bb307bea 100644
--- a/app/models/email_change_request.rb
+++ b/app/models/email_change_request.rb
@@ -1,10 +1,10 @@
 # frozen_string_literal: true
 
 class EmailChangeRequest < ActiveRecord::Base
-  belongs_to :old_email_token, class_name: 'EmailToken'
-  belongs_to :new_email_token, class_name: 'EmailToken'
+  belongs_to :old_email_token, class_name: 'EmailToken', dependent: :destroy
+  belongs_to :new_email_token, class_name: 'EmailToken', dependent: :destroy
   belongs_to :user
   belongs_to :requested_by, class_name: "User", foreign_key: :requested_by_user_id
 
   validates :new_email, presence: true, format: { with: EmailValidator.email_regex }
 
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index c2cb1d3f67..3f204b0f2f 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -3048,10 +3048,23 @@ describe UsersController do
       delete "/u/#{user.username}/preferences/email.json", params: { email: other_email.email }
 
       expect(user.user_emails.pluck(:email)).to contain_exactly(user_email.email, other_email.email)
       expect(user.email_change_requests).to contain_exactly(request_1)
     end
+
+    it "can destroy associated email tokens" do
+      new_email = 'new.n.cool@example.com'
+      updater = EmailUpdater.new(guardian: user.guardian, user: user)
+
+      expect { updater.change_to(new_email) }
+        .to change { user.email_tokens.count }.by(1)
+
+      expect { delete "/u/#{user.username}/preferences/email.json", params: { email: new_email } }
+        .to change { user.email_tokens.count }.by(-1)
+
+      expect(user.email_tokens.first.email).to eq(user.email)
+    end
   end
 
   describe '#is_local_username' do
     fab!(:user) { Fabricate(:user) }
     fab!(:group) { Fabricate(:group, name: "Discourse", mentionable_level: Group::ALIAS_LEVELS[:everyone]) }

Vulnerable code:   belongs_to :old_email_token, class_name: 'EmailToken'
  belongs_to :new_email_token, class_name: 'EmailToken'
Fixed code:   belongs_to :old_email_token, class_name: 'EmailToken', dependent: :destroy
  belongs_to :new_email_token, class_name: 'EmailToken', dependent: :destroy

    it "can destroy associated email tokens" do
      new_email = 'new.n.cool@example.com'
      updater = EmailUpdater.new(guardian: user.guardian, user: user)

      expect { updater.change_to(new_email) }
        .to change { user.email_tokens.count }.by(1)

      expect { delete "/u/#{user.username}/preferences/email.json", params: { email: new_email } }
        .to change { user.email_tokens.count }.by(-1)

      expect(user.email_tokens.first.email).to eq(user.email)
    end