WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12159 closed enhancement (fixed)

Define random keys and salts during setup-config.php

Reported by: nacin Owned by: ryan
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Security Keywords:
Focuses: Cc:

Description

Instead of simply linking to http://api.wordpress.org/secret-key/1.1/ in wp-config-sample.php, we should fetch that during setup-config.php and populate the key defines.

Defining keys after the fact generally causes cookie issues (see, for example, #12142), plus, there's no reason this shouldn't be done automatically.

I've attached a patch as an example. (In order to use http.php, we need apply_filters() and get_bloginfo(). The patch also produces a notice due to parse_url() -- again, just an example.)

This patch doesn't account for the three salts, which I think we should define as well. We should create a 1.2 version of the API and include those. (FWIW, the MU API includes salts but omits NONCE_KEY.)

Attachments (6)

12159.diff (1.9 KB) - added by nacin 4 years ago.
12159.2.diff (2.9 KB) - added by nacin 4 years ago.
Using the existing API. The get_bloginfo() hack is a little funky though.
setup-config.diff (536 bytes) - added by blepoxp 4 years ago.
Change substr() start position
12159.3.diff (3.1 KB) - added by nacin 4 years ago.
Allow fallback to wp_default_password()
12159.extraspecialchars.diff (643 bytes) - added by sivel 4 years ago.
Remove $extra_special_chars out from within the $special_chars if statement, so that you can use $extra_special_chars without $special_chars
12159.4.diff (2.4 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (30)

nacin4 years ago

nacin4 years ago

Using the existing API. The get_bloginfo() hack is a little funky though.

comment:2 in reply to: ↑ 1 nacin4 years ago

  • Keywords has-patch added

Replying to ryan:

http://api.wordpress.org/secret-key/1.1/?salt=1
returns the salts.

Thanks, patch updated with that.

comment:3 ryan4 years ago

(In [13026]) Update keys and salts with random values from api.wordpress.org. Props nacin. see #12159

blepoxp4 years ago

Change substr() start position

comment:4 blepoxp4 years ago

  • Cc glenn@… added
  • Keywords needs-testing added

I changed the start position of the substr on line 215 by one char because it kept creating syntax errors on my installation.
It was resulting in the following code without my patch.

define('AUTH_KEY',         ''%Gk-O^(Rn-U&@|a (KsjWQtXaK-tYck<gNrP}sn0ttd5b9yu[VpA+3i>Va Lj&$$');
define('SECURE_AUTH_KEY',  ''Mm~Ki0B(kY@op_,$To:=O:IhT`!F5/zMR&YY<$:Uh|;2]=.(n_$d7TVK Z$Q{iGC');
define('LOGGED_IN_KEY',    ''yylln)Lo^|?W0LYV)AiCxti%!pL|5uoGwn|^>*Q.}si_zAIRW)G^*AmPpj9:Y(/&');
define('NONCE_KEY',        ''VN7E{mGQgP3mZ.R>naE)@*-/@-B/TQO7L;Rd`JjVlblFE)M7-8&7bZt.Aj!P,}gA');
define('AUTH_SALT',        ''!_WMY`}W,xCtfSL{fJ.=uRAmQD]8*|3;Llw=af99]+Xlko:E@|,gOaH5k/[#;9jo');
define('SECURE_AUTH_SALT', ''OnO}*U:S&i4Sr9jvj+[|P{x*-j$s|5w;|@6(uO%FQC[qN&k Q}?0f6z|)`uk/ycI');
define('LOGGED_IN_SALT',   ''R|>7=]<vYWY&zEk[6,=s^k%a(RKy}~2g`bVD8Bo6({r++Xhs{hX}# TRT$)#+H-q');
define('NONCE_SALT',       ''JEbE<ihN_t)rfQX<3LRXd?Ozf%MAH{WBOO6z=cKL.-Z#T7U_R&o63UDJ6`iEG!dj');

Is this unique to my machine? If so, I can give you specs on my installation.

comment:5 nacin4 years ago

  • Keywords commit added; needs-testing removed

Not unique. At first I thought it was due to line endings, but it's not.

It's actually because the second patch accounted for a salt I forgot, SECURE_AUTH_SALT, which is one character longer than the next longest constant, hence every constant got another whitespace of padding. I just checked my local copy and I have 28 here. It didn't make it into the patch I uploaded, though.

comment:7 dd324 years ago

(In [13042]) Fix off-by-one substr() error. Props blepoxp. See #12159

comment:8 follow-up: dd324 years ago

[13026]

With the addition of the extra constants to the sample wp-config.php, It should be checked to see if the functions which use those fall back on a psuedorandom salt in the case that they are already defined.. Dont want half of any custom manual installs using the same salt..

comment:9 in reply to: ↑ 8 westi4 years ago

  • Keywords has-patch commit removed

Replying to dd32:

[13026]

With the addition of the extra constants to the sample wp-config.php, It should be checked to see if the functions which use those fall back on a psuedorandom salt in the case that they are already defined.. Dont want half of any custom manual installs using the same salt..

Reviewed with the following results:

  • SECRET_KEY, AUTH_KEY, SECURE_AUTH_KEY, LOGGED_IN_KEY, NONCE_KEY do check
  • AUTH_SALT, SECRET_SALT, SECURE_AUTH_SALT, LOGGED_IN_SALT, NONCE_SALT don't check.

Patch incoming.

comment:10 automattor4 years ago

(In [13058]) Ensure we ignore the _SALT defines if they are set to the "default" unconfigured values or empty. See #12159.

nacin4 years ago

Allow fallback to wp_default_password()

comment:12 sivel4 years ago

  • Cc matt@… added

comment:13 nacin4 years ago

I've uploaded a new patch, 12159.3.diff, that falls back to wp_generate_password() when the https request fails. (https is more likely to fail given that there might not be a transport available.)

It introduces a new constant, WP_SETUP_CONFIG, that way wp_generate_password() doesn't try to fetch transients that clearly do not exist. I find that cleaner than checking for function_exists().

I'm also considering additional special characters to wp_generate_password() (something sivel proposed in #8647) and also a way (via a URL variable, I imagine) to bypass the https check and go right to wp_generate_password(), to alleviate concerns in #8647.

comment:14 nacin4 years ago

(In [13133]) Fall back to wp_generate_password() in setup-config.php if HTTPS request for secret keys fails. Also use pretty link to secret-key API, see #12159

comment:15 nacin4 years ago

(In [13137]) Use an expanded special character set when generating auth keys and salts via wp_generate_password(). Props sivel, see #12159

comment:16 TobiasBg4 years ago

[13133] includes the line

static $seed = '';

Don't know if I'm right, but "static" is not available in PHP 4, is it?

comment:17 follow-up: dd324 years ago

Don't know if I'm right, but "static" is not available in PHP 4, is it?

Static Variables are, But Static Class members are not (IIRC)

comment:18 in reply to: ↑ 17 nacin4 years ago

Declaring a class method as static is PHP 5, but you can declare static variables. Same keyword, just different concepts.

Also, static variables != static properties. Static class properties are PHP 5.

comment:19 follow-up: aaroncampbell4 years ago

Regarding [13137], the parameters on wp_generate_password seem to work awkwardly. It seems that if you have two separate parameters for special characters and extra special characters, then you should be able to pass something like (12, false, true) and have it use standard and extra special, but not special (which it doesn't do). Alternatively, if the function should continue to work as it does, maybe we could remove the third parameter and let you pass something to specify a set of characters (Something like: 0 for standard, 1 for extended, 2 for all).

If we think one of these options should be used, let me know and I'll throw together a patch to apply.

comment:20 in reply to: ↑ 19 sivel4 years ago

Replying to aaroncampbell:

Regarding [13137], the parameters on wp_generate_password seem to work awkwardly. It seems that if you have two separate parameters for special characters and extra special characters, then you should be able to pass something like (12, false, true) and have it use standard and extra special, but not special (which it doesn't do). Alternatively, if the function should continue to work as it does, maybe we could remove the third parameter and let you pass something to specify a set of characters (Something like: 0 for standard, 1 for extended, 2 for all).

If we think one of these options should be used, let me know and I'll throw together a patch to apply.

comment:21 sivel4 years ago

Ok, not sure what happened there...

Replying to aaroncampbell:

Regarding [13137], the parameters on wp_generate_password seem to work awkwardly. It seems that if you have two separate parameters for special characters and extra special characters, then you should be able to pass something like (12, false, true) and have it use standard and extra special, but not special (which it doesn't do). Alternatively, if the function should continue to work as it does, maybe we could remove the third parameter and let you pass something to specify a set of characters (Something like: 0 for standard, 1 for extended, 2 for all).

If we think one of these options should be used, let me know and I'll throw together a patch to apply.

I see what you are saying, my original intention was to only allow $extra_special_chars if you were using $special_chars. Since that does seem a little akward, without context to the arguments, I would just move the $extra_special_chars out of the if for $special_chars.

Since it was an very easy change patch coming in a second.

sivel4 years ago

Remove $extra_special_chars out from within the $special_chars if statement, so that you can use $extra_special_chars without $special_chars

comment:22 nacin4 years ago

(In [13206]) Allow more special characters in wp_generate_password() second pass. props sivel, see #12159

nacin4 years ago

comment:23 nacin4 years ago

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

[13232] adds a URL query var to prevent the http call, and with that, this one is all set.

comment:24 hakre3 years ago

Related: #15088

Note: See TracTickets for help on using tickets.