From 4fd044a3d16664960e32b2c04207deca3cc418f5 Mon Sep 17 00:00:00 2001 From: Soatok Dreamseeker Date: Mon, 19 Oct 2020 17:31:50 -0400 Subject: [PATCH 1/9] Encrypt TOTP secrets. This exposes a versioned authenticated encryption interface (powered by libsodium, which is polyfilled in WordPress via sodium_compat). Version 1 of the under-the-hood protocol uses the HMAC-SHA256 hash of a constant string and SECURE_AUTH_SALT to generate a key. This can be changed safely in version 2 without breaking old users. Version 1 uses XChaCha20-Poly1305 to encrypt the TOTP secrets. The authentication tag on the ciphertext is also validated over the user's database ID and the version prefix. Threat model: * Protects against read-only SQLi due to encryption. * Protects against copy-and-paste attacks (replacing TOTP secrets from one account with another's), since the ciphertext+tag are bound to the user's database ID. * Protects against chosen-ciphertext attacks (IND-CCA2). * Does not protect against replay attacks. * Does not protect against attackers capable of reading the salt from the filesystem. --- composer.json | 1 + providers/class-two-factor-totp.php | 165 +++++++++++++++++++++- tests/providers/class-two-factor-totp.php | 28 ++++ 3 files changed, 192 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index cb5117f7..b1a440c4 100644 --- a/composer.json +++ b/composer.json @@ -25,6 +25,7 @@ "minimum-stability": "dev", "prefer-stable" : true, "require": { + "paragonie/sodium_compat": "^1.13", "php": ">=5.6" }, "require-dev": { diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index e64ecd8a..85725497 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -37,6 +37,23 @@ class Two_Factor_Totp extends Two_Factor_Provider { const DEFAULT_TIME_STEP_SEC = 30; const DEFAULT_TIME_STEP_ALLOWANCE = 4; + /** + * Prefix for encrypted TOTP secrets. Contains a version identifier. + * + * $t1$ -> TOTP v1 (RFC 6238, encrypted with XChaCha20-Poly1305, with a key derived from HMAC-SHA256 + * of SECURE_AUTH_SAL.) + * + * @var string + */ + const ENCRYPTED_TOTP_PREFIX = '$t1$'; + + /** + * Current "version" of the TOTP encryption protocol. + * + * 1 -> $t1$nonce|ciphertext|tag + */ + const ENCRYPTED_TOTP_VERSION = 1; + /** * Characters used in base32 encoding. * @@ -218,7 +235,12 @@ public function user_two_factor_options_update( $user_id ) { * @return string */ public function get_user_totp_key( $user_id ) { - return (string) get_user_meta( $user_id, self::SECRET_META_KEY, true ); + $user_meta_value = get_user_meta( $user_id, self::SECRET_META_KEY, true ); + if ( ! self::is_encrypted( $user_meta_value ) ) { + $user_meta_value = self::encrypt( $user_meta_value, $user_id ); + update_user_meta( $user_id, self::SECRET_META_KEY, $user_meta_value ); + } + return self::decrypt( $user_meta_value, $user_id ); } /** @@ -230,7 +252,8 @@ public function get_user_totp_key( $user_id ) { * @return boolean If the key was stored successfully. */ public function set_user_totp_key( $user_id, $key ) { - return update_user_meta( $user_id, self::SECRET_META_KEY, $key ); + $encrypted = self::encrypt( $key, $user_id ); + return update_user_meta( $user_id, self::SECRET_META_KEY, $encrypted ); } /** @@ -575,4 +598,142 @@ private static function abssort( $a, $b ) { } return ( $a < $b ) ? -1 : 1; } + + /** + * Is this string an encrypted TOTP secret? + * + * @param string $secret Stored TOTP secret. + * @return bool + */ + public static function is_encrypted( $secret ) { + if ( strlen( $secret ) < 40 ) { + return false; + } + if ( strpos( $secret, self::ENCRYPTED_TOTP_PREFIX ) !== 0 ) { + return false; + } + return true; + } + + /** + * Encrypt a TOTP secret. + * + * @param string $secret TOTP secret. + * @param int $user_id User ID. + * @param int $version (Optional) Version ID. + * @return string + * @throws SodiumException From sodium_compat or ext/sodium. + */ + public static function encrypt( $secret, $user_id, $version = self::ENCRYPTED_TOTP_VERSION ) { + $prefix = self::get_version_header( $version ); + $nonce = random_bytes( 24 ); + $ciphertext = sodium_crypto_aead_xchacha20poly1305_ietf_encrypt( + $secret, + self::serialize_aad( $prefix, $nonce, $user_id ), + $nonce, + self::get_key( $version ) + ); + // @codingStandardsIgnoreStart + return self::ENCRYPTED_TOTP_PREFIX . base64_encode( $nonce . $ciphertext ); + // @codingStandardsIgnoreEnd + } + + /** + * Decrypt a TOTP secret. + * + * Version information is encoded with the ciphertext and thus omitted from this function. + * + * @param string $encrypted Encrypted TOTP secret. + * @param int $user_id User ID. + * @return string + * @throws RuntimeException Decryption failed. + */ + public static function decrypt( $encrypted, $user_id ) { + if ( strlen( $encrypted ) < 4 ) { + throw new RuntimeException( 'Message is too short to be encrypted' ); + } + $prefix = substr( $encrypted, 0, 4 ); + $version = self::get_version_id( $prefix ); + if ( 1 === $version ) { + // @codingStandardsIgnoreStart + $decoded = base64_decode( substr( $encrypted, 4 ) ); + // @codingStandardsIgnoreEnd + $nonce = RandomCompat_substr( $decoded, 0, 24 ); + $ciphertext = RandomCompat_substr( $decoded, 24 ); + try { + $decrypted = sodium_crypto_aead_xchacha20poly1305_ietf_decrypt( + $ciphertext, + self::serialize_aad( $prefix, $nonce, $user_id ), + $nonce, + self::get_key( $version ) + ); + } catch ( SodiumException $ex ) { + throw new RuntimeException( 'Decryption failed', 0, $ex ); + } + } else { + throw new RuntimeException( 'Unknown version: ' . $version ); + } + + // If we don't have a string, throw an exception because decryption failed. + if ( ! is_string( $decrypted ) ) { + throw new RuntimeException( 'Could not decrypt TOTP secret' ); + } + return $decrypted; + } + + /** + * Serialize the Additional Authenticated Data for TOTP secret encryption. + * + * @param string $prefix Version prefix. + * @param string $nonce Encryption nonce. + * @param int $user_id User ID. + * @return string + */ + public static function serialize_aad( $prefix, $nonce, $user_id ) { + return $prefix . $nonce . pack( 'N', $user_id ); + } + + /** + * Get the version prefix from a given version number. + * + * @param int $number Version number. + * @return string + * @throws RuntimeException For incorrect versions. + */ + final private static function get_version_header( $number = self::ENCRYPTED_TOTP_VERSION ) { + switch ( $number ) { + case 1: + return '$t1$'; + } + throw new RuntimeException( 'Incorrect version number: ' . $number ); + } + + /** + * Get the version prefix from a given version number. + * + * @param string $prefix Version prefix. + * @return int + * @throws RuntimeException For incorrect versions. + */ + final private static function get_version_id( $prefix = self::ENCRYPTED_TOTP_PREFIX ) { + switch ( $prefix ) { + case '$t1$': + return 1; + } + throw new RuntimeException( 'Incorrect version identifier: ' . $prefix ); + } + + /** + * Get the encryption key for encrypting TOTP secrets. + * + * @param int $version Key derivation strategy. + * @return string + * @throws RuntimeException For incorrect versions. + */ + final private static function get_key( $version = self::ENCRYPTED_TOTP_VERSION ) { + if ( 1 === $version ) { + return hash_hmac( 'sha256', SECURE_AUTH_SALT, 'totp-encryption', true ); + } + throw new RuntimeException( 'Incorrect version number: ' . $version ); + } } diff --git a/tests/providers/class-two-factor-totp.php b/tests/providers/class-two-factor-totp.php index d13611ae..9f21fd2d 100644 --- a/tests/providers/class-two-factor-totp.php +++ b/tests/providers/class-two-factor-totp.php @@ -312,4 +312,32 @@ public function test_user_can_delete_secret() { ); } + /** + * Verify the encryption and decryption functions behave correctly + * + * @throws SodiumException Libsodium can fail. + */ + public function test_encrypt_decrypt() { + $user = new WP_User( $this->factory->user->create() ); + $key = $this->provider->generate_key(); + + if ( ! defined( 'SECURE_AUTH_SALT' ) ) { + define( 'SECURE_AUTH_SALT', random_bytes( 32 ) ); + } + + $encrypted = Two_Factor_Totp::encrypt( $key, $user->ID ); + $this->assertEquals( + Two_Factor_Totp::ENCRYPTED_TOTP_PREFIX, + substr( $encrypted, 0, 4 ), + 'Encryption defaults to the latest version.' + ); + + $decrypted = Two_Factor_Totp::decrypt( $encrypted, $user->ID ); + $this->assertSame( + $key, + $decrypted, + 'Decrypted secret must be identical to plaintext' + ); + } + } From 78531cc2febb92c45891c2fa1cd082cf7037169e Mon Sep 17 00:00:00 2001 From: George Stephanis Date: Thu, 8 Sep 2022 12:42:22 -0400 Subject: [PATCH 2/9] Pull the utility functions into the abstract provider class. This will make it easier for any two-factor provider to encrypt their secrets. --- providers/class-two-factor-provider.php | 159 ++++++++++++++++++++++++ providers/class-two-factor-totp.php | 155 ----------------------- 2 files changed, 159 insertions(+), 155 deletions(-) diff --git a/providers/class-two-factor-provider.php b/providers/class-two-factor-provider.php index a2f9be06..89d1d688 100644 --- a/providers/class-two-factor-provider.php +++ b/providers/class-two-factor-provider.php @@ -14,6 +14,23 @@ */ abstract class Two_Factor_Provider { + /** + * Prefix for encrypted secrets. Contains a version identifier. + * + * $t1$ -> v1 (RFC 6238, encrypted with XChaCha20-Poly1305, with a key derived from HMAC-SHA256 + * of SECURE_AUTH_SAL.) + * + * @var string + */ + const ENCRYPTED_PREFIX = '$t1$'; + + /** + * Current "version" of the encryption protocol. + * + * 1 -> $t1$nonce|ciphertext|tag + */ + const ENCRYPTED_VERSION = 1; + /** * Class constructor. * @@ -99,4 +116,146 @@ public function get_code( $length = 8, $chars = '1234567890' ) { } return $code; } + + /** + * Is this string an encrypted secret? + * + * @param string $secret Stored secret. + * @return bool + */ + public static function is_encrypted( $secret ) { + if ( strlen( $secret ) < 40 ) { + return false; + } + // Should we add in a more complex check here for multiple prefixes if this changes? + if ( strpos( $secret, self::ENCRYPTED_PREFIX ) !== 0 ) { + return false; + } + return true; + } + + /** + * Encrypt a secret. + * + * @param string $secret secret. + * @param int $user_id User ID. + * @param int $version (Optional) Version ID. + * @return string + * @throws SodiumException From sodium_compat or ext/sodium. + */ + public static function encrypt( $secret, $user_id, $version = self::ENCRYPTED_VERSION ) { + $prefix = self::get_version_header( $version ); + $nonce = random_bytes( 24 ); + $ciphertext = sodium_crypto_aead_xchacha20poly1305_ietf_encrypt( + $secret, + self::serialize_aad( $prefix, $nonce, $user_id ), + $nonce, + self::get_encryption_key( $version ) + ); + // @codingStandardsIgnoreStart + return self::ENCRYPTED_PREFIX . base64_encode( $nonce . $ciphertext ); + // @codingStandardsIgnoreEnd + } + + /** + * Decrypt a secret. + * + * Version information is encoded with the ciphertext and thus omitted from this function. + * + * @param string $encrypted Encrypted secret. + * @param int $user_id User ID. + * @return string + * @throws RuntimeException Decryption failed. + */ + public static function decrypt( $encrypted, $user_id ) { + if ( strlen( $encrypted ) < 4 ) { + throw new RuntimeException( 'Message is too short to be encrypted' ); + } + $prefix = substr( $encrypted, 0, 4 ); + $version = self::get_version_id( $prefix ); + if ( 1 === $version ) { + // @codingStandardsIgnoreStart + $decoded = base64_decode( substr( $encrypted, 4 ) ); + // @codingStandardsIgnoreEnd + $nonce = RandomCompat_substr( $decoded, 0, 24 ); + $ciphertext = RandomCompat_substr( $decoded, 24 ); + try { + $decrypted = sodium_crypto_aead_xchacha20poly1305_ietf_decrypt( + $ciphertext, + self::serialize_aad( $prefix, $nonce, $user_id ), + $nonce, + self::get_encryption_key( $version ) + ); + } catch ( SodiumException $ex ) { + throw new RuntimeException( 'Decryption failed', 0, $ex ); + } + } else { + throw new RuntimeException( 'Unknown version: ' . $version ); + } + + // If we don't have a string, throw an exception because decryption failed. + if ( ! is_string( $decrypted ) ) { + throw new RuntimeException( 'Could not decrypt secret' ); + } + return $decrypted; + } + + /** + * Serialize the Additional Authenticated Data for secret encryption. + * + * @param string $prefix Version prefix. + * @param string $nonce Encryption nonce. + * @param int $user_id User ID. + * @return string + */ + public static function serialize_aad( $prefix, $nonce, $user_id ) { + return $prefix . $nonce . pack( 'N', $user_id ); + } + + /** + * Get the version prefix from a given version number. + * + * @param int $number Version number. + * @return string + * @throws RuntimeException For incorrect versions. + */ + final private static function get_version_header( $number = self::ENCRYPTED_VERSION ) { + switch ( $number ) { + case 1: + return '$t1$'; + } + throw new RuntimeException( 'Incorrect version number: ' . $number ); + } + + /** + * Get the version prefix from a given version number. + * + * @param string $prefix Version prefix. + * @return int + * @throws RuntimeException For incorrect versions. + */ + final private static function get_version_id( $prefix = self::ENCRYPTED_PREFIX ) { + switch ( $prefix ) { + case '$t1$': + return 1; + } + throw new RuntimeException( 'Incorrect version identifier: ' . $prefix ); + } + + /** + * Get the encryption key for encrypting secrets. + * + * If we want to change the salt that we're using to encrypt/decrypt, + * this is where we change it. + * + * @param int $version Key derivation strategy. + * @return string + * @throws RuntimeException For incorrect versions. + */ + final private static function get_encryption_key( $version = self::ENCRYPTED_VERSION ) { + if ( 1 === $version ) { + return hash_hmac( 'sha256', SECURE_AUTH_SALT, 'two-factor-encryption', true ); + } + throw new RuntimeException( 'Incorrect version number: ' . $version ); + } } diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index 85725497..ae773a9c 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -37,23 +37,6 @@ class Two_Factor_Totp extends Two_Factor_Provider { const DEFAULT_TIME_STEP_SEC = 30; const DEFAULT_TIME_STEP_ALLOWANCE = 4; - /** - * Prefix for encrypted TOTP secrets. Contains a version identifier. - * - * $t1$ -> TOTP v1 (RFC 6238, encrypted with XChaCha20-Poly1305, with a key derived from HMAC-SHA256 - * of SECURE_AUTH_SAL.) - * - * @var string - */ - const ENCRYPTED_TOTP_PREFIX = '$t1$'; - - /** - * Current "version" of the TOTP encryption protocol. - * - * 1 -> $t1$nonce|ciphertext|tag - */ - const ENCRYPTED_TOTP_VERSION = 1; - /** * Characters used in base32 encoding. * @@ -598,142 +581,4 @@ private static function abssort( $a, $b ) { } return ( $a < $b ) ? -1 : 1; } - - /** - * Is this string an encrypted TOTP secret? - * - * @param string $secret Stored TOTP secret. - * @return bool - */ - public static function is_encrypted( $secret ) { - if ( strlen( $secret ) < 40 ) { - return false; - } - if ( strpos( $secret, self::ENCRYPTED_TOTP_PREFIX ) !== 0 ) { - return false; - } - return true; - } - - /** - * Encrypt a TOTP secret. - * - * @param string $secret TOTP secret. - * @param int $user_id User ID. - * @param int $version (Optional) Version ID. - * @return string - * @throws SodiumException From sodium_compat or ext/sodium. - */ - public static function encrypt( $secret, $user_id, $version = self::ENCRYPTED_TOTP_VERSION ) { - $prefix = self::get_version_header( $version ); - $nonce = random_bytes( 24 ); - $ciphertext = sodium_crypto_aead_xchacha20poly1305_ietf_encrypt( - $secret, - self::serialize_aad( $prefix, $nonce, $user_id ), - $nonce, - self::get_key( $version ) - ); - // @codingStandardsIgnoreStart - return self::ENCRYPTED_TOTP_PREFIX . base64_encode( $nonce . $ciphertext ); - // @codingStandardsIgnoreEnd - } - - /** - * Decrypt a TOTP secret. - * - * Version information is encoded with the ciphertext and thus omitted from this function. - * - * @param string $encrypted Encrypted TOTP secret. - * @param int $user_id User ID. - * @return string - * @throws RuntimeException Decryption failed. - */ - public static function decrypt( $encrypted, $user_id ) { - if ( strlen( $encrypted ) < 4 ) { - throw new RuntimeException( 'Message is too short to be encrypted' ); - } - $prefix = substr( $encrypted, 0, 4 ); - $version = self::get_version_id( $prefix ); - if ( 1 === $version ) { - // @codingStandardsIgnoreStart - $decoded = base64_decode( substr( $encrypted, 4 ) ); - // @codingStandardsIgnoreEnd - $nonce = RandomCompat_substr( $decoded, 0, 24 ); - $ciphertext = RandomCompat_substr( $decoded, 24 ); - try { - $decrypted = sodium_crypto_aead_xchacha20poly1305_ietf_decrypt( - $ciphertext, - self::serialize_aad( $prefix, $nonce, $user_id ), - $nonce, - self::get_key( $version ) - ); - } catch ( SodiumException $ex ) { - throw new RuntimeException( 'Decryption failed', 0, $ex ); - } - } else { - throw new RuntimeException( 'Unknown version: ' . $version ); - } - - // If we don't have a string, throw an exception because decryption failed. - if ( ! is_string( $decrypted ) ) { - throw new RuntimeException( 'Could not decrypt TOTP secret' ); - } - return $decrypted; - } - - /** - * Serialize the Additional Authenticated Data for TOTP secret encryption. - * - * @param string $prefix Version prefix. - * @param string $nonce Encryption nonce. - * @param int $user_id User ID. - * @return string - */ - public static function serialize_aad( $prefix, $nonce, $user_id ) { - return $prefix . $nonce . pack( 'N', $user_id ); - } - - /** - * Get the version prefix from a given version number. - * - * @param int $number Version number. - * @return string - * @throws RuntimeException For incorrect versions. - */ - final private static function get_version_header( $number = self::ENCRYPTED_TOTP_VERSION ) { - switch ( $number ) { - case 1: - return '$t1$'; - } - throw new RuntimeException( 'Incorrect version number: ' . $number ); - } - - /** - * Get the version prefix from a given version number. - * - * @param string $prefix Version prefix. - * @return int - * @throws RuntimeException For incorrect versions. - */ - final private static function get_version_id( $prefix = self::ENCRYPTED_TOTP_PREFIX ) { - switch ( $prefix ) { - case '$t1$': - return 1; - } - throw new RuntimeException( 'Incorrect version identifier: ' . $prefix ); - } - - /** - * Get the encryption key for encrypting TOTP secrets. - * - * @param int $version Key derivation strategy. - * @return string - * @throws RuntimeException For incorrect versions. - */ - final private static function get_key( $version = self::ENCRYPTED_TOTP_VERSION ) { - if ( 1 === $version ) { - return hash_hmac( 'sha256', SECURE_AUTH_SALT, 'totp-encryption', true ); - } - throw new RuntimeException( 'Incorrect version number: ' . $version ); - } } From 41624631d66e1ea33e87b6b45579552395c7336c Mon Sep 17 00:00:00 2001 From: George Stephanis Date: Thu, 8 Sep 2022 12:45:31 -0400 Subject: [PATCH 3/9] Correct unit test reference to constant. It's defined in the parent class, but should still be accessible through the child class here. --- tests/providers/class-two-factor-totp.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/providers/class-two-factor-totp.php b/tests/providers/class-two-factor-totp.php index 9f21fd2d..2e4a4567 100644 --- a/tests/providers/class-two-factor-totp.php +++ b/tests/providers/class-two-factor-totp.php @@ -327,7 +327,7 @@ public function test_encrypt_decrypt() { $encrypted = Two_Factor_Totp::encrypt( $key, $user->ID ); $this->assertEquals( - Two_Factor_Totp::ENCRYPTED_TOTP_PREFIX, + Two_Factor_Totp::ENCRYPTED_PREFIX, substr( $encrypted, 0, 4 ), 'Encryption defaults to the latest version.' ); From b2af88135744e701614a5b367f6239d76efdbbd9 Mon Sep 17 00:00:00 2001 From: Ian Dunn Date: Thu, 20 Oct 2022 11:34:52 -0700 Subject: [PATCH 4/9] Require `RandomCompat_substr()` to avoid fatal error `random_compat/random.php` doesn't include `byte_safe_strings.php` on PHP 7+, because it claims the functions already exist, but `RandomCompat_substr()` is not part of Core PHP. --- providers/class-two-factor-provider.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/providers/class-two-factor-provider.php b/providers/class-two-factor-provider.php index 89d1d688..f384658b 100644 --- a/providers/class-two-factor-provider.php +++ b/providers/class-two-factor-provider.php @@ -171,6 +171,9 @@ public static function decrypt( $encrypted, $user_id ) { if ( strlen( $encrypted ) < 4 ) { throw new RuntimeException( 'Message is too short to be encrypted' ); } + + require_once ABSPATH . WPINC . '/random_compat/byte_safe_strings.php'; + $prefix = substr( $encrypted, 0, 4 ); $version = self::get_version_id( $prefix ); if ( 1 === $version ) { From 8419e431bf39aa74a9d5e7300420d0d131bb6e28 Mon Sep 17 00:00:00 2001 From: Ian Dunn Date: Thu, 20 Oct 2022 11:35:54 -0700 Subject: [PATCH 5/9] Use `wp_salt()` instead of `SECURE_AUTH_SALT` for compatibility. `SECURE_AUTH_SALT` isn't going to always be defined on older installations, and may be defined as "put your unique phrase here" in semi-broken configs. `wp_salt( 'secure_auth' )` provides a fallback in those environments. Co-Authored-By: Dion Hulse --- providers/class-two-factor-provider.php | 2 +- tests/providers/class-two-factor-totp.php | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/providers/class-two-factor-provider.php b/providers/class-two-factor-provider.php index f384658b..9bc1ad0c 100644 --- a/providers/class-two-factor-provider.php +++ b/providers/class-two-factor-provider.php @@ -257,7 +257,7 @@ final private static function get_version_id( $prefix = self::ENCRYPTED_PREFIX ) */ final private static function get_encryption_key( $version = self::ENCRYPTED_VERSION ) { if ( 1 === $version ) { - return hash_hmac( 'sha256', SECURE_AUTH_SALT, 'two-factor-encryption', true ); + return hash_hmac( 'sha256', wp_salt( 'secure_auth' ), 'two-factor-encryption', true ); } throw new RuntimeException( 'Incorrect version number: ' . $version ); } diff --git a/tests/providers/class-two-factor-totp.php b/tests/providers/class-two-factor-totp.php index 2e4a4567..737efb42 100644 --- a/tests/providers/class-two-factor-totp.php +++ b/tests/providers/class-two-factor-totp.php @@ -318,13 +318,9 @@ public function test_user_can_delete_secret() { * @throws SodiumException Libsodium can fail. */ public function test_encrypt_decrypt() { - $user = new WP_User( $this->factory->user->create() ); + $user = new WP_User( self::factory()->user->create() ); $key = $this->provider->generate_key(); - if ( ! defined( 'SECURE_AUTH_SALT' ) ) { - define( 'SECURE_AUTH_SALT', random_bytes( 32 ) ); - } - $encrypted = Two_Factor_Totp::encrypt( $key, $user->ID ); $this->assertEquals( Two_Factor_Totp::ENCRYPTED_PREFIX, From 57065caad170c7339ab0a17d4073bdad1a79bc64 Mon Sep 17 00:00:00 2001 From: Ian Dunn Date: Fri, 21 Oct 2022 10:01:22 -0700 Subject: [PATCH 6/9] Remove `final` because private methods can't be overridden --- providers/class-two-factor-provider.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/providers/class-two-factor-provider.php b/providers/class-two-factor-provider.php index 9bc1ad0c..fe6881aa 100644 --- a/providers/class-two-factor-provider.php +++ b/providers/class-two-factor-provider.php @@ -222,7 +222,7 @@ public static function serialize_aad( $prefix, $nonce, $user_id ) { * @return string * @throws RuntimeException For incorrect versions. */ - final private static function get_version_header( $number = self::ENCRYPTED_VERSION ) { + private static function get_version_header( $number = self::ENCRYPTED_VERSION ) { switch ( $number ) { case 1: return '$t1$'; @@ -237,7 +237,7 @@ final private static function get_version_header( $number = self::ENCRYPTED_VERS * @return int * @throws RuntimeException For incorrect versions. */ - final private static function get_version_id( $prefix = self::ENCRYPTED_PREFIX ) { + private static function get_version_id( $prefix = self::ENCRYPTED_PREFIX ) { switch ( $prefix ) { case '$t1$': return 1; @@ -255,7 +255,7 @@ final private static function get_version_id( $prefix = self::ENCRYPTED_PREFIX ) * @return string * @throws RuntimeException For incorrect versions. */ - final private static function get_encryption_key( $version = self::ENCRYPTED_VERSION ) { + private static function get_encryption_key( $version = self::ENCRYPTED_VERSION ) { if ( 1 === $version ) { return hash_hmac( 'sha256', wp_salt( 'secure_auth' ), 'two-factor-encryption', true ); } From 8e7e38c430d9e5e92d8b89aff56e545bb9d9dbac Mon Sep 17 00:00:00 2001 From: Ian Dunn Date: Thu, 20 Oct 2022 13:09:07 -0700 Subject: [PATCH 7/9] [WIP] Add dedicated TOTP encryption salt --- providers/class-two-factor-provider.php | 99 ++++++++++++++++- providers/class-two-factor-totp.php | 66 +++++++++++ providers/css/totp-admin.css | 32 ++++++ tests/providers/class-two-factor-provider.php | 104 ++++++++++++++++++ tests/providers/class-two-factor-totp.php | 7 ++ tests/wp-config.php | 14 +++ 6 files changed, 320 insertions(+), 2 deletions(-) create mode 100644 providers/css/totp-admin.css create mode 100644 tests/providers/class-two-factor-provider.php diff --git a/providers/class-two-factor-provider.php b/providers/class-two-factor-provider.php index fe6881aa..2a940625 100644 --- a/providers/class-two-factor-provider.php +++ b/providers/class-two-factor-provider.php @@ -18,7 +18,8 @@ abstract class Two_Factor_Provider { * Prefix for encrypted secrets. Contains a version identifier. * * $t1$ -> v1 (RFC 6238, encrypted with XChaCha20-Poly1305, with a key derived from HMAC-SHA256 - * of SECURE_AUTH_SAL.) + * of the child class's `ENCRYPTION_SALT_NAME`. If that doesn't exist, the key falls back + * to `wp_salt( 'secure_auth' )`. * * @var string */ @@ -257,8 +258,102 @@ private static function get_version_id( $prefix = self::ENCRYPTED_PREFIX ) { */ private static function get_encryption_key( $version = self::ENCRYPTED_VERSION ) { if ( 1 === $version ) { - return hash_hmac( 'sha256', wp_salt( 'secure_auth' ), 'two-factor-encryption', true ); + $name = get_called_class()::ENCRYPTION_SALT_NAME; + $salt = defined( $name ) ? constant( $name ) : wp_salt( 'secure_auth' ); + + return hash_hmac( 'sha256', $salt, 'two-factor-encryption', true ); } throw new RuntimeException( 'Incorrect version number: ' . $version ); } + + /** + * Get the path to `wp-config.php`. + * + * If merging to Core, move this to `wp-load.php` and use there for DRYness. + * + * @return string + */ + public static function get_config_path() { + $path = ''; + + if ( file_exists( ABSPATH . 'wp-config.php' ) ) { + $path = ABSPATH . 'wp-config.php'; + } elseif ( file_exists( dirname( ABSPATH ) . '/wp-config.php' ) && ! file_exists( dirname( ABSPATH ) . '/wp-settings.php' ) ) { + // The config file resides one level above ABSPATH but is not part of another installation. + $path = dirname( ABSPATH ) . '/wp-config.php'; + } + + return $path; + } + + /** + * Attempt to create the given constant if it doesn't already exist. + * + * If created, it'll also be defined so it can be used in the current process. + * + * @param string $name + * + * @return bool `true` if already exists, `true` if created, `false` if could not be created. + */ + public static function maybe_create_config_salt( $name ) { + if ( defined( $name ) ) { + return true; + } + + $result = false; + $config_path = self::get_config_path(); + $config_contents = file_get_contents( $config_path ); + + // Need to check this in addition to `defined()` above, to avoid writing duplicates + // during edge cases where the config isn't included (like certain unit tests setups). + $already_exists = false !== stripos( $config_contents, $name ); + + if ( ! $already_exists && is_writable( $config_path ) ) { + // todo in test suite iswritable always returns true, even when file is chmod 444 + // also todo, locally it still writes to conf file when chmod 444 - retest now that have changed some things + + $salt_value = wp_generate_password( 64, true, true ); + // todo is wp_generate_pw strong enough here, or need to copy setup-config.php use of random_int() ? + // see https://core.trac.wordpress.org/ticket/35290 and others from blame + // doesn't wpgenpw also uses a cspring, and the same alphabet+length? + + $new_constant = self::get_config_salt_definition( $name, $salt_value ); + + // Put it at the beginning for simplicity/reliability. + $config_contents = str_replace( ' + //

%s

+ // 
%s
+ // ', + // + // __( 'Could not create encryption key, please add this to your wp-config.php:', 'two-factor' ), + // esc_html( trim( self::get_config_salt_definition( self::ENCRYPTION_SALT_NAME, $salt_value ) ) ) + // ); + //} } /** diff --git a/providers/css/totp-admin.css b/providers/css/totp-admin.css new file mode 100644 index 00000000..c944e904 --- /dev/null +++ b/providers/css/totp-admin.css @@ -0,0 +1,32 @@ +/* remove this if don't end up adding the warning */ + +.two-factor-methods-table .notice { + //width: auto; + /*max-width: 100%;*/ +/* overflow-x: scroll;*/ + /*width: clamp( 300px, 60%, 400px ); + max-width: clamp( 300px, 60%, 400px );*/ + overflow: hidden; +} + +.two-factor-methods-table .notice pre { + /*width: 100%; + max-width: 100%; + overflow-x: scroll;*/ + + background-color: #d7d7d7; + overflow: hidden; +} + +.two-factor-methods-table .notice pre code { + /* todo get this to not overflow page */ + + /*display: block;*/ + padding: 10px; + overflow: scroll; +/* + width: clamp( 300px, 60%, 400px ); + max-width: 400px;*/ + + background-color: transparent; +} diff --git a/tests/providers/class-two-factor-provider.php b/tests/providers/class-two-factor-provider.php new file mode 100644 index 00000000..72e0391d --- /dev/null +++ b/tests/providers/class-two-factor-provider.php @@ -0,0 +1,104 @@ +assertFalse( defined( $constant_name ) ); + $this->assertFalse( stripos( self::$original_config, $constant_name ) ); + + $result = Two_Factor_Provider::maybe_create_config_salt( $constant_name ); + $new_config = file_get_contents( self::$config_path ); + + // It does exist now + $this->assertTrue( $result ); + $this->assertTrue( defined( $constant_name ) ); + $this->assertTrue( 64 === strlen( constant( $constant_name ) ) ); + $this->assertNotEmpty( $new_config ); + $this->assertGreaterThan( 0, stripos( $new_config, $constant_name ) ); + } + + /** + * Test that existing constants aren't redefined. + * + * @covers Two_Factor_Provider::maybe_create_config_salt() + */ + public function test_create_existing_constant() { + $this->assertTrue( defined( 'DB_NAME' ) ); + $result = Two_Factor_Provider::maybe_create_config_salt( 'DB_NAME' ); + $this->assertTrue( $result ); + $this->assertSame( self::$original_config, file_get_contents( self::$config_path ) ); + } + + /** + * Test that unwritable files return false + * + * @covers Two_Factor_Provider::maybe_create_config_salt() + */ + //public function test_unwritable_config() { + // // todo ugh don't waste more time on this, just test it manually once to make sure works and can leave this test out + // + // chmod( self::$config_path, 0444 ); + // clearstatcache( true, self::$config_path ); // doesn't work, neither does any other variation of params + // $this->assertFalse( is_writable( self::$config_path ) ); + // // todo ^ says can write even though perms are 444 + // + // $this->assertFalse( defined( 'FOO_UNWRITABLE' ) ); + // $result = Two_Factor_Provider::maybe_create_config_salt( 'FOO_UNWRITABLE' ); + // $this->assertFalse( $result ); + //} +} diff --git a/tests/providers/class-two-factor-totp.php b/tests/providers/class-two-factor-totp.php index 737efb42..6a1d7fa8 100644 --- a/tests/providers/class-two-factor-totp.php +++ b/tests/providers/class-two-factor-totp.php @@ -315,6 +315,13 @@ public function test_user_can_delete_secret() { /** * Verify the encryption and decryption functions behave correctly * + * @covers Two_Factor_Provider::encrypt() + * @covers Two_Factor_Provider::get_version_header() + * @covers Two_Factor_Provider::serialize_aad() + * @covers Two_Factor_Provider::get_encryption_key() + * @covers Two_Factor_Provider::decrypt() + * @covers Two_Factor_Provider::get_version_id() + * * @throws SodiumException Libsodium can fail. */ public function test_encrypt_decrypt() { diff --git a/tests/wp-config.php b/tests/wp-config.php index f95043f9..d5ea3051 100644 --- a/tests/wp-config.php +++ b/tests/wp-config.php @@ -25,6 +25,20 @@ $table_prefix = getenv( 'WORDPRESS_TABLE_PREFIX' ) ?: 'wpphpunittests_'; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited + +/* + * Warning: Changing this value will break decryption for existing users, and prevent + * them from logging in with this factor. If you change this you must create a constant + * to facilitate migration: + * + * define( 'TWO_FACTOR_TOTP_ENCRYPTION_SALT_MIGRATE', 'place the old value here' ); + * + * See {@TODO support article URL} for more information. + */ +define( 'TWO_FACTOR_TOTP_ENCRYPTION_SALT', '4N:v{FDL,s?:UM[[1>?.:Dq?=Iwh5%z]!f,2-6rDyv0/-za<03;q`J-YV:QOu;&3' ); +define( 'SECURE_AUTH_SALT', '389lrsuytneiarsm39p80talurynetim32ta790stjuynareitm3298pluynatri' ); + + define( 'WP_TESTS_DOMAIN', 'example.org' ); define( 'WP_TESTS_EMAIL', 'admin@example.org' ); define( 'WP_TESTS_TITLE', 'Test Blog' ); From 2bcc7ed4c83d5fbdaf583f2a6c14ac932bb66a31 Mon Sep 17 00:00:00 2001 From: Ian Dunn Date: Tue, 25 Oct 2022 16:00:32 -0700 Subject: [PATCH 8/9] wip - add warnings if salt not defined --- providers/class-two-factor-totp.php | 119 ++++++++++++++++++++-------- 1 file changed, 88 insertions(+), 31 deletions(-) diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index 01b41ca4..85845497 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -64,6 +64,7 @@ protected function __construct() { add_action( 'personal_options_update', array( $this, 'user_two_factor_options_update' ) ); add_action( 'edit_user_profile_update', array( $this, 'user_two_factor_options_update' ) ); add_action( 'two_factor_user_settings_action', array( $this, 'user_settings_action' ), 10, 2 ); + add_filter( 'site_status_tests', array( $this, 'add_site_health_checks' ) ); return parent::__construct(); } @@ -167,7 +168,22 @@ public function user_two_factor_options( $user ) { $key = $this->generate_key(); $site_name = get_bloginfo( 'name', 'display' ); $totp_title = apply_filters( 'two_factor_totp_title', $site_name . ':' . $user->user_login, $user ); + $show_salt_warning = ! defined( self::ENCRYPTION_SALT_NAME ) && current_user_can( 'install_plugins' ); + // todo might need to use manage_network in multisite. test + ?> + + +
+

+ wp-config.php. Please see Site Health tool for details.', 'two-factor' ), + admin_url('site-health.php') + ); ?> +

+
+ +

@@ -333,37 +349,6 @@ public function admin_notices( $user_id ) { - //

%s

- // 
%s
- // ', - // - // __( 'Could not create encryption key, please add this to your wp-config.php:', 'two-factor' ), - // esc_html( trim( self::get_config_salt_definition( self::ENCRYPTION_SALT_NAME, $salt_value ) ) ) - // ); - //} } /** @@ -647,4 +632,76 @@ private static function abssort( $a, $b ) { } return ( $a < $b ) ? -1 : 1; } + + /** + * Add checks for the Site Health screen. + * + * @param array $tests + * + * @return array + */ + public function add_site_health_checks( $tests ) { + $tests['direct']['two_factor_totp_encryption_salt_exists'] = array( + 'label' => 'Two Factor TOTP Encryption salt exists', + 'test' => array( $this, 'encryption_salt_exists_check' ), + ); + + return $tests; + } + + /** + * Checks that the encryption salt exists for Site Health. + * + * @return array + */ + public function encryption_salt_exists_check() { + $result = array( + 'label' => __( 'Two Factor TOTP Encryption salt exists', 'two-factor' ), + 'status' => 'good', + 'test' => 'two_factor_totp_encryption_salt_exists', + 'actions' => '', + + 'badge' => array( + 'label' => __( 'Security' ), + 'color' => 'blue', + ), + + 'description' => '

' . sprintf( + __( 'The %s constant exists in %s, which is necessary for strong TOTP authentication.', 'two-factor' ), + '' . self::ENCRYPTION_SALT_NAME . '', + 'wp-config.php' + ) . '

', + ); + + if ( ! defined( self::ENCRYPTION_SALT_NAME ) ) { + $result['label'] = __( "Two Factor TOTP Encryption salt doesn't exist", 'two-factor' ); + $result['status'] = 'critical'; + + $salt_value = wp_generate_password( 64, true, true ); + // todo needs to match whatever's used in maybe_create_config_salt() + + $result['description'] = sprintf( + '

%s

%s

%s

', + __( 'Could not automatically create encryption key, which weakens the security of TOTP authentication. Please add this to your wp-config.php:', 'two-factor' ), + esc_html( trim( self::get_config_salt_definition( self::ENCRYPTION_SALT_NAME, $salt_value ) ) ), + __( 'If users have already setup TOTP, then adding that will prevent them from logging in with TOTP. You can rotate the keys to fix this, or allow them to log in with their backup factor and re-setup TOTP. ', 'two-factor' ), + // todo need to give more info on rotating + // todo overflow-x: scroll, maybe need word break too? + + // todo this creates a situation where _have_ to rotate keys, b/c previously fell back to using wp_salt() + // so would have to give them migration instructions + // better to just do nothing and let them use wpsalt()? + // but could be high correlation between sites that expect strong security and sites that have wp-config unwritable by php + // so that'd be using weaker security on those sites without the admins even knowing it + // maybe better to not fallback to wpsalt() and require the constant be configured? + // maybe disable provider if it doesn't exist? + // then show warning here and on plugins.php? + // actually, using the wpsalt() fallback is only weaker if they don't have SECURE_NONCE_SALT setup in wpconfig + // that's rare, especially among sites that care about security + // so probably fine to just silently fall back rather than bothering with all this + ); + } + + return $result; + } } From e7cd00ae8a07e8f0944e1e638d8a3cf84d7061bb Mon Sep 17 00:00:00 2001 From: Ian Dunn Date: Tue, 25 Oct 2022 16:01:39 -0700 Subject: [PATCH 9/9] wip - catch decrypt fatal. need to ensure good UX --- providers/class-two-factor-totp.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index 85845497..bc29d5fe 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -274,7 +274,17 @@ public function get_user_totp_key( $user_id ) { $user_meta_value = self::encrypt( $user_meta_value, $user_id ); update_user_meta( $user_id, self::SECRET_META_KEY, $user_meta_value ); } - return self::decrypt( $user_meta_value, $user_id ); + + try { + $decrypted = self::decrypt( $user_meta_value, $user_id ); + } catch ( RuntimeException $exception ) { + $decrypted = ''; + // todo this is probably wrong. + // er maybe not + // means that the salt changed, and they need to rotate + } + + return $decrypted; } /**