Make WordPress Core

Opened 3 years ago

Last modified 4 months ago

#53784 assigned defect (bug)

Limiting user enumeration through the REST API

Reported by: ehtis's profile ehtis Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests has-screenshots dev-reviewed changes-requested
Focuses: rest-api, privacy Cc:

Description

Via endpoints like /wp/v2/comments?search=$term, it's currently possible to perform email discovery through brute force. In this case, emails of commenters.

Not exactly the same, but previous discussion (for login forms) is at: https://core.trac.wordpress.org/ticket/9568#comment:82

After an H1 report and some discussion within the security team, it was decided we should probably "fix" this and have more public discussion.

Authorized users should be able to search comment data that's non-public.

Report by dawidpieper.

Change History (19)

#1 @dd32
3 years ago

  • Owner dd32 deleted

Just removing my assignment from the ticket, as I don't have any work underway on this and don't want to risk someone not touching it because of the assignment :)

#2 @waike
3 years ago

Related issue: Using the search parameter on /wp-json/wp/v2/users, it's possible to extract user email addresses one character at a time, because the search parameter is matched against users' email fields.

https://wordpress.org/wp-json/wp/v2/users?search=m@ottodestruct.com     -> no results
https://wordpress.org/wp-json/wp/v2/users?search=n@ottodestruct.com     -> no results
https://wordpress.org/wp-json/wp/v2/users?search=o@ottodestruct.com     -> 1 result
(...)
https://wordpress.org/wp-json/wp/v2/users?search=mtto@ottodestruct.com  -> no results
https://wordpress.org/wp-json/wp/v2/users?search=ntto@ottodestruct.com  -> no results
https://wordpress.org/wp-json/wp/v2/users?search=otto@ottodestruct.com  -> 1 result

I wrote a quick proof-of-concept tool to automate this process.

Link: https://github.com/JeppW/WordPress-Email-Extractor

This ticket was mentioned in PR #1660 on WordPress/wordpress-develop by fictiont.


3 years ago
#3

  • Keywords has-patch added; needs-patch removed

There is a security problem in the current WP REST API.

Using search parameter available at /wp-json/wp/v2/users and /wp-json/wp/v2/comments it is possible to brute force users emails and comment authors emails as shown on example below provided by @JeppW:

https://wordpress.org/wp-json/wp/v2/users?search=m@ottodestruct.com -> no results
https://wordpress.org/wp-json/wp/v2/users?search=n@ottodestruct.com -> no results
https://wordpress.org/wp-json/wp/v2/users?search=o@ottodestruct.com -> 1 result
(...)
https://wordpress.org/wp-json/wp/v2/users?search=mtto@ottodestruct.com -> no results
https://wordpress.org/wp-json/wp/v2/users?search=ntto@ottodestruct.com -> no results
https://wordpress.org/wp-json/wp/v2/users?search=otto@ottodestruct.com -> 1 result

User email and comment author email aren't public fields, they aren't returned in the users list and comments list.
This patch fixes problem by removing fields user_email and comment_author_email from search logic at related endpoints:
../v2/users?search=
../v2/comments?search=

Here is an example from a local machine of how the fix works:

  • before applying the fix:

https://i0.wp.com/user-images.githubusercontent.com/4171817/132241360-dd98d6c6-3bf2-4184-a845-8c252e1ddc26.png

  • after applying the fix:

https://i0.wp.com/user-images.githubusercontent.com/4171817/132241373-beca1ec9-6353-4b2f-ab76-11569f96991a.png

Props ehtis, waike.
Fixes https://core.trac.wordpress.org/ticket/53784.

#4 @fictiont
3 years ago

  • Component changed from Users to REST API
  • Keywords has-unit-tests has-screenshots added

#5 @fictiont
3 years ago

  • Focuses privacy added

#6 follow-up: @audrasjb
3 years ago

Thank for working on this @fictiont !
The PR looks good to me, I'm only wondering if the long comment above the change is really needed.
I'd prefer to add a hook to filter $allowed_columns and to use it to document properly the default behavior.

#7 in reply to: ↑ 6 @fictiont
3 years ago

Replying to audrasjb:

Thank for working on this @fictiont !
The PR looks good to me, I'm only wondering if the long comment above the change is really needed.
I'd prefer to add a hook to filter $allowed_columns and to use it to document properly the default behavior.

Thank you @audrasjb for the suggestion! I agree that would be a better approach. However, I'm not sure I could make an update to the patch before the meeting.

#8 @audrasjb
3 years ago

No worries, that's not a blocker at all :)

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


3 years ago

#10 @audrasjb
3 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 5.9

Marking for 5.9 consideration

#11 @TimothyBlynJacobs
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

I've left some feedback on the PR. I think this still needs some more work before it is ready. We may also want to commit this in a minor release so it can be backported. Cc: @peterwilsoncc.

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


2 years ago

#13 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to 6.0

5.9 Beta 1 is tomorrow. As there's still work to do in the PR, moving it to 6.0. However, if it should land in a minor for backporting, please move it to that minor milestone.

peterwilsoncc commented on PR #1660:


2 years ago
#14

I've merged in and retargeted this to trunk (new default branch name).

TimothyBJacobs commented on PR #1660:


2 years ago
#15

So I think there are valid multiple points of view here, but from my perspective we should move the current_user_can check outside of WP_Comments_Query and into the Controller so it functions the same way as WP_User_Query.

I think the Query class should be responsible for providing the data the programmer asked for, and for the individual interfaces to handle access controls how they deem appropriate. Otherwise I can't create an interface for let's say comment moderation that has those more advanced order by support unless I also grant list_users which may be undesirable.

Our User, Term and Comment Query APIs are currently unified in not performing capability checks themselves. This is of course in contrast to WP_Query itself which is why I think my POV is debatable. However, I think WP_Query is an older design and itself mixes concerns by also being the main query handler. If possible, we should try and maintain the design pattern that the new query classes currently follow.

costdev commented on PR #1660:


2 years ago
#16

I'm inclined to agree with @TimothyBJacobs.

The interface (i.e. the controller) should determine how it will respond to a request. That comes with the responsibility to ensure that it doesn't expose information that it shouldn't. One interface may allow access to information in some or all circumstances, and another may lock it down entirely.

The purpose of WP_Comment_Query is to provide a foundation for querying comments. WP_Comment_Query cannot determine the purpose of a particular interface, the use cases that the interface is intended to cover, and cannot make this a blanket decision for all interfaces that query comments.

For all intents and purposes, if ( current_user_can( 'list_users' ) ) {} is akin to a can_include_author_email() method. Not all interfaces will need or use this, thus it could be argued that adding the capability checks to WP_Comment_Query would verge on violating the Interface Segregation Principle.

---

An outstanding use case: If I wanted to have a frontend interface for comment moderation that utilized the REST API and allowed sorting by the comment author's email address, I would still need to grant the list_users capability, which as @TimothyBJacobs highlighted, may not be desirable.

This use case goes outside of this ticket IMO, and may mean adding new list_user_email and list_user_ip capabilities - noting that desiring access to one may not mean desiring access to the other.

peterwilsoncc commented on PR #1660:


2 years ago
#17

Thanks both, let's go with that approach then.

It's unlikely I'll have any bandwidth to push any updates to the PR prior to the WP beta next week.

#18 @costdev
23 months ago

  • Keywords changes-requested added
  • Milestone changed from 6.0 to Future Release

With 6.0 RC1 tomorrow, this ticket still requires more work. Moving to the Future Release milestone.

JeppW commented on PR #1660:


4 months ago
#19

Looks like someone else reported this issue and it was patched in WordPress 6.3.2.

Reference: wpscan.com/blog/email-leak-oracle-vulnerability-addressed-in-wordpress-6-3-2

Note: See TracTickets for help on using tickets.