Confirmable "change email" vulnerability - race condition permits user to confirm email address they have no access to #5783
Description
I have attempted to report this via email (to heartcombo@googlegroups.com) as requested in the README, but after 90 days of no response after several follow-ups, I feel it's better to be public than not at all.
Vulnerability details
We have found a security issue in the Confirmable "change email" flow, where a user can end up confirming an email address that they have no access to. The flow for this is:
- Attacker registers
attacker1@email.com - Attacker changes their email to
attacker2@email.com, but does not yet confirm this - Attacker submits two concurrent "change email" requests
a. one changing toattacker2@email.com
b. one changing tovictim@email.com
When request 3.a is run, the Confirmable.postpone_email_change_until_confirmation_and_regenerate_confirmation_token method sets both the unconfirmed_email and confirmation_token properties. But as the unconfirmed_email value is the same as the model already had from step 2, this attribute is not included in the SQL UPDATE statement. The SQL UPDATE statement only updates the confirmation_token. This token is emailed to the attacker2@email.com address.
If the "victim" race request (3.b) completes first, it will update both the unconfirmed_email and the confirmation_token. But then request 3.a will replace just the token. The model's end state is having the confirmation token that was sent to the attacker, but with the unconfirmed_email of the victim.
When the attacker follows the confirmation link, they will have confirmed the victim's email address, on an account that the attacker controls.
Demonstrative test case:
Here is a test case demonstrating this issue. A fix for this is to add unconfirmed_email_will_change! when setting the unconfirmed_email (https://github.com/heartcombo/devise/blob/v4.9.4/lib/devise/models/confirmable.rb#L264), to force this property to be included in the SQL UPDATE statement even if it "hasn't changed".
class ConfirmationOnChangeTest < Devise::IntegrationTest
test 'concurrent "update email" requests should not allow confirmation_token and unconfirmed_email to get out of sync' do
attacker_email = "attacker@example.com"
victim_email = "victim@example.com"
attacker = create_admin
# update the email address of the attacker, but do not confirm it yet
attacker.update(email: attacker_email)
# a concurrent request also updates the email address to the victim, while this request's model is in memory
Admin.where(id: attacker.id).update_all(
unconfirmed_email: victim_email,
confirmation_token: "different token"
)
# now we update to the same prior unconfirmed email address, and confirm
attacker.update(email: attacker_email)
attacker_token = attacker.raw_confirmation_token
visit_admin_confirmation_with_token(attacker_token)
attacker.reload
assert attacker.confirmed?
assert_equal attacker_email, attacker.email
end
end
Application fix
From an application perspective, adding these setters to the relevant model(s) works well:
class User < ApplicationRecord
# ensure that the confirmation token and unconfirmed email are always saved together, even if one value "hasn't changed"
def confirmation_token=(value)
write_attribute(:confirmation_token, value)
unconfirmed_email_will_change!
confirmation_token_will_change!
end
def unconfirmed_email=(value)
write_attribute(:unconfirmed_email, value)
unconfirmed_email_will_change!
confirmation_token_will_change!
end
end