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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5ff442a
Store the login nonce hashed
kasparsd Sep 8, 2022
f6a5019
Merge remote-tracking branch 'origin/master' into add/totp-hashes
kasparsd Sep 8, 2022
0c01f88
Add tests
kasparsd Sep 8, 2022
58ff60f
Ensure all required keys are present
kasparsd Sep 8, 2022
702adc6
Use the correct order
kasparsd Sep 8, 2022
13f8dfa
Some more tests
kasparsd Sep 8, 2022
9f5a9a1
Update the env to run the tests locally
kasparsd Sep 8, 2022
1722119
Update WP too
kasparsd Sep 8, 2022
5c293a4
Formatting
kasparsd Sep 8, 2022
c16e184
Require a fresh nonce for each request
kasparsd Sep 8, 2022
1ab863c
Use a more reliable API for getting the home URL
kasparsd Sep 8, 2022
6db97cd
Delete the nonce only if it is incorrect and in the DB
kasparsd Sep 8, 2022
6844ad6
Test that invalid nonce checks delete nonces
kasparsd Sep 8, 2022
b146d33
Install WP with all default themes
kasparsd Sep 8, 2022
b1b106b
Reorder packages
kasparsd Sep 8, 2022
0cafae8
Per linter
kasparsd Sep 8, 2022
1dfe473
We need the WP core installers package to place it in a known location
kasparsd Sep 8, 2022
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
40 changes: 30 additions & 10 deletions class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ public static function backup_2fa() {
}

if ( true !== self::verify_login_nonce( $user->ID, $nonce ) ) {
wp_safe_redirect( get_bloginfo( 'url' ) );
wp_safe_redirect( home_url() );
exit;
}

Expand Down 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 @@ -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'] ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Account for missing nonce params in the DB. This should never happen in normal circumstance.

return false;
}

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

return true;
// Require a fresh nonce if verification fails.
self::delete_login_nonce( $user_id );

return false;
}

/**
Expand All @@ -806,7 +826,7 @@ public static function login_form_validate_2fa() {
}

if ( true !== self::verify_login_nonce( $user->ID, $nonce ) ) {
wp_safe_redirect( get_bloginfo( 'url' ) );
wp_safe_redirect( home_url() );
exit;
}

Expand Down
25 changes: 15 additions & 10 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,32 @@
"issues": "https://github.com/WordPress/two-factor/issues"
},
"config": {
"sort-packages": true,
"platform": {
"php": "5.6.20"
},
"allow-plugins": {
"roots/wordpress-core-installer": true,
"dealerdirect/phpcodesniffer-composer-installer": true
"dealerdirect/phpcodesniffer-composer-installer": true,
"roots/wordpress-core-installer": true
}
},
"extra": {
"wordpress-install-dir": "wordpress"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was needed after updating to roots/wordpress-full which has the core themes bundled.

},
"require": {
"php": ">=5.6"
},
"require-dev": {
"php-coveralls/php-coveralls": "^2.4",
"wp-coding-standards/wpcs": "^2.3",
"roots/wordpress": "^5.9",
"wp-phpunit/wp-phpunit": "^5.9",
"phpunit/phpunit": "^5",
"yoast/phpunit-polyfills": "^1.0",
"phpcompatibility/phpcompatibility-wp": "^2.1",
"automattic/vipwpcs": "^2.3",
"dealerdirect/phpcodesniffer-composer-installer": "^0.7.2",
"automattic/vipwpcs": "^2.3"
"php-coveralls/php-coveralls": "^2.5",
"phpcompatibility/phpcompatibility-wp": "^2.1",
"phpunit/phpunit": "^5",
"roots/wordpress-core-installer": "^1.100",
"roots/wordpress-full": "^6.0",
"wp-coding-standards/wpcs": "^2.3",
"wp-phpunit/wp-phpunit": "^6.0",
"yoast/phpunit-polyfills": "^1.0"
},
"scripts": {
"lint": "phpcs",
Expand Down
Loading