heartcombo
`Confirmable` "change email" vulnerability - race condition permits user to confirm email address they have no access to
I have attempted to report this via email (to [email protected]) 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
[email protected] - Attacker changes their email to
[email protected], but does not yet confirm this - Attacker submits two concurrent "change email" requests
a. one changing to
[email protected]b. one changing to[email protected]
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 [email protected] 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 = "[email protected]"
victim_email = "[email protected]"
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
@grantcox Have you created a CVE for this? https://cveform.mitre.org/
@grantcox thanks for the report, I've seen your original email and meant to reply, but well took me so long, sorry about that. (Also I need to kill [email protected].)
I'll try to take a look at the report and reproduction in more detail.
@grantcox Have you created a CVE for this? https://cveform.mitre.org/
I had not, but I have just submitted that now, thank you for the reminder. I will update here when a CVE number is issued.
No, not yet, only the auto-reply that they'll investigate it. It is "CVE Request 1905238" for whatever that is worth. I haven't submitted a CVE before, but looking online it seems common to take weeks or months for them to be issued...
FWIW I sent a status request to [email protected] on Sept 6, a month after the initial submission, and have received no response to that either. 🤷