Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 2 years ago

#19599 closed task (blessed) (fixed)

Localizations should not need to worry about the default secret key

Reported by: nacin's profile nacin Owned by: nacin's profile nacin
Milestone: 3.4 Priority: high
Severity: major Version: 3.4
Component: I18N Keywords: has-patch close
Focuses: Cc:

Description

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.

Attachments (7)

19599.patch (2.4 KB) - added by SergeyBiryukov 12 years ago.
19599.2.patch (5.8 KB) - added by SergeyBiryukov 12 years ago.
19599.3.patch (2.0 KB) - added by SergeyBiryukov 12 years ago.
19599.4.patch (1.9 KB) - added by SergeyBiryukov 12 years ago.
19599.diff (4.0 KB) - added by nacin 12 years ago.
19599.2.diff (5.4 KB) - added by nacin 12 years ago.
fattab.patch (846 bytes) - added by ocean90 12 years ago.

Download all attachments as: .zip

Change History (28)

#2 @SergeyBiryukov
12 years ago

  • Keywords has-patch added

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.

#3 @nacin
12 years ago

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.

#4 @SergeyBiryukov
12 years ago

Refreshed the patch to include the suggestions.

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

#5 @SergeyBiryukov
12 years ago

Simplified logic in 19599.4.patch

@nacin
12 years ago

#6 @nacin
12 years ago

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.
  1. If any two keys or salts match in value, the fallback is triggered.
  1. 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
12 years ago

#7 @nacin
12 years ago

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

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.

#8 @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#9 follow-up: @nacin
12 years ago

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.

#10 in reply to: ↑ 9 @nacin
12 years ago

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.

#11 @nacin
12 years ago

  • Priority changed from normal to high
  • Severity changed from normal to blocker

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.

#12 @nacin
12 years ago

  • Version set to 3.4

#14 @jane
12 years ago

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

#15 @markjaquith
12 years ago

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?

#16 @nacin
12 years ago

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.

#17 @nacin
12 years ago

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.

#18 @nacin
12 years ago

  • Keywords close added
  • Severity changed from blocker to major

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.

#19 @nacin
12 years ago

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

In [20894]:

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

#20 @nacin
12 years ago

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.

@ocean90
12 years ago

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.