From 8e7e38c430d9e5e92d8b89aff56e545bb9d9dbac Mon Sep 17 00:00:00 2001 From: Ian Dunn Date: Thu, 20 Oct 2022 13:09:07 -0700 Subject: [PATCH] [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' );