From 6fc84bd346ca5a20c2acf04da7aafcf5948aad11 Mon Sep 17 00:00:00 2001 From: Grant Cox Date: Tue, 8 Jul 2025 09:21:55 +1000 Subject: [PATCH 1/2] Fix race condition vulnerability, by ensuring the unconfirmed_email is always saved even if the submitted value is the same as the in-memory model's state --- lib/devise/models/confirmable.rb | 1 + test/integration/confirmable_test.rb | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index 6ce22c30f0..c920e7e323 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -262,6 +262,7 @@ def generate_confirmation_token! def postpone_email_change_until_confirmation_and_regenerate_confirmation_token @reconfirmation_required = true self.unconfirmed_email = self.email + unconfirmed_email_will_change! self.email = self.devise_email_in_database self.confirmation_token = nil generate_confirmation_token diff --git a/test/integration/confirmable_test.rb b/test/integration/confirmable_test.rb index 8e6f68ef2d..0f2c1a83a9 100644 --- a/test/integration/confirmable_test.rb +++ b/test/integration/confirmable_test.rb @@ -354,4 +354,29 @@ def visit_admin_confirmation_with_token(confirmation_token) assert_contain(/Email.*already.*taken/) assert admin.reload.pending_reconfirmation? end + + 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 From f12a6c3fe7c1dec2868a9499c9203573c8f6dd9e Mon Sep 17 00:00:00 2001 From: Grant Cox Date: Fri, 8 Aug 2025 04:23:58 +1000 Subject: [PATCH 2/2] Improve the Confirmable race condition test Co-authored-by: Rune Philosof <57357936+runephilosof-abtion@users.noreply.github.com> --- test/integration/confirmable_test.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/integration/confirmable_test.rb b/test/integration/confirmable_test.rb index 0f2c1a83a9..a909dc6449 100644 --- a/test/integration/confirmable_test.rb +++ b/test/integration/confirmable_test.rb @@ -355,22 +355,26 @@ def visit_admin_confirmation_with_token(confirmation_token) assert admin.reload.pending_reconfirmation? end - test 'concurrent "update email" requests should not allow confirmation_token and unconfirmed_email to get out of sync' do + test 'concurrent "update email" requests should not allow confirming a victim email address' 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) + attacker.update!(email: attacker_email) + + # A new request starts, to update the unconfirmed email again. + attacker = Admin.find_by(id: attacker.id) - # a concurrent request also updates the email address to the victim, while this request's model is in memory + # A concurrent request also updates the email address to the victim, while the `attacker` 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) + # Now the attacker updates to the same prior unconfirmed email address, and confirm. + # This should update the `unconfirmed_email` in the database, even though it is unchanged from the models point of view. + attacker.update!(email: attacker_email) attacker_token = attacker.raw_confirmation_token visit_admin_confirmation_with_token(attacker_token)