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?