wp_signups.activation_key stores activation keys in plain text

[tomdxw]

Steps

1. Visit /wp-admin/user-new.php (on a multisite installation - I haven't tested on single site)
2. Fill out the "Add New User" form but do not check the "Skip Confirmation Email" checkbox
3. The user will be sent an email containing a link to /wp-activate.php?key=7259c714857ef009

Actual behaviour

This key is stored in the database unencrypted:

mysql> select activation_key from wp_signups where signup_id=4;
+------------------+
| activation_key   |
+------------------+
| 7259c714857ef009 |
+------------------+
1 row in set (0.00 sec)

Expected behaviour

wp_users.user_activation_key contains a timestamp and a hash of the key. wp_signups.activation_key is no less important to security and so should include these security features too.

[madvic]

+1

[bor0]

Hi,

The current proposed patch (38474.patch) will implement this feature, with additionally providing signup_id to be able to retrieve records from the database.

Example workflow (given a multi-side WordPress setup):

1. Visit /wp-admin/user-new.php
2. Fill out the "Add New User" form but do not check the "Skip Confirmation Email" checkbox
3. The user will be sent an email containing a link to /wp-activate.php?key=<KEY>&signup_id=<SIGNUP_ID>
4. Check the records in the database

   mysql> select * from wp_signups;
   +-----------+--------+------+-------+------------+----------------------+---------------------+---------------------+--------+------------------------------------+--------------------------------------------------------------------+
   | signup_id | domain | path | title | user_login | user_email           | registered          | activated           | active | activation_key                     | meta                                                               |
   +-----------+--------+------+-------+------------+----------------------+---------------------+---------------------+--------+------------------------------------+--------------------------------------------------------------------+
   |         1 |        |      |       | testa      | ..........@......com | 2017-01-15 01:04:44 | 0000-00-00 00:00:00 |      0 | $P$BOEBNJ7xvVIc6JfWQuFzip208ua.5b0 | a:2:{s:11:"add_to_blog";s:1:"1";s:8:"new_role";s:10:"subscriber";} |
   +-----------+--------+------+-------+------------+----------------------+---------------------+---------------------+--------+------------------------------------+--------------------------------------------------------------------+
   1 row in set (0.00 sec)
   

5. When the user clicks the link, they should be able to login (there is a check for both signup_id and activation_key in the backend)

[bor0]

@SergeyBiryukov could you please review/provide your input on this?

Probably not that big of a security issue, as someone that has access to the db has access to all of the content more or less. However, if they use an activation key they can login and upload files, delete files, etc.

[tomdxw]

This weakness has been assigned CVE-2017-14990 by MITRE.

[chriscct7]

The early tags are used to milestone events needing to occur early in a cycle. WordPress 4.9 has reached Beta already, so this will not be making the 4.9 release.

Aside from that, it doesn't appear anyone's yet answered how this patch affects users who have already been issued an email to activate an account, then the upgrade to this patch occurs, what happens to those links? Do they continue working?

[jeremyfelt]

Thanks for opening a ticket, @tomdxw.

In the future, if you believe you are reporting a security vulnerability, please follow the guidelines at ​https://make.wordpress.org/core/handbook/testing/reporting-security-vulnerabilities/. With the text entered in the original issue, there should have also been a required check-box input confirming that a security vulnerability was not being reported.

That said, this is an area that could use some hardening and is okay to be fixed as a public ticket. I don't believe a CVE is necessary. See #24783 as an example of a related issue that has been addressed publicly in the past. Ideally we'll be able to use a similar fix to help communicate the activation key change to any pending users.

@bor0 - Thank you for the initial patch. I think you're on the right path. It'd be good if we can resolve this without the addition of another parameter on the URL (signup_id). See [25696] for an example of how we've handled an old format and new format at the same time. Using the plain text key in the activation URL is okay because we can compare it with an old or new (hashed) version in the DB. I'm going to assign ownership of the ticket to you and will happily review ongoing patches. :)

I'm going to put this in the 5.0 milestone for now, though we may be able to ship it as part of a 4.9.1 release with the right progress.

[bor0]

@jeremyfelt thanks for the example changeset! That approach looks good to me. I will rework the patch.

[bor0]

Hey @jeremyfelt!

Looking at the previous patch I just recalled why I introduced signup_id to the GET parameter.

It's so that we don't need to get all the rows from $wpdb->signups, and call CheckPassword on each one of them to see if it matches. We can get rid of signup_id but it's probably faster to do it this way?

In #24783 they use the same approach, but use user_login instead of signup_id. However, we don't have user_login in this context.

In any case I updated the patch to throw a WP_Error in the case of $key === $signup->activation_key for legacy data, and also did some code style fixes and updated the filters to contain the hashed key as well.

Let me know how that looks and we can go from there.

Thanks!

[SergeyBiryukov]

Replying to bor0:

Looking at the previous patch I just recalled why I introduced signup_id to the GET parameter.

It's so that we don't need to get all the rows from $wpdb->signups, and call CheckPassword on each one of them to see if it matches. We can get rid of signup_id but it's probably faster to do it this way?

I might be missing something, but wpmu_activate_signup() only gets one row (WHERE activation_key = %s), why would it get all the rows from $wpdb->signups? I still don't see the need for signup_id there.

On a related note, the patch adds a Signup ID input to the activation form. Where the user is supposed to get that value?

[tomdxw]

@bor0:

The wp_users.user_activation_key field includes a timestamp, and by default the activation link expires after 24 hours. This means that if somebody receives an activation email but they forget about it, and then an attacker gains access to their email days or weeks later, the attacker doesn't have instant access to the site.

​https://github.com/WordPress/WordPress/blob/2ad86e1e82722d8cdae17ff10e34672c8e6ab93a/wp-includes/user.php#L2195-L2196

​https://github.com/WordPress/WordPress/blob/2ad86e1e82722d8cdae17ff10e34672c8e6ab93a/wp-includes/user.php#L2248-L2271

I think it would be appropriate to use a timestamp here also.

[bor0]

Replying to SergeyBiryukov:

I might be missing something, but wpmu_activate_signup() only gets one row (WHERE activation_key = %s), why would it get all the rows from $wpdb->signups? I still don't see the need for signup_id there.

This is done so that we catch any legacy activation keys (see check where $key === $signup->activation_key).

Replying to tomdxw:

I think it would be appropriate to use a timestamp here also.

I like this approach. Thanks! I will be updating the patch.

[bor0]

Add timestamp expire check

[bor0]

Bugfixes after local testings and code reorganization

[flixos90]

This one is ready for review.

[peterwilsoncc]

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

[Morno]

when will this be resolved?

[beyernreich]

Replying to Morno:

when will this be resolved?

I would also be very happy if this security gap were resolved. Otherwise it will be harder to argue to customers that WordPress can be a secure platform for web applications.

## Patch Testing & Adaptation Report

### Description
This report validates that the new adapted patch is working, although it requires further testing
IMPORTANT NOTE FOR CODE REVIEWERS: I've tried to adapt the patch _as-it-was_. I have not further reviewed if the code is fully correct.

### Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.4.6
• Server: nginx/1.27.4
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.4.6)
• Browser: Chrome 135.0.0.0
• OS: Windows 10/11
• Theme: Twenty Twenty-Five 1.2
• MU Plugins: None activated
• Plugins:
  • Micro Email Testing 1.0.0
  • Test Reports 1.2.0

### Actual Results

1. ✅ Issue resolved with patch.

### Additional Notes
The 8 years ago original patch wasn't applying because multiple changes have been introduced since then.
To name some:

1. PHP 8.0 forces mandatory fields like $signup_id to go in front of optional like $meta
2. This section:

   $key = !empty($_GET['key']) ? $_GET['key'] : $_POST['key'];
   $result = wpmu_activate_signup( $key );
   

Doesn't exist anymore. I've taking just the logic for $signup_id but probably more checks should be introduced.

One additional consideration:
With the new block themes, since wp-activate doesn't have a template, resulting in Deprecation notices like:

[17-Apr-2025 15:50:24 UTC] PHP Deprecated:  File Theme without header.php is <strong>deprecated</strong> since version 3.0.0 with no alternative available. Please include a header.php template in your theme. in /var/www/src/wp-includes/functions.php on line 6120
[17-Apr-2025 15:50:24 UTC] PHP Deprecated:  File Theme without footer.php is <strong>deprecated</strong> since version 3.0.0 with no alternative available. Please include a footer.php template in your theme. in /var/www/src/wp-includes/functions.php on line 6120

This obviously is not happening with classic themes Adaptation

Trac ticket: https://core.trac.wordpress.org/ticket/38474

[SirLouen]

Raising attention Security team: @whyisjake @johnbillion

[SirLouen]

I've also finished sorting the unit tests.
Ready for dev-review

[johnbillion]

#63573 was marked as a duplicate.

[dmsnell]

@bor0 @SergeyBiryukov not sure if you two are still familiar with this. I also have questions about the introduction of signup_id

@bor0 is it a problem to fetch all stored activation keys per login and email combination? I would think that we would intentionally want to ensure that we don’t leak information that could be used in a timing attack to expose information about the key generation.

I’m not sure what we are trying to protect with it, so I am probably just overlooking its value. My impression is that in general, we should expect one or a countable few number of activation keys per username and email, and since we are already querying on those, the database traffic would be light.

in fact, given that expiration is hard-coded in WordPress, we might also be able to cull results simply by adding a WHERE clause on registered column.

either way, it seems like it should be unnecessary and a single key should be suitable.

---

did we consider sending some kind of HMAC or signature over the request? I think we should be able to send a key via email that doesn’t exist in the database, but which can be used to reconstruct what’s in the database and look for an exact match.

## Summary

• wp_signups.activation_key stored activation keys as plain text. This patches it to use the same timestamp:phpass_hash format already used by wp_users.user_activation_key (introduced in [25696]).
• Activation URLs gain a signup_id parameter so the correct row can be fetched for hash verification without a table scan.
• Legacy plain-text keys (pre-upgrade pending activations) continue to work for backwards compatibility.
• A new activate_signup_expiration filter (default: DAY_IN_SECONDS) controls key expiry.
• 9 new PHPUnit tests; one existing test fixed to work with hashed keys.

Fixes #38474. See also: https://core.trac.wordpress.org/ticket/38474

Props bor0, tomdxw, jeremyfelt, SergeyBiryukov, SirLouen, dmsnell.

## Test plan

### Automated (PHPUnit)

npm install
# edit .env: set LOCAL_MULTISITE=true
npm run env:start
npm run env:install

# New tests:
npm run test:php -- -c tests/phpunit/multisite.xml   --filter Tests_Multisite_wpmuActivateSignup

# Fixed regression test:
npm run test:php -- -c tests/phpunit/multisite.xml   --filter test_should_not_fail_for_data_used_by_a_deleted_user

All 9 tests should pass.

### Manual

1. Hashed key in DB — Register at /wp-signup.php as a logged-out user. Check wp_signups.activation_key: should be 1700000000:$P$Bxxx…, not a plain hex string.

2. Activation link works — The email link contains both key= and signup_id=. Clicking it shows "Your account is now active!"

3. Signup ID field on form — Visit /wp-activate.php with no params. The form should show both "Activation Key" and "Signup ID" fields.

4. Wrong key rejected — Visit /wp-activate.php?key=WRONGKEY&signup_id=<valid_id>. Activation must fail.

5. Legacy key BC — Insert a row into wp_signups with a plain-text activation_key (simulates a pre-upgrade pending activation). Visiting the activation URL with that key and its signup_id should still succeed — existing pending activations must not break after upgrade.

6. Expiry — Add add_filter('activate_signup_expiration', fn() => -1) to an mu-plugin, sign up, try to activate. Must fail with an expired-key error.

🤖 Generated with ​Claude Code

[bor0]

@dmsnell thanks for raising this one, it's been quite some time since I last tackled it :)

I refreshed the patch and opened a PR: ​https://github.com/WordPress/wordpress-develop/pull/12235

Regarding your questions:

signup_id: After hashing, WHERE activation_key = %s can no longer match the plain-text key from the email URL. We need an indexed column to fetch the right row before calling CheckPassword(). Also, user_login/user_email aren't in the URL, and selecting through all pending rows with CheckPassword() will be slow. signup_id is not sensitive and solves this with a single indexed lookup.

HMAC: If AUTH_KEY ever rotates, all pending activations break simultaneously with no recovery. The phpass approach doesn't have that failure mode.

The patch also fixes the BC issue raised in #9 — legacy plain-text keys (pre-upgrade pending activations) now continue to work instead of being rejected.

[dmsnell]

user_login/user_email aren't in the URL

Seems like email could be in the URL if we added it, especially if we send something like "{$email}:{$timestamp}:{$key}"

If this triplet were sent via email and we only stored the hash in the database, could we not reconstruct that hash and never store the key outside of the email?

<?php

if ( false === ( $provided_data = base64_decode( $link_payload ) ) {
        return new WP_Error( … );
}

$provided_parts = preg_split( '~:~', $provided_data );
if ( 3 !== count( $provided_parts ) ) {
        return new WP_Error( … );
}

list( $email, $timestamp, $key ) = $provided_data;
$last_registration = expiry_window( $timestamp );
$hash = wp_hmac( $provided_data );

$query = $wpdb->prepare(
        'SELECT * FROM $wpdb->signups WHERE user_email = %s AND activation_key = %s AND registered >= %s',
        $email,
        $hash,
        $last_registration
);

$signup = $wpdb->get_row( $query );

signup_id is not sensitive

I guess I’m just trying to be sure that we aren’t overlooking something because we think that the addition of a safe extra parameter keeps the system safe. With the signup_key, we might be exposing more knowledge about the internal activation keys because someone can make requests against one signup key using generated activation keys.

all pending activations break simultaneously…legacy plain-text keys (pre-upgrade pending activations) now continue to work

we can also look at upgrading all existing keys that are not expired, but I wonder how important this issue is given that activation keys are ephemeral in nature. if AUTH_KEY rotates, that seems like a great opportunity to expire pending activations.

we can also ensure key upgrades during a WordPress update simply by adding a prefix noting the key version, like "{v2-hmac}:{$hash}" for new keys and "{v2-pending}:{$legacy_key}" for those keys which were not expired during the database update. then our query contains one OR but either way, can still be based purely on an identity lookup on the indexed activation_key column.

but I wonder how significant the need is to add special handling for the set of signups that are pending during the site upgrade. it will be a 24-hour period, but realistically, we might expect most of those to be resolved within a few minutes of sending the email, and for the short window during the site upgrade, there may not be as many signups as usual (since operators are likely to upgrade their sites off-peak).

not saying we definitely shouldn’t support these, but raising the question whether it’s essential. if it is essential, we have multiple ways that we could do this which don’t involve creating two lookup values.

[bor0]

Hey @dmsnell, thanks for the feedback.

I went ahead and implemented the HMAC approach, updated the PR ​https://github.com/WordPress/wordpress-develop/pull/12235

The flow is now:

1. Build a triplet email:timestamp:random
2. Store base64(HMAC-SHA256(triplet, AUTH_KEY+AUTH_SALT)) in the DB
3. Send base64url(triplet) in the email.

On activation: decode the triplet, recompute the HMAC, query WHERE user_email = %s AND activation_key = %s. No signup_id in URLs at all.

On the legacy BC: kept the fallback since the recovery UX if a pending activation breaks is poor (no retry flow for signups), and the activate_signup_expiration filter means some sites have windows well beyond 24h.

Your versioning idea ({v2-hmac}:{hash} / {v2-pending}:{legacy_key} via dbDelta) is cleaner and would let us drop the fallback eventually. Happy to add that as a follow-up if you prefer

Thanks @dmsnell, @peterwilsoncc - addressed your comments, lmk how this looks! 🙇‍♂️

[dmsnell]

@bor0 thanks for all the effort you are putting into this now after all this time ☺️

what are your thoughts on the newer HMAC approach? I definitely like that we are back to a direct one-item lookup in the database, have a single key, and that key is not stored in the database.

one thing that remains is that a change to the site’s salt will invalidate existing registration records, but I think that’s appropriate and good to do.

[bor0]

@dmsnell I like the HMAC approach, especially given your previous reasoning how it's consistent with how WordPress already handles salt rotations invalidating sessions.

one thing that remains is that a change to the site’s salt will invalidate existing registration records, but I think that’s appropriate and good to do.

I think that's okay. It might impact the UX, but this is consistent with the same salt key rotation behavior. Maybe we can just add a brief note in the docblock of _wp_generate_signup_key() about it

[dmsnell]

@csmall2 since the patch has diverged from what you have been maintaining, do you have thoughts you’d like to share on this HMAC-based approach rather than the signup_id?