Skip to content

Commit 1f2b4b0

Browse files
dcavinsdcavins
committed
Improve security of messages endpoint (pre-bp15 branch).
In messages endpoint, sanitize the incoming `user_id` parameter value to avoid user spoofing. Special thanks to trihedron who first reported this issue responsibly. Props emaralive, jjj, renato, trihedron, substitute99, j2k14a, g_r_i_n_n, pythonime, dizconnect (Sanjorn Keeratirungsan), eneednar19, bb-hunter (Mustafa Ahmed), yhalo (Yaohui Wang), Ngo Anh Duc, ekbreks, jeromewincek (Jerome Wincek), taylsec, ajaah-254, izumi_hyun, mickey_cyberkid (Michael Okyere), underdog_theori, duyytrann (Duy Tran), safe-us (Safe Us Team), miauuu, daupaul (Dau-Po Yu).
1 parent 2b204e8 commit 1f2b4b0

2 files changed

Lines changed: 185 additions & 44 deletions

File tree

‎includes/bp-messages/classes/class-bp-rest-messages-endpoint.php‎

Lines changed: 113 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -191,17 +191,10 @@ public function get_items_permissions_check( $request ) {
191191
);
192192

193193
if ( is_user_logged_in() ) {
194-
$user = bp_rest_get_user( $request->get_param( 'user_id' ) );
195-
196-
if ( ! $user instanceof WP_User ) {
197-
$retval = new WP_Error(
198-
'bp_rest_invalid_id',
199-
__( 'Invalid member ID.', 'buddypress' ),
200-
array(
201-
'status' => 404,
202-
)
203-
);
204-
} elseif ( (int) bp_loggedin_user_id() === $user->ID || bp_current_user_can( 'bp_moderate' ) ) {
194+
$user_id = $this->validate_requested_user_id( $request );
195+
if ( is_wp_error( $user_id ) ) {
196+
$retval = $user_id;
197+
} elseif ( (int) bp_loggedin_user_id() === $user_id || bp_current_user_can( 'bp_moderate' ) ) {
205198
$retval = true;
206199
} else {
207200
$retval = new WP_Error(
@@ -293,35 +286,58 @@ public function get_item( $request ) {
293286
* @return true|WP_Error
294287
*/
295288
public function get_item_permissions_check( $request ) {
296-
$error = new WP_Error(
289+
$retval = new WP_Error(
297290
'bp_rest_authorization_required',
298-
__( 'Sorry, you are not allowed to see this thread.', 'buddypress' ),
291+
__( 'Sorry, you are not allowed to perform this action.', 'buddypress' ),
299292
array(
300-
'status' => rest_authorization_required_code(),
293+
'status' => rest_authorization_required_code()
301294
)
302295
);
303296

304-
$retval = $error;
305-
$user_id = bp_loggedin_user_id();
306-
if ( ! empty( $request->get_param( 'user_id' ) ) ) {
307-
$user_id = $request->get_param( 'user_id' );
308-
}
309-
310-
$id = $request->get_param( 'id' );
311-
297+
// Must be logged in.
312298
if ( is_user_logged_in() ) {
313-
$thread = BP_Messages_Thread::is_valid( $id );
314-
315-
if ( empty( $thread ) ) {
316-
$retval = new WP_Error(
317-
'bp_rest_invalid_id',
318-
__( 'Sorry, this thread does not exist.', 'buddypress' ),
319-
array(
320-
'status' => 404,
321-
)
322-
);
323-
} elseif ( bp_current_user_can( 'bp_moderate' ) || messages_check_thread_access( $id, $user_id ) ) {
324-
$retval = true;
299+
$thread_id = $request->get_param( 'id' );
300+
301+
// Thread ID must be requested.
302+
if ( ! empty( $thread_id ) ) {
303+
304+
// Get validity of thread.
305+
$thread_valid = messages_is_valid_thread( $thread_id );
306+
307+
// Thread not valid.
308+
if ( empty( $thread_valid ) ) {
309+
$retval = new WP_Error(
310+
'bp_rest_invalid_id',
311+
__( 'Sorry, this thread does not exist.', 'buddypress' ),
312+
array(
313+
'status' => 404,
314+
)
315+
);
316+
317+
// Thread is valid.
318+
} else {
319+
320+
// User ID.
321+
$user_id = $this->validate_requested_user_id( $request );
322+
323+
// Thread participant.
324+
if ( ! is_wp_error( $user_id ) ) {
325+
$participant = (bool) messages_check_thread_access( $thread_id, $user_id );
326+
}
327+
328+
// Invalid user.
329+
if ( is_wp_error( $user_id ) ) {
330+
$retval = $user_id;
331+
332+
// Moderators can access threads with valid thread participant.
333+
} elseif ( bp_current_user_can( 'bp_moderate' ) && $participant ) {
334+
$retval = true;
335+
336+
// Valid user must be thread participant.
337+
} elseif ( $participant ) {
338+
$retval = true;
339+
}
340+
}
325341
}
326342
}
327343

@@ -456,10 +472,10 @@ public function update_item( $request ) {
456472
// Setting context.
457473
$request->set_param( 'context', 'edit' );
458474

459-
// Updated user id.
460-
$updated_user_id = bp_loggedin_user_id();
461-
if ( ! empty( $request->get_param( 'user_id' ) ) ) {
462-
$updated_user_id = $request->get_param( 'user_id' );
475+
// User ID.
476+
$updated_user_id = $this->validate_requested_user_id( $request );
477+
if ( is_wp_error( $updated_user_id ) ) {
478+
return $updated_user_id;
463479
}
464480

465481
// Get the thread.
@@ -492,6 +508,18 @@ public function update_item( $request ) {
492508
}
493509

494510
$updated_message = wp_list_filter( $thread->messages, array( 'id' => $message_id ) );
511+
512+
// Invalid message ID.
513+
if ( empty( $updated_message ) ) {
514+
return new WP_Error(
515+
'bp_rest_invalid_id',
516+
__( 'Sorry, this message does not exist.', 'buddypress' ),
517+
array(
518+
'status' => 404,
519+
)
520+
);
521+
}
522+
495523
$updated_message = reset( $updated_message );
496524

497525
/**
@@ -701,9 +729,10 @@ public function delete_item( $request ) {
701729
// Setting context.
702730
$request->set_param( 'context', 'edit' );
703731

704-
$user_id = bp_loggedin_user_id();
705-
if ( ! empty( $request->get_param( 'user_id' ) ) ) {
706-
$user_id = $request->get_param( 'user_id' );
732+
// User ID.
733+
$user_id = $this->validate_requested_user_id( $request );
734+
if ( is_wp_error( $user_id ) ) {
735+
return $user_id;
707736
}
708737

709738
// Get the thread before it's deleted.
@@ -1068,7 +1097,7 @@ public function get_thread_object( $thread_id, $user_id = 0 ) {
10681097
}
10691098

10701099
// Validate the thread ID.
1071-
$thread_id = BP_Messages_Thread::is_valid( $thread_id );
1100+
$thread_id = messages_is_valid_thread( $thread_id );
10721101

10731102
if ( false === (bool) $thread_id ) {
10741103
return '';
@@ -1520,4 +1549,46 @@ public function get_collection_params() {
15201549
*/
15211550
return apply_filters( 'bp_rest_messages_collection_params', $params );
15221551
}
1552+
1553+
/**
1554+
* Validate the requested user ID.
1555+
*
1556+
* Falls back to the logged in user ID if the requested user ID is empty or
1557+
* if the current user doesn't have moderation capabilities.
1558+
*
1559+
* Returns a WP_Error if the requested user ID is invalid.
1560+
*
1561+
* @since 14.5.0
1562+
*
1563+
* @param WP_REST_Request $request Full details about the request.
1564+
* @return WP_Error|int
1565+
*/
1566+
private function validate_requested_user_id( $request ) {
1567+
1568+
// Get the user ID from the request.
1569+
$retval = $request->get_param( 'user_id' );
1570+
1571+
// Maybe fallback/override user ID to logged in ID.
1572+
if ( empty( $retval ) || ! bp_current_user_can( 'bp_moderate' ) ) {
1573+
$retval = bp_loggedin_user_id();
1574+
$request->set_param( 'user_id', $retval );
1575+
}
1576+
1577+
// Get the user object.
1578+
$user = bp_rest_get_user( $retval );
1579+
1580+
// Requested user not valid.
1581+
if ( ! $user instanceof WP_User ) {
1582+
$retval = new WP_Error(
1583+
'bp_rest_invalid_id',
1584+
__( 'Invalid member ID.', 'buddypress' ),
1585+
array(
1586+
'status' => 404,
1587+
)
1588+
);
1589+
}
1590+
1591+
// Return ID or error.
1592+
return $retval;
1593+
}
15231594
}

‎tests/testcases/messages/test-controller.php‎

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,30 @@ public function test_get_item_admin_access() {
267267
$this->assertTrue( isset( $message['subject']['rendered'] ) );
268268
}
269269

270+
/**
271+
* @group get_item
272+
*/
273+
public function test_get_item_prevent_counterfeit_user_id() {
274+
$u1 = static::factory()->user->create();
275+
$u2 = static::factory()->user->create();
276+
$u3 = static::factory()->user->create();
277+
$m = $this->bp::factory()->message->create_and_get( array(
278+
'sender_id' => $u1,
279+
'recipients' => array( $u2 ),
280+
'subject' => 'Foo',
281+
) );
282+
283+
$this->bp::set_current_user( $u3 );
284+
285+
$request = new WP_REST_Request( 'GET', $this->endpoint_url . '/' . $m->thread_id );
286+
$request->set_param( 'context', 'view' );
287+
$request->set_param( 'user_id', $u2 );
288+
$response = $this->server->dispatch( $request );
289+
290+
$this->assertErrorResponse( 'bp_rest_authorization_required', $response, rest_authorization_required_code() );
291+
$this->assertSame( 403, $response->get_status() );
292+
}
293+
270294
/**
271295
* @group get_item
272296
*/
@@ -589,6 +613,29 @@ public function test_update_item_with_user_with_no_access() {
589613
);
590614
}
591615

616+
/**
617+
* @group update_item
618+
*/
619+
public function test_update_item_prevent_counterfeit_user_id() {
620+
$u1 = static::factory()->user->create();
621+
$u2 = static::factory()->user->create();
622+
$u3 = static::factory()->user->create();
623+
$m = $this->bp::factory()->message->create_and_get( array(
624+
'sender_id' => $u1,
625+
'recipients' => array( $u2 ),
626+
'subject' => 'Foo',
627+
) );
628+
629+
$this->bp::set_current_user( $u3 );
630+
631+
$request = new WP_REST_Request( 'PUT', sprintf( $this->endpoint_url . '/%d', $m->thread_id ) );
632+
$request->set_param( 'user_id', $u2 );
633+
$response = $this->server->dispatch( $request );
634+
635+
$this->assertErrorResponse( 'bp_rest_authorization_required', $response, rest_authorization_required_code() );
636+
$this->assertSame( 403, $response->get_status() );
637+
}
638+
592639
/**
593640
* @group delete_item
594641
*/
@@ -674,13 +721,13 @@ public function test_delete_item_with_user_with_no_access() {
674721
public function test_delete_item_user_is_not_logged_in() {
675722
$u1 = static::factory()->user->create();
676723
$u2 = static::factory()->user->create();
677-
$m = $this->bp::factory()->message->create( array(
724+
$m = $this->bp::factory()->message->create_and_get( array(
678725
'sender_id' => $u1,
679726
'recipients' => array( $u2 ),
680727
'subject' => 'Foo',
681728
) );
682729

683-
$request = new WP_REST_Request( 'DELETE', $this->endpoint_url . '/' . $m );
730+
$request = new WP_REST_Request( 'DELETE', $this->endpoint_url . '/' . $m->thread_id );
684731

685732
$this->assertErrorResponse(
686733
'bp_rest_authorization_required',
@@ -689,6 +736,29 @@ public function test_delete_item_user_is_not_logged_in() {
689736
);
690737
}
691738

739+
/**
740+
* @group delete_item
741+
*/
742+
public function test_delete_item_prevent_counterfeit_user_id() {
743+
$u1 = static::factory()->user->create();
744+
$u2 = static::factory()->user->create();
745+
$u3 = static::factory()->user->create();
746+
$m = $this->bp::factory()->message->create_and_get( array(
747+
'sender_id' => $u1,
748+
'recipients' => array( $u2 ),
749+
'subject' => 'Foo',
750+
) );
751+
752+
$this->bp::set_current_user( $u3 );
753+
754+
$request = new WP_REST_Request( 'DELETE', $this->endpoint_url . '/' . $m->thread_id );
755+
$request->set_param( 'user_id', $u2 );
756+
$response = $this->server->dispatch( $request );
757+
758+
$this->assertErrorResponse( 'bp_rest_authorization_required', $response, rest_authorization_required_code() );
759+
$this->assertSame( 403, $response->get_status() );
760+
}
761+
692762
/**
693763
* @group starred
694764
*/

0 commit comments

Comments
 (0)