Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6504 closed defect (bug) (fixed)

Messages viewable to any logged out visitor

Reported by: CodeMonkeyBanana Owned by: johnjamesjacoby
Milestone: 2.3.2 Priority: normal
Severity: blocker Version:
Component: Messages Keywords: has-patch 2nd-opinion
Cc: brajesh@…, mikeymike81@…, stephen@…, espellcaste@…

Description

I have noticed when implementing ajax in theme that it is possible to view anyones messages.

If you navigate to any users profile page and then enter this into javascript console it will show you the messages for the user you are viewing:

jq.post( ajaxurl, { action: 'messages_filter' })
  .done(function( data ) {
    document.write(data);
  });

I tested on my live site and it worked. I think some extra security check is needed.

PS. I chose "I am not reporting a security issue" because this isn't a security issue with wordpress, it is buddypress specific. Was that wrong?

Attachments (2)

6504.diff (3.3 KB) - added by sbrajesh 5 years ago.
Do not attach actions that requires logged in privilege to wp_ajax_nopriv hook
6504.01.patch (3.0 KB) - added by johnjamesjacoby 5 years ago.

Download all attachments as: .zip

Change History (21)

#1 @sbrajesh
5 years ago

  • Cc brajesh@… added
  • Keywords has-patch added

Confirmed.
The reason it is happening, we are attaching various ajax actions to wp_ajax_nopriv_ actions.
In case of messages, When user is not logged in, it lists all messages without using user_id in the query.

A simple solution is to break down $actions array into privileged actions and non privileged actions. We only attach privileged actions to wp_ajax and not to wp_ajax_noprim

I have attached an initial patch, that fixes it for bp-legacy. need to check if it is happening in bp-default too.


@sbrajesh
5 years ago

Do not attach actions that requires logged in privilege to wp_ajax_nopriv hook

#2 @henry.wright
5 years ago

@sbrajesh you should mention this on Slack. Considering the severity of the issue, I'm thinking the guys might want to hide this ticket until the fix is released

Last edited 5 years ago by henry.wright (previous) (diff)

This ticket was mentioned in Slack in #buddypress by sbrajesh. View the logs.


5 years ago

#4 @CodeMonkeyBanana
5 years ago

I think that removing the nopriv might not be enough as a user can log in to their own account and then manipulate post request to still view stuff they shouldn't. I don't have proof of that yet, I will put code up tomorrow if needed.

#5 @CodeMonkeyBanana
5 years ago

  • Cc mikeymike81@… added

#6 follow-up: @sbrajesh
5 years ago

To be honest, There is a loophole. Won't be posting anything here though.

Last edited 5 years ago by sbrajesh (previous) (diff)

#7 @johnjamesjacoby
5 years ago

In the future, let's treat issues like this as security issues. For this one, I'm fine hashing it out here until/unless someone feels much more strongly than I do about it.

A few notes while mobile:

  • I'll look into this ASAP
  • The AJAX and JS from bp-default/legacy is a known pain-point, and needs more scrutinization similar to this
  • We will probably want to quick-fix this for 2.3.2, and roll something more comprehensive into 2.4
  • Something like roles & caps would help us here
  • I'll reply back in the next 24 hours with details and progress

#8 follow-up: @CodeMonkeyBanana
5 years ago

@johnjamesjacoby Where should I report further security issues?

#9 in reply to: ↑ 8 @netweb
5 years ago

  • Cc stephen@… added

Replying to CodeMonkeyBanana:

@johnjamesjacoby Where should I report further security issues?

Send an email to security@wordpress.org and the team will make sure they find the appropriate people :)

Last edited 5 years ago by netweb (previous) (diff)

#10 @hnla
5 years ago

PS. I chose "I am not reporting a security issue" because this isn't a security issue with wordpress, it is buddypress specific. Was that wrong?

While this might not be a security issue it is a privacy issue and many sites and communities take this quite seriously, wanting sites and communities that are private for members only and this sort of issue knocks their confidence. It probably shouldn't have been raised in this public manner but addressed directly to one of the lead developers, JJJ initially, and slack provides an easy DM approach to alerting to a potential problem, with obviously security@wordpress in addition or as fallback.

#11 in reply to: ↑ 6 @boonebgorges
5 years ago

  • Milestone changed from Awaiting Review to 2.3.2
  • Severity changed from major to blocker

In the future, let's treat issues like this as security issues.

+1. In the future, please send reports of this nature to security@….

Replying to sbrajesh:

To be honest, There is a loophole. Won't be posting anything here though.

Confirmed. There is a way to spoof the user ID even when logged in, though it's very much not obvious. We need a couple different kinds of hardening here.

#12 follow-up: @sbrajesh
5 years ago

There is a simple solution to the user id spoofing.
Unless we add roles/caps in future who can see other's message, w can simply reset user_id in bp_has_message_threads after the parsing of the arguments. Except if super admin, It should always reset to get_current_user_id() for now.

That will avoid any future leak there.

#13 in reply to: ↑ 12 @boonebgorges
5 years ago

Replying to sbrajesh:

There is a simple solution to the user id spoofing.
Unless we add roles/caps in future who can see other's message, w can simply reset user_id in bp_has_message_threads after the parsing of the arguments. Except if super admin, It should always reset to get_current_user_id() for now.

That will avoid any future leak there.

Yes, this is probably the most secure thing to do, though I'm not a big fan of doing these kinds of blocks at the level of the template function. I'm going to ping you on Slack to chat more about it :)

#14 @espellcaste
5 years ago

  • Cc espellcaste@… added

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


5 years ago

This ticket was mentioned in Slack in #buddypress by jjj. View the logs.


5 years ago

#17 @johnjamesjacoby
5 years ago

  • Keywords 2nd-opinion added

6504.01.patch proposes the following:

  • Introduces bp_messages_restrict_current_user() as a filter on the bp_after_has_message_threads_parse_args filter.
  • If user is not logged in, we wipe out the $args array completely. This makes the query arguments use their fallbacks from BP_Messages_Thread::get_current_threads_for_user() which are:
    array(
    	'user_id'      => false,
    	'box'          => 'inbox',
    	'type'         => 'all',
    	'limit'        => null,
    	'page'         => null,
    	'search_terms' => '',
    	'meta_query'   => array()
    )
    
  • Sets smarter defaults for $user_id_sql and $sender_sql so user ID 0 is the default user being queried for. This means if no user ID is passed 0 is assumed, which wouldn't have any results anyways.
  • I also cleaned up single & double quote usage in to better depict which $sql query parts have nested PHP variables in them, and which ones are literals or ran through $wpdb->prepare().

I like this approach because it sets up a completely pluggable paradigm of intelligent default enforcements of content restrictions. Rather than build assumptions into existing functions and methods that could be being used in an infinite number of ways, we can intercept argument combinations we know to be unintended or potentially malicious, and black-list them in a way that can be unhooked by savvy developers.

Last edited 5 years ago by johnjamesjacoby (previous) (diff)

#18 @johnjamesjacoby
5 years ago

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

In 9948:

Messages: Introduce filter to enforce private message thread query boundaries.

This change ensures that all queries for private messages will always return anticipated results, even when certain malformed values are passed in. It specifically hardens the user ID argument to prevent accidental overriding.

Fixes #6504. Props r-a-y. (trunk, for 2.4.0)

#19 @johnjamesjacoby
5 years ago

In 9949:

Messages: Introduce filter to enforce private message thread query boundaries.

This change ensures that all queries for private messages will always return anticipated results, even when certain malformed values are passed in. It specifically hardens the user ID argument to prevent accidental overriding.

Fixes #6504. Props r-a-y. (2.3 branch, for 2.3.2)

Note: See TracTickets for help on using tickets.