Expire password reset links after 24 hours (by default). This causes existing password reset links to become invalid.

Props markjaquith, voldemortensen, johnbillion, MikeHansenMe, dd32
See #32429


git-svn-id: https://develop.svn.wordpress.org/trunk@33019 602fd350-edb4-49c9-b593-d223f7449a82

src/wp-includes/user.php

@@ -2458,18 +2458,42 @@ function check_password_reset_key($key, $login) {
 		$wp_hasher = new PasswordHash( 8, true );
 	}
 
-	if ( $wp_hasher->CheckPassword( $key, $row->user_activation_key ) )
+	/**
-		return get_userdata( $row->ID );
+	 * Filter the expiration time of password reset keys.
+	 *
+	 * @since 4.3.0
+	 *
+	 * @param int $expiration The expiration time in seconds.
+	 */
+	$expiration_duration = apply_filters( 'password_reset_expiration', DAY_IN_SECONDS );
 
-	if ( $key === $row->user_activation_key ) {
+	if ( false !== strpos( $row->user_activation_key, ':' ) ) {
+		list( $pass_request_time, $pass_key ) = explode( ':', $row->user_activation_key, 2 );
+		$expiration_time = $pass_request_time + $expiration_duration;
+	} else {
+		$pass_key = $row->user_activation_key;
+		$expiration_time = false;
+	}
+
+	$hash_is_correct = $wp_hasher->CheckPassword( $key, $pass_key );
+
+	if ( $hash_is_correct && $expiration_time && time() < $expiration_time ) {
+		return get_userdata( $row->ID );
+	} elseif ( $hash_is_correct && $expiration_time ) {
+		// Key has an expiration time that's passed
+		return new WP_Error( 'expired_key', __( 'Invalid key' ) );
+	}
+
+	if ( hash_equals( $row->user_activation_key, $key ) || ( $hash_is_correct && ! $expiration_time ) ) {
 		$return = new WP_Error( 'expired_key', __( 'Invalid key' ) );
 		$user_id = $row->ID;
 
 		/**
 		 * Filter the return value of check_password_reset_key() when an
-		 * old-style key is used (plain-text key was stored in the database).
+		 * old-style key is used.
 		 *
-		 * @since 3.7.0
+		 * @since 3.7.0 Previously plain-text keys were stored in the database.
+		 * @since 4.3.0 Previously key hashes were stored without an expiration time.
 		 *
 		 * @param WP_Error $return  A WP_Error object denoting an expired key.
 		 *                          Return a WP_User object to validate the key.

src/wp-login.php

@@ -363,7 +363,7 @@ function retrieve_password() {
 		require_once ABSPATH . WPINC . '/class-phpass.php';
 		$wp_hasher = new PasswordHash( 8, true );
 	}
-	$hashed = $wp_hasher->HashPassword( $key );
+	$hashed = time() . ':' . $wp_hasher->HashPassword( $key );
 	$wpdb->update( $wpdb->users, array( 'user_activation_key' => $hashed ), array( 'user_login' => $user_login ) );
 
 	$message = __('Someone requested that the password be reset for the following account:') . "\r\n\r\n";
@@ -528,10 +528,11 @@ case 'retrievepassword' :
 	}
 
 	if ( isset( $_GET['error'] ) ) {
-		if ( 'invalidkey' == $_GET['error'] )
+		if ( 'invalidkey' == $_GET['error'] ) {
-			$errors->add( 'invalidkey', __( 'Sorry, that key does not appear to be valid.' ) );
+			$errors->add( 'invalidkey', __( 'Your password reset link appears to be invalid. Please request a new lnk below.' ) );
-		elseif ( 'expiredkey' == $_GET['error'] )
+		} elseif ( 'expiredkey' == $_GET['error'] ) {
-			$errors->add( 'expiredkey', __( 'Sorry, that key has expired. Please try again.' ) );
+			$errors->add( 'expiredkey', __( 'Your password reset link has expired. Please request a new link below.' ) );
+		}
 	}
 
 	$lostpassword_redirect = ! empty( $_REQUEST['redirect_to'] ) ? $_REQUEST['redirect_to'] : '';

tests/phpunit/tests/auth.php

@@ -6,10 +6,14 @@
  */
 class Tests_Auth extends WP_UnitTestCase {
 	var $user_id;
+	var $wp_hasher;
 
 	function setUp() {
 		parent::setUp();
 		$this->user_id = $this->factory->user->create();
+
+		require_once ABSPATH . WPINC . '/class-phpass.php';
+		$this->wp_hasher = new PasswordHash( 8, true );
 	}
 
 	function test_auth_cookie_valid() {
@@ -159,4 +163,124 @@ class Tests_Auth extends WP_UnitTestCase {
 		// Password broken by setting it to be too long.
 		$this->assertInstanceOf( 'WP_Error', $user );
 	}
+
+	/**
+	 * @ticket 32429
+	 */
+	function test_user_activation_key_is_checked() {
+		global $wpdb;
+
+		$key  = wp_generate_password( 20, false );
+		$user = $this->factory->user->create_and_get();
+		$wpdb->update( $wpdb->users, array(
+			'user_activation_key' => strtotime( '-1 hour' ) . ':' . $this->wp_hasher->HashPassword( $key ),
+		), array(
+			'ID' => $user->ID,
+		) );
+
+		// A valid key should be accepted
+		$check = check_password_reset_key( $key, $user->user_login );
+		$this->assertInstanceOf( 'WP_User', $check );
+		$this->assertSame( $user->ID, $check->ID );
+
+		// An invalid key should be rejected
+		$check = check_password_reset_key( 'key', $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+
+		// An empty key should be rejected
+		$check = check_password_reset_key( '', $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+
+		// A truncated key should be rejected
+		$partial = substr( $key, 0, 10 );
+		$check = check_password_reset_key( $partial, $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+	}
+
+	/**
+	 * @ticket 32429
+	 */
+	function test_expired_user_activation_key_is_rejected() {
+		global $wpdb;
+
+		$key  = wp_generate_password( 20, false );
+		$user = $this->factory->user->create_and_get();
+		$wpdb->update( $wpdb->users, array(
+			'user_activation_key' => strtotime( '-48 hours' ) . ':' . $this->wp_hasher->HashPassword( $key ),
+		), array(
+			'ID' => $user->ID,
+		) );
+
+		// An expired but otherwise valid key should be rejected
+		$check = check_password_reset_key( $key, $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+	}
+
+	/**
+	 * @ticket 32429
+	 */
+	function test_empty_user_activation_key_fails_key_check() {
+		global $wpdb;
+
+		$user = $this->factory->user->create_and_get();
+
+		// An empty user_activation_key should not allow any key to be accepted
+		$check = check_password_reset_key( 'key', $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+
+		// An empty user_activation_key should not allow an empty key to be accepted
+		$check = check_password_reset_key( '', $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+	}
+
+	/**
+	 * @ticket 32429
+	 */
+	function test_legacy_user_activation_key_is_rejected() {
+		global $wpdb;
+
+		// A legacy user_activation_key is one without the `time()` prefix introduced in WordPress 4.3.
+
+		$key  = wp_generate_password( 20, false );
+		$user = $this->factory->user->create_and_get();
+		$wpdb->update( $wpdb->users, array(
+			'user_activation_key' => $this->wp_hasher->HashPassword( $key ),
+		), array(
+			'ID' => $user->ID,
+		) );
+
+		// A legacy user_activation_key should not be accepted
+		$check = check_password_reset_key( $key, $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+
+		// An empty key with a legacy user_activation_key should be rejected
+		$check = check_password_reset_key( '', $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+	}
+
+	/**
+	 * @ticket 32429
+	 * @ticket 24783
+	 */
+	function test_plaintext_user_activation_key_is_rejected() {
+		global $wpdb;
+
+		// A plaintext user_activation_key is one stored before hashing was introduced in WordPress 3.7.
+
+		$key  = wp_generate_password( 20, false );
+		$user = $this->factory->user->create_and_get();
+		$wpdb->update( $wpdb->users, array(
+			'user_activation_key' => $key,
+		), array(
+			'ID' => $user->ID,
+		) );
+
+		// A plaintext user_activation_key should not allow an otherwise valid key to be accepted
+		$check = check_password_reset_key( $key, $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+
+		// A plaintext user_activation_key should not allow an empty key to be accepted
+		$check = check_password_reset_key( '', $user->user_login );
+		$this->assertInstanceOf( 'WP_Error', $check );
+	}
 }