Messages viewable to any logged out visitor

[CodeMonkeyBanana]

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?

[sbrajesh]

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]

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

[henry.wright]

@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

[CodeMonkeyBanana]

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.

[sbrajesh]

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

[johnjamesjacoby]

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

[CodeMonkeyBanana]

@johnjamesjacoby Where should I report further security issues?

[netweb]

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 :)

[hnla]

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.

[boonebgorges]

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.

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

[boonebgorges]

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 :)

[johnjamesjacoby]

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

[johnjamesjacoby]

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)

[johnjamesjacoby]

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)