WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35290 closed defect (bug) (fixed)

Use CSPRNG to generate salt instead of API, if possible.

Reported by: sarciszewski Owned by: dd32
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.0
Component: Security Keywords: has-patch commit
Focuses: administration Cc:
PR Number:

Description

https://security.stackexchange.com/questions/61676/why-do-wordpress-installations-retrieve-the-crypto-secrets-remotely-on-installat

https://github.com/WordPress/WordPress/blob/ced6b063a3b7cb7f7b33d9eafb356881ca1ea199/wp-admin/setup-config.php#L282

Since we have random_bytes() as of WordPress 4.4 (even on PHP 5.2.x), I propose something like this:

try {
	$chars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!@#$%^&*()-_ []{}<>~`+=,.;:/?|';
	$maxl = strlen($chars) - 1;
	for ( $i = 0; $i < 8; $i++ ) {
		$key = '';
		for ( $j = 0; $j < 64; $j++ ) {
			$key .= $chars[random_int(0, $max)];
		}
		$secret_keys[] = $key;
	}
} catch (Exception $ex) {
	$secret_keys = wp_remote_get( 'https://api.wordpress.org/secret-key/1.1/salt/' );
}

Instead of just using wp_generate_password(64, true true), this prioritizes the CSPRNG and falls back to the API approach if it's not available (since wp_generate_password() falls back silently if random_compat throws an Exception).

Attachments (2)

35290.diff (1.8 KB) - added by diddledan 4 years ago.
Attempt at using CSPRNG falling back to API (if enabled) and further falling back to original wp_generate_password()
35290.2.diff (1.8 KB) - added by diddledan 4 years ago.
second patch, replacing previous, following @dd32's suggested improvements

Download all attachments as: .zip

Change History (10)

#1 @jorbin
4 years ago

  • Milestone changed from Awaiting Review to 4.5

This seems like a good extension of the random_bytes work done in 4.4

@diddledan
4 years ago

Attempt at using CSPRNG falling back to API (if enabled) and further falling back to original wp_generate_password()

#2 @diddledan
4 years ago

  • Keywords has-patch added

I believe this patch should do the job :-)

#4 @dd32
4 years ago

@diddledan Thanks for the patch! A few things though

  • There's a typo, $maxl instead of $max
  • In general we avoid array-syntax access to arrays in favour of substr()

@diddledan
4 years ago

second patch, replacing previous, following @dd32's suggested improvements

#5 @jorbin
4 years ago

  • Keywords commit added
  • Owner set to dd32
  • Status changed from new to assigned

This looks good. @dd32 want to get this in?

#6 @dd32
4 years ago

  • Status changed from assigned to accepted

#7 @dd32
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 36872:

Setup config: Generate the default secret keys & salts from the local CSPRNG if available, falling back to the WordPress.org API and a backup psuedo random source.

Props diddledan.
Fixes #35290

#8 @johnbillion
4 years ago

  • Version changed from trunk to 3.0
Note: See TracTickets for help on using tickets.