[WIP] Add dedicated TOTP encryption salt

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( '<?php', '<?php ' . $new_constant, $config_contents );
+			$written         = file_put_contents( $config_path, $config_contents );
+
+			if ( $written ) {
+				define( $name, $salt_value );
+
+				$result = true;
+			}
+		}
+
+		return $result;
+	}
+
+	/**
+	 * Get the definition of a new salt constant, for `wp-config.php`
+	 *
+	 * @param string $name       The name of the constant.
+	 * @param string $salt_value The value of the constant.
+	 *
+	 * @return string
+	 */
+	protected static function get_config_salt_definition( $name, $salt_value ) {
+		$new_constant = "\n
+			/*
+			 * 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( '{$name}_MIGRATE', 'place the old value here' );
+			 *
+			 * See {@TODO support article URL} for more information.
+			 */
+			define( '$name', '$salt_value' );\n";
+
+		return str_replace( "\t", '', $new_constant );
+	}
 }

providers/class-two-factor-totp.php

@@ -31,6 +31,13 @@ class Two_Factor_Totp extends Two_Factor_Provider {
 	 */
 	const ACTION_SECRET_DELETE = 'totp-delete';
 
+	/**
+	 * Name of the constant used as an encryption salt
+	 *
+	 * @var string
+	 */
+	const ENCRYPTION_SALT_NAME = 'TWO_FACTOR_TOTP_ENCRYPTION_SALT';
+
 	const DEFAULT_KEY_BIT_SIZE        = 160;
 	const DEFAULT_CRYPTO              = 'sha1';
 	const DEFAULT_DIGIT_COUNT         = 6;
@@ -50,6 +57,9 @@ class Two_Factor_Totp extends Two_Factor_Provider {
 	 * @codeCoverageIgnore
 	 */
 	protected function __construct() {
+		self::maybe_create_config_salt( self::ENCRYPTION_SALT_NAME );
+
+		//add_action( 'admin_enqueue_scripts', array( $this, 'enqueue_assets' ) );
 		add_action( 'two_factor_user_options_' . __CLASS__, array( $this, 'user_two_factor_options' ) );
 		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' ) );
@@ -71,6 +81,31 @@ public static function get_instance() {
 		return $instance;
 	}
 
+	/**
+	 * Enqueue assets.
+	 *
+	 * @param string $hook Current page.
+	 */
+	//public static function enqueue_assets( $hook ) {
+	//	/* remove this if don't end up adding the warning */
+	//
+	//	if ( ! in_array( $hook, array( 'user-edit.php', 'profile.php' ), true ) ) {
+	//		return;
+	//	}
+	//
+	//	$user_id = Two_Factor_Core::current_user_being_edited();
+	//	if ( ! $user_id ) {
+	//		return;
+	//	}
+	//
+	//	wp_enqueue_style(
+	//		'totp-admin',
+	//		plugins_url( 'css/totp-admin.css', __FILE__ ),
+	//		null,
+	//		TWO_FACTOR_VERSION
+	//	);
+	//}
+
 	/**
 	 * Returns the name of the provider.
 	 */
@@ -298,6 +333,37 @@ public function admin_notices( $user_id ) {
 				<?php
 			}
 		}
+
+		// `self::maybe_create_config_salt()` wasn't able to create it, so ask admins to do it manually.
+		//if ( ! defined( self::ENCRYPTION_SALT_NAME ) && current_user_can( 'install_plugins' ) ) {
+		//	//  might need to use manage_network in multisite. test
+		//
+		//	// todo wait, this'll create 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
+		// moving this to a Site Health warning might give more room to explain, or maybe just rely on external article
+		//
+		//	$salt_value = wp_generate_password( 64, true, true );
+		//	// todo make DRY w/ base class unless it stays simple ^
+		//
+		//	printf(
+		//		'<div class="notice notice-warning inline">
+		//			<p>%s</p>
+		//			<pre><code>%s</code></pre>
+		//		</div>',
+		//
+		//		__( 'Could not create encryption key, please add this to your <code>wp-config.php</code>:', 'two-factor' ),
+		//		esc_html( trim( self::get_config_salt_definition( self::ENCRYPTION_SALT_NAME, $salt_value ) ) )
+		//	);
+		//}
 	}
 
 	/**

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;
+}

tests/providers/class-two-factor-provider.php

@@ -0,0 +1,104 @@
+<?php
+/**
+ * Test Two Factor Provider.
+ *
+ * @package Two_Factor
+ */
+
+/**
+ * Class Tests_Two_Factor_Provider
+ *
+ * @package Two_Factor
+ * @group providers
+ */
+class Tests_Two_Factor_Provider extends WP_UnitTestCase {
+	private static $config_path;
+	private static $original_config;
+	//private static $original_config_permissions;
+
+	/**
+	 * Setup shared fixtures before any tests run.
+	 *
+	 * @param WP_UnitTest_Factory $factory
+	 */
+	public static function wpSetUpBeforeClass( $factory ) {
+		//ini_set( 'realpath_cache_size', 0 ); didn't help
+
+		self::$config_path                 = Two_Factor_Provider::get_config_path();
+		self::$original_config             = file_get_contents( self::$config_path );
+		//self::$original_config_permissions = substr( sprintf( '%o', fileperms( self::$config_path ) ), -4 );
+
+		if ( empty( self::$original_config ) ) {
+			self::fail( 'Config file is empty.' );
+		}
+	}
+
+	/**
+	 * Restore global state between tests.
+	 */
+	public function tear_down() {
+		//chmod( self::$config_path, octdec( self::$original_config_permissions ) );
+		//clearstatcache here too, same as whatever below
+
+		$restored = file_put_contents( self::$config_path, self::$original_config );
+
+		if ( false === $restored ) {
+			self::fail( 'Failed to restore original config.' );
+		}
+
+		parent::tear_down();
+	}
+
+	/**
+	 * Test that new constants can be created.
+	 *
+	 * @covers Two_Factor_Provider::maybe_create_config_salt()
+	 */
+	public function test_create_new_constant() {
+		$constant_name = 'FOO_NEW';
+
+		// It doesn't exist yet
+		$this->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 );
+	//}
+}

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() {

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', '[email protected]' );
 define( 'WP_TESTS_TITLE', 'Test Blog' );