Skip to:
Content

BuddyPress.org

Opened 7 years ago

Closed 7 years ago

#7401 closed defect (bug) (fixed)

Wrong user data leak with external object cache

Reported by: m_uysl's profile m_uysl Owned by: boonebgorges's profile boonebgorges
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.0
Component: Extended Profile Keywords: has-patch commit
Cc:

Description

Steps to reproduce

Create a new field group and add field(s). Ex: "Facebook"
Create new user and leave "Facebook" field empty.
Fill your "Facebook" field.

Create new php file and try to fetch non-exist data.

<?php
require_once 'wp-load.php';
$maybe_has_facebook = BP_XProfile_ProfileData::get_value_byid( xprofile_get_field_id_from_name('Facebook'), 2 );

Then try to edit that user, you will see your field data in there.

This problem caused by missing $field_id when creating empty cache https://buddypress.trac.wordpress.org/browser/tags/2.7.3/src/bp-xprofile/classes/class-bp-xprofile-profiledata.php#L492 and
https://buddypress.trac.wordpress.org/browser/tags/2.7.3/src/bp-xprofile/classes/class-bp-xprofile-group.php#L450 fetching wrong data in there.

Tested with memcached/redis on trunk.

Attachments (3)

7401.diff (520 bytes) - added by m_uysl 7 years ago.
7401.2.diff (3.0 KB) - added by boonebgorges 7 years ago.
7401.3.diff (3.2 KB) - added by m_uysl 7 years ago.

Download all attachments as: .zip

Change History (9)

@m_uysl
7 years ago

#1 @r-a-y
7 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 2.8
  • Version set to 2.0

Nice catch, @m_uysl!

Originally committed in r7820.

@boonebgorges
7 years ago

#2 @boonebgorges
7 years ago

  • Keywords 2nd-opinion added

@m_uysl - Thanks a lot for the ticket and the patch! It's helped me to understand the issue a bit better. It's now clear to me why the cache corruption happens in the case where an admin is editing another user's profile. The mechanics of the more general case (as described in #6091) are still not clear to me, but I'm fairly sure they're related.

It seems to me that the underlying architectural problem here is that cached data for another user is fetched in the first place. When you call BP_XProfile_Group::get(), the field objects are populated using xprofile_get_field(). This instantiates a BP_XProfile_Field object, with the default $user_id and $get_data params. As such, these field objects contain data for the *logged-in* user, even if you are fetching the fields/groups of another user.

There are a bunch of ways to address this. The one that requires the least amount of code is 7401.2.diff. Briefly: when fetching the field objects in BP_XProfile_Group::get(), be sure to request them in a way that doesn't fetch any user data at all. (BP_XProfile_Group::get() gets the needed user data separately.) This change requires passing around the $user_id and $get_data params, which I don't really like, but it does the trick.

@m_uysl and @r-a-y What do you think of this more general change? As far as I can see, it will ensure that this kind of cross-pollination is impossible in the future.

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


7 years ago

@m_uysl
7 years ago

#4 @m_uysl
7 years ago

The mechanics of the more general case (as described in #6091) are still not clear to me, but I'm fairly sure they're related.

I think so.

This change requires passing around the $user_id and $get_data params, which I don't really like, but it does the trick.

I hate extra parameters as well but it's less terrifying than fetching wrong data :)

What do you think of this more general change?

It certainly makes sense BP_XProfile_Group::get() should always fetch data belongs to correct user.

@boonebgorges I updated your patch by adding $user_id to fake cache data. (just in case)

#5 @r-a-y
7 years ago

  • Keywords commit added; 2nd-opinion removed

Looks good to me!

#6 @boonebgorges
7 years ago

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

In 11316:

XProfile: More consistent cache behavior when fetching user data.

  • Inside of a profile group loop (BP_XProfile_Group::get()), don't fetch user data when pulling up BP_XProfile_Field objects. In the absence of finer-grained information about users, fetching a field object grabs the data associated with the logged-in user. But in many cases, the logged-in user is irrelevant to the fields being looped over, so there's no benefit to pulling up this data. (When necessary - fetch_data - the data is queried separately, later in the get() method.)
  • When caching database misses for a data query (because the specifed user doesn't have anything filled in for the given field), store the field_id and user_id properties on the cached object. This ensures that values are properly associated with their fields when being displayed.

These changes resolve an issue where cached data for the logged-in user
can be shown erroneously on another user's profile, when the other user
doesn't have a value for a given field.

Props m_uysl, r-a-y.
See #6091. Fixes #7401.

Note: See TracTickets for help on using tickets.