Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hash the login nonce for storage to avoid leaking it via DB access #453

Merged
merged 17 commits into from
Sep 8, 2022
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions class-two-factor-core.php
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,17 @@ public static function login_url( $params = array(), $scheme = 'login' ) {
return add_query_arg( $params, site_url( 'wp-login.php', $scheme ) );
}

/**
* Get the hash of a nonce for storage and comparison.
*
* @param string $nonce Nonce value to be hashed.
*
* @return string
*/
protected static function hash_login_nonce( $nonce ) {
return wp_hash( $nonce, 'nonce' );
}

/**
* Create the login nonce.
*
Expand All @@ -737,15 +748,21 @@ public static function login_url( $params = array(), $scheme = 'login' ) {
* @return array
*/
public static function create_login_nonce( $user_id ) {
$login_nonce = array();
$login_nonce = array(
'expiration' => time() + HOUR_IN_SECONDS,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need a nonce that is valid for an hour? How about we reduce it to 10 minutes or something like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #473 👍🏻

);

try {
$login_nonce['key'] = bin2hex( random_bytes( 32 ) );
} catch ( Exception $ex ) {
$login_nonce['key'] = wp_hash( $user_id . wp_rand() . microtime(), 'nonce' );
}
$login_nonce['expiration'] = time() + HOUR_IN_SECONDS;

if ( ! update_user_meta( $user_id, self::USER_META_NONCE_KEY, $login_nonce ) ) {
// Store the nonce hashed to avoid leaking it via database access.
$login_nonce_stored = $login_nonce;
$login_nonce_stored['key'] = self::hash_login_nonce( $login_nonce['key'] );
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we hash the nonce for storage and pass the original to the form.


if ( ! update_user_meta( $user_id, self::USER_META_NONCE_KEY, $login_nonce_stored ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store the nonce with the key hashed.

return false;
}

Expand Down Expand Up @@ -779,12 +796,13 @@ public static function verify_login_nonce( $user_id, $nonce ) {
return false;
}

if ( $nonce !== $login_nonce['key'] || time() > $login_nonce['expiration'] ) {
self::delete_login_nonce( $user_id );
return false;
if ( hash_equals( self::hash_login_nonce( $nonce ), $login_nonce['key'] ) && time() < $login_nonce['expiration'] ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a positive check for readability.

return true;
}

return true;
self::delete_login_nonce( $user_id );

return false;
}

/**
Expand Down