Hash the login nonce for storage to avoid leaking it via DB access #453
Conversation
class-two-factor-core.php
Outdated
| 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'] ) { |
There was a problem hiding this comment.
Use a positive check for readability.
| 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'] ); |
There was a problem hiding this comment.
Here we hash the nonce for storage and pass the original to the form.
| $login_nonce_stored = $login_nonce; | ||
| $login_nonce_stored['key'] = self::hash_login_nonce( $login_nonce['key'] ); | ||
|
|
||
| if ( ! update_user_meta( $user_id, self::USER_META_NONCE_KEY, $login_nonce_stored ) ) { |
There was a problem hiding this comment.
Store the nonce with the key hashed.
class-two-factor-core.php
Outdated
| if ( ! $login_nonce ) { | ||
|
|
||
| // Require a fresh nonce for each request. | ||
| self::delete_login_nonce( $user_id ); |
There was a problem hiding this comment.
Is there a reason to not require a new nonce for every login attempt?
class-two-factor-core.php
Outdated
| @@ -846,8 +866,6 @@ public static function login_form_validate_2fa() { | |||
| exit; | |||
| } | |||
|
|
|||
| self::delete_login_nonce( $user->ID ); | |||
There was a problem hiding this comment.
Nonces are now deleted whenever they are validated.
| @@ -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, | |||
There was a problem hiding this comment.
Do we really need a nonce that is valid for an hour? How about we reduce it to 10 minutes or something like that?
| @@ -775,16 +792,19 @@ public static function delete_login_nonce( $user_id ) { | |||
| */ | |||
| public static function verify_login_nonce( $user_id, $nonce ) { | |||
| $login_nonce = get_user_meta( $user_id, self::USER_META_NONCE_KEY, true ); | |||
| if ( ! $login_nonce ) { | |||
|
|
|||
| if ( ! $login_nonce || empty( $login_nonce['key'] ) || empty( $login_nonce['expiration'] ) ) { | |||
There was a problem hiding this comment.
Account for missing nonce params in the DB. This should never happen in normal circumstance.
| } | ||
| }, | ||
| "extra": { | ||
| "wordpress-install-dir": "wordpress" |
There was a problem hiding this comment.
This was needed after updating to roots/wordpress-full which has the core themes bundled.
|
Looks reasonable. Shouldn't need any migration tooling either, as login nonces don't last, anyone in the middle of a login over the upgrade can just log in again, nbd. |
|
This looks good at first glance Although I would like the $user_id and $expiration to be part of the MAC to absolutely guarantee that a given "challenge_token" can only ever be used for the user that created it and that the expiration can't be tampered with either. $data = [
'key' => bin2hex(random_bytes(32)),
'user_id' => $user->ID,
'expiration' => $expiration
]
$hashed_validator = hash_hmac('sha256', json_encode($data, JSON_THROW_ON_ERROR), wp_salt('nonce'));
$store_me = [
'expiration => $expiration,
'hashed_validator => $hashed_validator
]On the validation side you just reconstruct the validator from the single parts |
|
@calvinalkan we're working on another release in the (hopefully) near-term, so if you are able to push up a PR for your suggestion then I can coordinate to get review/merge on that... thanks! |
Hash the second factor login screen nonce in storage to avoid leaking it via DB access but pass in unhashed to the second factor input form.