Commit 0252777
Fix race condition vulnerability, by ensuring the
Fix 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:
1. Attacker registers `attacker1@email.com`
2. Attacker changes their email to `attacker2@email.com`, but does not yet confirm this
3. Attacker submits two concurrent "change email" requests
a. one changing to `attacker2@email.com`
b. one changing to `victim@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.
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>unconfirmed_email is always saved (#5784)1 parent 879f79f commit 0252777
File tree
4 files changed
+47
-1
lines changed- lib/devise
- models
- test/integration
4 files changed
+47
-1
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
1 | 6 | | |
2 | 7 | | |
3 | 8 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
258 | 258 | | |
259 | 259 | | |
260 | 260 | | |
261 | | - | |
262 | 261 | | |
263 | 262 | | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
264 | 266 | | |
265 | 267 | | |
266 | 268 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
38 | 42 | | |
39 | 43 | | |
40 | 44 | | |
| |||
61 | 65 | | |
62 | 66 | | |
63 | 67 | | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
64 | 75 | | |
65 | 76 | | |
66 | 77 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
354 | 354 | | |
355 | 355 | | |
356 | 356 | | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
357 | 385 | | |
0 commit comments