Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#35290 closed defect (bug) (fixed)

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

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

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 diddledani 11 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 diddledani 11 years ago.
second patch, replacing previous, following @dd32's suggested improvements

Download all attachments as: .zip

Change History (10)

#1 @jorbin
11 years ago

  • Milestone Awaiting Review → 4.5

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

@diddledani
11 years ago

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

#2 @diddledani
11 years ago

  • Keywords has-patch added

I believe this patch should do the job :-)

#4 @dd32
11 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()

@diddledani
11 years ago

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

#5 @jorbin
11 years ago

  • Keywords commit added
  • Owner set to dd32
  • Status new → assigned

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

#6 @dd32
11 years ago

  • Status assigned → accepted

#7 @dd32
11 years ago

  • Resolution → fixed
  • Status accepted → 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
11 years ago

  • Version trunk → 3.0
Note: See TracTickets for help on using tickets.