From 5b5866443941667175e32bfcd37aaac7a8934b4e Mon Sep 17 00:00:00 2001 From: Dion Hulse Date: Wed, 1 Jul 2015 06:32:07 +0000 Subject: [PATCH] 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 | 34 ++++++++-- src/wp-login.php | 11 ++-- tests/phpunit/tests/auth.php | 124 +++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/wp-includes/user.php b/src/wp-includes/user.php index ba9fa819fd..a1c12cdb01 100644 --- a/src/wp-includes/user.php +++ b/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. diff --git a/src/wp-login.php b/src/wp-login.php index 2056852726..080322cc09 100644 --- a/src/wp-login.php +++ b/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'] ) - $errors->add( 'invalidkey', __( 'Sorry, that key does not appear to be valid.' ) ); - elseif ( 'expiredkey' == $_GET['error'] ) - $errors->add( 'expiredkey', __( 'Sorry, that key has expired. Please try again.' ) ); + if ( 'invalidkey' == $_GET['error'] ) { + $errors->add( 'invalidkey', __( 'Your password reset link appears to be invalid. Please request a new lnk below.' ) ); + } elseif ( 'expiredkey' == $_GET['error'] ) { + $errors->add( 'expiredkey', __( 'Your password reset link has expired. Please request a new link below.' ) ); + } } $lostpassword_redirect = ! empty( $_REQUEST['redirect_to'] ) ? $_REQUEST['redirect_to'] : ''; diff --git a/tests/phpunit/tests/auth.php b/tests/phpunit/tests/auth.php index 120d8fcbe1..90141883f4 100644 --- a/tests/phpunit/tests/auth.php +++ b/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 ); + } }