#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)
Change History (21)
#2
@
9 years ago
@sbrajesh you should want to 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
This ticket was mentioned in Slack in #buddypress by sbrajesh. View the logs.
9 years ago
#4
@
9 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.
#6
follow-up:
↓ 11
@
9 years ago
To be honest, There is a loophole. Won't be posting anything here though.
#7
@
9 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
#9
in reply to:
↑ 8
@
9 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 :)
#10
@
9 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
@
9 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:
↓ 13
@
9 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
@
9 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 :)
This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.
9 years ago
This ticket was mentioned in Slack in #buddypress by jjj. View the logs.
9 years ago
#17
@
9 years ago
- Keywords 2nd-opinion added
6504.01.patch proposes the following:
- Introduces
bp_messages_restrict_current_user()
as a filter on thebp_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 fromBP_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.
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.