Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#38474 reviewing enhancement

wp_signups.activation_key stores activation keys in plain text

Reported by: tomdxw's profile tomdxw Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 4.6.1
Component: Security Keywords: has-patch needs-testing
Focuses: multisite Cc:

Description

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.

Attachments (6)

38474.patch (9.4 KB) - added by bor0 7 years ago.
Screen Shot 2017-01-15 at 02.04.56 AM.png (36.2 KB) - added by bor0 7 years ago.
Screen Shot 2017-01-15 at 02.06.48 AM.png (405.1 KB) - added by bor0 7 years ago.
38474.2.patch (10.8 KB) - added by bor0 6 years ago.
38474.3.patch (11.5 KB) - added by bor0 6 years ago.
Add timestamp expire check
38474.4.patch (11.8 KB) - added by bor0 6 years ago.
Bugfixes after local testings and code reorganization

Download all attachments as: .zip

Change History (25)

#2 @tomdxw
7 years ago

  • Keywords 4.8-early needs-patch added

@bor0
7 years ago

#3 @bor0
7 years ago

  • Keywords has-patch added; needs-patch removed

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)

#4 @bor0
7 years ago

@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.

#5 @SergeyBiryukov
7 years ago

  • Focuses multisite added

#6 @tomdxw
7 years ago

  • Keywords 4.9-early added; 4.8-early removed

#7 @tomdxw
6 years ago

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

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


6 years ago

#9 @chriscct7
6 years ago

  • Keywords 4.9-early removed

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?

#10 @jeremyfelt
6 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to bor0
  • Status changed from new to assigned

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.

#11 @bor0
6 years ago

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

@bor0
6 years ago

#12 follow-ups: @bor0
6 years ago

  • Keywords has-patch added; needs-patch removed

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!

#13 in reply to: ↑ 12 ; follow-up: @SergeyBiryukov
6 years ago

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?

#14 in reply to: ↑ 12 @tomdxw
6 years ago

@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.

#15 in reply to: ↑ 13 @bor0
6 years ago

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
6 years ago

Add timestamp expire check

@bor0
6 years ago

Bugfixes after local testings and code reorganization

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 years ago

#17 @flixos90
6 years ago

  • Owner changed from bor0 to SergeyBiryukov
  • Status changed from assigned to reviewing

This one is ready for review.

#18 @peterwilsoncc
5 years ago

  • Milestone changed from 5.0 to 5.1

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

#19 @pento
5 years ago

  • Keywords needs-testing added
  • Milestone changed from 5.1 to Future Release
Note: See TracTickets for help on using tickets.