From db24fa795f82ca4feb6183015988460b1182af85 Mon Sep 17 00:00:00 2001 From: Dhrupo Nil Date: Fri, 30 Oct 2026 12:04:41 +0600 Subject: [PATCH] Security: Harden secret names, value parameters and the root key on All Settings. Three small fixes to the Secrets API: * `wp_secrets_validate_name()` anchors its segment pattern with `\z`. With `$`, a name with a trailing newline passed, and since option names are trimmed it shared a row with the name without the newline while encrypting for itself. * Parameters that carry a plaintext value through `wp_set_secret()` and `wp_set_network_secret()` are marked `#[\SensitiveParameter]`, and so are the parameters that carry key material: `$key_material` on `WP_Secrets_Keyring::wrap()` and its implementations, and `$master_key` on the `WP_Secrets_Cipher` methods and `WP_Secrets_Libsodium_Provider::demote_slot()`. An exception thrown while saving does not print the value or a key in a stack trace. * Settings > All Settings masks `_wp_secrets_root_key` and excludes it from the save, as it already does for connector API keys. Replacing the root key made every stored secret unreadable. Props aryanjasala, ericmann, dhrupo. Fixes #66414. --- src/wp-admin/options.php | 4 + src/wp-includes/secrets.php | 23 +++- .../class-wp-secrets-broken-keyring.php | 5 +- .../class-wp-secrets-broken-provider.php | 9 +- .../secrets/class-wp-secrets-cipher.php | 28 ++++- .../class-wp-secrets-config-key-provider.php | 5 +- .../class-wp-secrets-libsodium-provider.php | 19 +++- .../secrets/interface-wp-secrets-keyring.php | 8 +- .../secrets/interface-wp-secrets-provider.php | 9 +- .../tests/secrets/wpSecretsValidateName.php | 3 + .../secrets/wpSetSecretAndWpGetSecret.php | 106 ++++++++++++++++++ 11 files changed, 205 insertions(+), 14 deletions(-) diff --git a/src/wp-admin/options.php b/src/wp-admin/options.php index 8f065f64473e6..8b69bfe459b2b 100644 --- a/src/wp-admin/options.php +++ b/src/wp-admin/options.php @@ -439,6 +439,10 @@ // Mask connector API keys and prevent updates from this screen. $value = _wp_connectors_mask_api_key( $option->option_value ); $disabled = true; + } elseif ( WP_Secrets_Key_Manager::ROOT_KEY_OPTION === $option->option_name ) { + // Replacing the root key makes every stored secret unreadable. + $value = _wp_connectors_mask_api_key( $option->option_value ); + $disabled = true; } else { $value = $option->option_value; $options_to_update[] = $option->option_name; diff --git a/src/wp-includes/secrets.php b/src/wp-includes/secrets.php index 99d87ed0d7413..4084b80ada874 100644 --- a/src/wp-includes/secrets.php +++ b/src/wp-includes/secrets.php @@ -249,7 +249,7 @@ function wp_secrets_validate_name( string $name ) { ); } - $segment_pattern = '/^[a-z0-9]([a-z0-9_-]*[a-z0-9])?$/'; + $segment_pattern = '/^[a-z0-9]([a-z0-9_-]*[a-z0-9])?\z/'; if ( 0 === $slashes ) { if ( ! preg_match( $segment_pattern, $name ) ) { @@ -423,7 +423,11 @@ function wp_using_secrets_dropin(): bool { * @param string $value The plaintext value to store. * @return true|WP_Error */ -function wp_set_secret( string $name, $value ) { +function wp_set_secret( + string $name, + #[\SensitiveParameter] + $value +) { return _wp_secrets_set( $name, $value, false ); } @@ -560,7 +564,11 @@ function wp_list_secrets( string $namespace = '' ) { // phpcs:ignore Universal.N * @param string $value The plaintext value to store. * @return true|WP_Error */ -function wp_set_network_secret( string $name, $value ) { +function wp_set_network_secret( + string $name, + #[\SensitiveParameter] + $value +) { return _wp_secrets_set( $name, $value, true ); } @@ -724,7 +732,14 @@ function wp_secrets_provider_label(): string { * wp_secret_changed. * @return true|WP_Error */ -function _wp_secrets_set( string $name, $value, bool $network, bool $needs_rotation = false, ?string $action_override = null ) { +function _wp_secrets_set( + string $name, + #[\SensitiveParameter] + $value, + bool $network, + bool $needs_rotation = false, + ?string $action_override = null +) { return _wp_secrets_get_provider()->set( $name, $value, $network, $needs_rotation, $action_override ); } diff --git a/src/wp-includes/secrets/class-wp-secrets-broken-keyring.php b/src/wp-includes/secrets/class-wp-secrets-broken-keyring.php index 85b51f9e05e67..1ad873345af85 100644 --- a/src/wp-includes/secrets/class-wp-secrets-broken-keyring.php +++ b/src/wp-includes/secrets/class-wp-secrets-broken-keyring.php @@ -30,7 +30,10 @@ final class WP_Secrets_Broken_Keyring implements WP_Secrets_Keyring { * @param string $key_material Ignored. * @return WP_Error */ - public function wrap( string $key_material ): WP_Error { + public function wrap( + #[\SensitiveParameter] + string $key_material + ): WP_Error { return $this->error(); } diff --git a/src/wp-includes/secrets/class-wp-secrets-broken-provider.php b/src/wp-includes/secrets/class-wp-secrets-broken-provider.php index 8c429c55e0705..8edeee93b1073 100644 --- a/src/wp-includes/secrets/class-wp-secrets-broken-provider.php +++ b/src/wp-includes/secrets/class-wp-secrets-broken-provider.php @@ -66,7 +66,14 @@ public function get( string $name, string $version, bool $network = false ): WP_ * @param string|null $action Ignored. * @return WP_Error */ - public function set( string $name, $value, bool $network = false, bool $needs_rotation = false, ?string $action = null ): WP_Error { + public function set( + string $name, + #[\SensitiveParameter] + $value, + bool $network = false, + bool $needs_rotation = false, + ?string $action = null + ): WP_Error { return $this->error(); } diff --git a/src/wp-includes/secrets/class-wp-secrets-cipher.php b/src/wp-includes/secrets/class-wp-secrets-cipher.php index 4dc631597ca3b..a1d526a0afb5f 100644 --- a/src/wp-includes/secrets/class-wp-secrets-cipher.php +++ b/src/wp-includes/secrets/class-wp-secrets-cipher.php @@ -69,7 +69,16 @@ final class WP_Secrets_Cipher { * @phpstan-param 'site'|'network' $scope * @phpstan-param WP_Secret_Version::CURRENT|WP_Secret_Version::PREVIOUS $slot */ - public function encrypt_value( string $master_key, string $scope, int $site_id, string $name, string $slot, $plaintext ) { + public function encrypt_value( + #[\SensitiveParameter] + string $master_key, + string $scope, + int $site_id, + string $name, + string $slot, + #[\SensitiveParameter] + $plaintext + ) { $check = $this->validate_common( $master_key, $scope, $site_id, $name, $slot ); if ( is_wp_error( $check ) ) { @@ -142,7 +151,15 @@ public function encrypt_value( string $master_key, string $scope, int $site_id, * @phpstan-param 'site'|'network' $scope * @phpstan-param WP_Secret_Version::CURRENT|WP_Secret_Version::PREVIOUS $slot */ - public function decrypt_value( string $master_key, string $scope, int $site_id, string $name, string $slot, $record ) { + public function decrypt_value( + #[\SensitiveParameter] + string $master_key, + string $scope, + int $site_id, + string $name, + string $slot, + $record + ) { $check = $this->validate_common( $master_key, $scope, $site_id, $name, $slot ); if ( is_wp_error( $check ) ) { @@ -211,7 +228,12 @@ public function decrypt_value( string $master_key, string $scope, int $site_id, * @param string $plaintext Value to fingerprint. * @return string|WP_Error 32-character hex string on success. WP_Error on failure. */ - public function fingerprint( string $master_key, $plaintext ) { + public function fingerprint( + #[\SensitiveParameter] + string $master_key, + #[\SensitiveParameter] + $plaintext + ) { if ( ! $this->is_valid_master_key( $master_key ) ) { return new WP_Error( WP_SECRETS_ERROR_KEY_UNAVAILABLE, diff --git a/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php b/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php index c99adc925ecd0..819814f4886bc 100644 --- a/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php +++ b/src/wp-includes/secrets/class-wp-secrets-config-key-provider.php @@ -84,7 +84,10 @@ public function __construct( bool $use_previous_key = false ) { * @param string $key_material Raw key material to protect. * @return string|WP_Error */ - public function wrap( string $key_material ) { + public function wrap( + #[\SensitiveParameter] + string $key_material + ) { if ( '' === $key_material ) { return new WP_Error( WP_SECRETS_ERROR_INVALID_VALUE, diff --git a/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php b/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php index 0c21195c64de4..ba4cc69cf8cd9 100644 --- a/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php +++ b/src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php @@ -245,7 +245,15 @@ private function slot_fingerprint( $slot ): string { * @phpstan-return array{dk: string, dk_nonce: string, ct: string, nonce: string, fingerprint: string, created: int, needs_rotation: bool}|WP_Error * @phpstan-param 'site'|'network' $scope */ - private function demote_slot( WP_Secrets_Cipher $cipher, string $master_key, string $scope, int $site_id, string $name, array $current_slot ) { + private function demote_slot( + WP_Secrets_Cipher $cipher, + #[\SensitiveParameter] + string $master_key, + string $scope, + int $site_id, + string $name, + array $current_slot + ) { $plaintext = $cipher->decrypt_value( $master_key, $scope, $site_id, $name, WP_Secret_Version::CURRENT, $current_slot ); if ( is_wp_error( $plaintext ) ) { @@ -289,7 +297,14 @@ private function demote_slot( WP_Secrets_Cipher $cipher, string $master_key, str * wp_import_option_as_secret() passes 'imported'. * @return true|WP_Error */ - public function set( string $name, $value, bool $network = false, bool $needs_rotation = false, ?string $action = null ) { + public function set( + string $name, + #[\SensitiveParameter] + $value, + bool $network = false, + bool $needs_rotation = false, + ?string $action = null + ) { $name_check = wp_secrets_validate_name( $name ); if ( is_wp_error( $name_check ) ) { diff --git a/src/wp-includes/secrets/interface-wp-secrets-keyring.php b/src/wp-includes/secrets/interface-wp-secrets-keyring.php index d378062908a9e..044e8270be0ff 100644 --- a/src/wp-includes/secrets/interface-wp-secrets-keyring.php +++ b/src/wp-includes/secrets/interface-wp-secrets-keyring.php @@ -30,12 +30,18 @@ interface WP_Secrets_Keyring { * re-wrapped value with update_site_option(), which reports an unchanged value * as a failure, and WP_Secrets_Keyring_Conformance checks this. * + * Attributes are not inherited, so an implementation must mark $key_material + * with #[\SensitiveParameter] itself to keep it out of stack traces. + * * @since 7.2.0 * * @param string $key_material Raw key material to protect. * @return string|WP_Error Opaque wrapped value on success, WP_Error on failure. */ - public function wrap( string $key_material ); + public function wrap( + #[\SensitiveParameter] + string $key_material + ); /** * Unwraps (decrypts) previously wrapped key material. diff --git a/src/wp-includes/secrets/interface-wp-secrets-provider.php b/src/wp-includes/secrets/interface-wp-secrets-provider.php index d1916aa7237f7..671855afa5579 100644 --- a/src/wp-includes/secrets/interface-wp-secrets-provider.php +++ b/src/wp-includes/secrets/interface-wp-secrets-provider.php @@ -138,7 +138,14 @@ public function get( string $name, string $version, bool $network = false ); * 'updated'. * @return true|WP_Error */ - public function set( string $name, $value, bool $network = false, bool $needs_rotation = false, ?string $action = null ); + public function set( + string $name, + #[\SensitiveParameter] + $value, + bool $network = false, + bool $needs_rotation = false, + ?string $action = null + ); /** * Deletes a secret. diff --git a/tests/phpunit/tests/secrets/wpSecretsValidateName.php b/tests/phpunit/tests/secrets/wpSecretsValidateName.php index 7555552d39674..6f88a5194ddeb 100644 --- a/tests/phpunit/tests/secrets/wpSecretsValidateName.php +++ b/tests/phpunit/tests/secrets/wpSecretsValidateName.php @@ -102,6 +102,9 @@ public static function data_invalid_names(): array { 'dot in key' => array( 'plugin/key.name' ), 'non-ascii' => array( 'plügin/key' ), 'one character over the max' => array( str_repeat( 'a', 85 ) . '/' . str_repeat( 'b', 87 ) ), + 'newline after key' => array( "plugin/key\n" ), + 'newline after namespace' => array( "plugin\n/key" ), + 'newline, unnamespaced' => array( "apikey\n" ), ); } } diff --git a/tests/phpunit/tests/secrets/wpSetSecretAndWpGetSecret.php b/tests/phpunit/tests/secrets/wpSetSecretAndWpGetSecret.php index a7943aee78067..90201ac2c64b2 100644 --- a/tests/phpunit/tests/secrets/wpSetSecretAndWpGetSecret.php +++ b/tests/phpunit/tests/secrets/wpSetSecretAndWpGetSecret.php @@ -231,4 +231,110 @@ function ( ...$args ) use ( &$captured_args ) { $this->assertStringNotContainsString( 'a-plaintext-value-that-must-not-leak', (string) $arg ); } } + + /** + * The options API trims option names, so a name with a trailing newline + * would share a row with the name without it while encrypting for itself. + * + * @ticket 66414 + */ + public function test_a_trailing_newline_name_cannot_overwrite_the_secret_without_it(): void { + wp_set_secret( 'myplugin/api-key', 'value-plain' ); + + $result = wp_set_secret( "myplugin/api-key\n", 'value-newline' ); + + $this->assertWPError( $result ); + $this->assertSame( WP_SECRETS_ERROR_INVALID_NAME, $result->get_error_code() ); + + $secret = wp_get_secret( 'myplugin/api-key' ); + + $this->assertInstanceOf( WP_Secret::class, $secret ); + $this->assertSame( 'value-plain', $secret->reveal() ); + } + + /** + * @ticket 66414 + * + * @requires PHP 8.2 + */ + public function test_an_exception_while_setting_does_not_expose_the_value_in_the_trace(): void { + if ( ini_get( 'zend.exception_ignore_args' ) ) { + $this->markTestSkipped( 'Exception traces do not include arguments with zend.exception_ignore_args enabled.' ); + } + + add_action( + 'wp_secret_changed', + static function () { + throw new RuntimeException( 'Listener failed.' ); + } + ); + + $exception = null; + + try { + wp_set_secret( 'myplugin/api-key', 'a-plaintext-value-that-must-not-leak' ); + } catch ( RuntimeException $e ) { + $exception = $e; + } + + $this->assertInstanceOf( RuntimeException::class, $exception ); + + $string_args = array(); + foreach ( $exception->getTrace() as $frame ) { + foreach ( $frame['args'] ?? array() as $arg ) { + if ( is_string( $arg ) ) { + $string_args[] = $arg; + } + } + } + + $this->assertContains( 'myplugin/api-key', $string_args, 'The trace should include the other arguments.' ); + $this->assertNotContains( 'a-plaintext-value-that-must-not-leak', $string_args ); + } + + /** + * @ticket 66414 + * + * @requires PHP 8.2 + * + * @dataProvider data_parameters_that_receive_a_secret_value + * + * @param callable-string|array{0: class-string, 1: string} $target The function or method. + * @param string $parameter The parameter name. + */ + public function test_parameters_that_receive_a_secret_value_are_sensitive( $target, string $parameter ): void { + $reflection = is_array( $target ) ? new ReflectionMethod( $target[0], $target[1] ) : new ReflectionFunction( $target ); + + foreach ( $reflection->getParameters() as $reflection_parameter ) { + if ( $parameter === $reflection_parameter->getName() ) { + $this->assertNotEmpty( $reflection_parameter->getAttributes( SensitiveParameter::class ) ); + return; + } + } + + $this->fail( "Parameter \${$parameter} not found." ); + } + + /** + * @return array + */ + public static function data_parameters_that_receive_a_secret_value(): array { + return array( + 'wp_set_secret()' => array( 'wp_set_secret', 'value' ), + 'wp_set_network_secret()' => array( 'wp_set_network_secret', 'value' ), + '_wp_secrets_set()' => array( '_wp_secrets_set', 'value' ), + 'WP_Secrets_Provider::set()' => array( array( WP_Secrets_Provider::class, 'set' ), 'value' ), + 'WP_Secrets_Libsodium_Provider::set()' => array( array( WP_Secrets_Libsodium_Provider::class, 'set' ), 'value' ), + 'WP_Secrets_Broken_Provider::set()' => array( array( WP_Secrets_Broken_Provider::class, 'set' ), 'value' ), + 'WP_Secrets_Cipher::encrypt_value()' => array( array( WP_Secrets_Cipher::class, 'encrypt_value' ), 'plaintext' ), + 'WP_Secrets_Cipher::fingerprint()' => array( array( WP_Secrets_Cipher::class, 'fingerprint' ), 'plaintext' ), + 'WP_Secrets_Cipher::encrypt_value() key' => array( array( WP_Secrets_Cipher::class, 'encrypt_value' ), 'master_key' ), + 'WP_Secrets_Cipher::decrypt_value() key' => array( array( WP_Secrets_Cipher::class, 'decrypt_value' ), 'master_key' ), + 'WP_Secrets_Cipher::fingerprint() key' => array( array( WP_Secrets_Cipher::class, 'fingerprint' ), 'master_key' ), + 'WP_Secrets_Keyring::wrap()' => array( array( WP_Secrets_Keyring::class, 'wrap' ), 'key_material' ), + 'WP_Secrets_Config_Key_Provider::wrap()' => array( array( WP_Secrets_Config_Key_Provider::class, 'wrap' ), 'key_material' ), + 'WP_Secrets_Broken_Keyring::wrap()' => array( array( WP_Secrets_Broken_Keyring::class, 'wrap' ), 'key_material' ), + 'WP_Secrets_Libsodium_Provider::demote_slot()' => array( array( WP_Secrets_Libsodium_Provider::class, 'demote_slot' ), 'master_key' ), + ); + } }