Localizations should not need to worry about the default secret key

[nacin]

Currently localizations must define $wp_default_secret_key in a $locale.php file in order to translate the key placeholder strings and not weaken wp_salt().

This is less than ideal. Let's automagically detect whether the default value is still being used, even if it has been translated, and prevent the need for the $wp_default_secret_key global.

[nacin]

See #14024.

[SergeyBiryukov]

We could check if the same (non-empty) value is used for more than two secret keys and assume it's $wp_default_secret_key then.

19599.patch​ is an attempt to implement this.

19599.2.patch​ goes a bit further and cleans up repeated defined() and '' != ... checks.

[nacin]

Coding logic still seems a bit dense and repetitive, but it's a good start. I agree with the angle, though we could probably go with two keys, rather than more than two. SECRET_KEY should be excluded from the count, as it is not one of the eight.

[SergeyBiryukov]

Refreshed the patch to include the suggestions.

I've also removed the checks for empty values for the count, seems they are unnecessary.

[SergeyBiryukov]

Simplified logic in 19599.4.patch​

[nacin]

19599.2.diff​ rewrites wp_salt().

1. DB fallback is provided for the eight main keys and salts, and also SECRET_KEY, since it is used for custom schemes.

2. If any two keys or salts match in value, the fallback is triggered.

3. If any key or salt matches 'put your unique phrase here', the fallback is triggered.

Now, local builds. We want to eliminate the need for $locale.php files, so we want to avoid $wp_default_secret_key. Additionally, #14024 has caused translators to instead override default-constants.php. This is even less desirable.

Point 2 ensures that a localized 'put your unique phrase here' only works if the value is used exactly once — say, if the other 7 are properly unique. However, it could also be that the only one defined is SECRET_KEY (perhaps the install started with 2.5), and either way, we should try to mitigate this.

So, Point 4: For 3.4 localized builds, we will append $wp_secret_key_default to version.php, the same way we append $wp_local_package to version.php. We will use this variable to ensure that for all fresh local installs of 3.4 or higher will end up covered.

For installs prior to 3.4, we cannot use the existing $wp_default_secret_key, as otherwise we will re-introduce #14024. again, point 2 is strong enough to mitigate any issues.

[nacin]

In [19771]:

Provide a DB fallback for keys in wp_salt(). Fall back when any secret is used more than once. Change how we detect a localized 'put your unique phrase here' -- eliminate $wp_default_secret_key and introduce $wp_secret_key_default to be added during the localized build process, not by translators. fixes #19599.

[nacin]

This could potentially cause a log-out after an upgrade, and based on how $wp_locale_package can get overridden in an upgrade, we may need to store default keys in the DB.

[nacin]

In [20135]:

Internally cache return values of wp_salt(). Always run the filter. Big performance gains on a pageload that generates hundreds of nonces. see #19599.

[nacin]

Replying to nacin:

In [20135]:

Internally cache return values of wp_salt(). Always run the filter. Big performance gains on a pageload that generates hundreds of nonces. see #19599.

wp_create_nonce() was 2% of the processing time on themes.php with wpcom-themes in place. This reduced it down to 0.28%, with no flexibility lost for the filter.

[nacin]

Things left to do:

• Remove $wp_secret_key_default, as with the duplicate key checks, this is isn't really necessary. (This could also cause problems when you temporarily update to an English translation of a new version before going back to the locale.)

• In admin/includes/update-core.php, re-issue cookies using the new secret keys, to prevent a logout on upgrade.

Can consider this a blocker for RC1.

[SergeyBiryukov]

[20691]

[jane]

@nacin: 3 weeks ago you said this would be a blocker for RC1, but don't see any activity on this. Update?

[markjaquith]

Having the update-initiating user get kicked out during the update process is unacceptable. Terrible user experience. That's the blocker. Less of a blocker, but still annoying, is that other site users will get their cookies invalidated.

Proposal: for 3.4, we continue to accept login cookies salted with the old default constant salts, and upgrade them to the new cookie with the db-driven salts on the fly. Then, for 3.5, we stop accepting the old default constant salts. This puts off the security-enhancing benefit, but will result in a better user experience, as only people who haven't logged in to WP since 3.3 will get the boot in 3.5.

Thoughts?

[nacin]

Good conversation with MarkJaquith about this in IRC: ​https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-05-24&sort=asc#m398731

The proposal seems sound, and I can work on it tomorrow, once I get some sleep and ryan can catch up on the IRC log and weigh in.

[nacin]

In [20887]:

When auth_redirect() detects a logged out user and the target
page was about.php?updated, then issue a message welcoming them.

This is to prevent it from being so jolting if you are taken to
the login screen after an update.

In WordPress 3.4, the changes to wp_salt() provide for extra
security, but will cause a log-out for any installs without 8
unique keys and salts in wp-config.php (with some exceptions).
Properly re-issuing cookies, even for the logged in user, is
not easily doable via admin/includes/update-core.php, as that
file is included long after the headers are sent.

see #19599.

[nacin]

After lots of additional discussion, [20887] is about as good as it is going to get. It is not easy in a 3.3 environment (so admin/includes/update-core.php) to issue 3.4 cookies, as headers are already sent. It is not easy in a 3.4 environment (so post-upgrade) to allow 3.3 cookies to be valid, as we won't have things like $wp_default_secret_key to work with for localized builds, and it results in a lot of extra code for not much benefit.

Leaving open so we can test this a bit further, but it's done as far as I can tell.

[nacin]

In [20894]:

Use correct variable when redirecting to about.php from update-core.php. fixes #19599.

[nacin]

In [20895]:

Run _redirect_to_about_wordpress() in 3.4 due to the change in [20894] and the reliance on ?updated in [20887]. see #19599.