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-provider.php b/providers/class-two-factor-provider.php index a2f9be06..2a940625 100644 --- a/providers/class-two-factor-provider.php +++ b/providers/class-two-factor-provider.php @@ -14,6 +14,24 @@ */ 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 the child class's `ENCRYPTION_SALT_NAME`. If that doesn't exist, the key falls back + * to `wp_salt( 'secure_auth' )`. + * + * @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 +117,243 @@ 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' ); + } + + require_once ABSPATH . WPINC . '/random_compat/byte_safe_strings.php'; + + $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. + */ + 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. + */ + 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. + */ + private static function get_encryption_key( $version = self::ENCRYPTED_VERSION ) { + if ( 1 === $version ) { + $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( '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') + ); ?> +

+
+ +

@@ -218,7 +269,22 @@ 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 ); + } + + 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; } /** @@ -230,7 +296,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 +642,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; + } } 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 d13611ae..6a1d7fa8 100644 --- a/tests/providers/class-two-factor-totp.php +++ b/tests/providers/class-two-factor-totp.php @@ -312,4 +312,35 @@ 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() { + $user = new WP_User( self::factory()->user->create() ); + $key = $this->provider->generate_key(); + + $encrypted = Two_Factor_Totp::encrypt( $key, $user->ID ); + $this->assertEquals( + Two_Factor_Totp::ENCRYPTED_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' + ); + } + } 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' );