From e6f6fb08615c28bdb264abd384340d35b67a4709 Mon Sep 17 00:00:00 2001 From: Olivia Lee Date: Tue, 30 Dec 2025 02:49:38 -0800 Subject: [PATCH] validate membership events returned by remote servers This fixes a vulnerability where an attacker with a a malicious remote server and a user on the local server can trick the local server into signing arbitrary events. The attacker issue a remote leave as the local user to a room on the malicious server. Without any validation of the make_leave response, the local server would sign the attacker-controlled event and pass it back to the malicious server with send_leave. The join endpoints is also fixed in this commit, but is less useful for exploitation because the local server replaces the "content" field returned by the remote server. Remote invites are unaffected because we already check that the event returned from /invite has the same event ID as the event passed to it. --- book/changelog.md | 4 ++ src/api/client_server/membership.rs | 100 +++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/book/changelog.md b/book/changelog.md index 2fcd48d6..c01b7c48 100644 --- a/book/changelog.md +++ b/book/changelog.md @@ -59,6 +59,10 @@ This will be the first release of Grapevine since it was forked from Conduit 8. Fix vulnerability that allows a malicious server to trick a grapevine server into signing arbitrary forged events via the send_invite endpoint. ([!205](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/205)) +9. Fix vulnerability that allows a malicious user on a grapevine server to use a + malicious remote server to trick the local server into signing arbitrary + events via remote leave. + ([!206](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/206)) ### Removed diff --git a/src/api/client_server/membership.rs b/src/api/client_server/membership.rs index e0afec6b..8e4d20f3 100644 --- a/src/api/client_server/membership.rs +++ b/src/api/client_server/membership.rs @@ -23,7 +23,7 @@ use ruma::{ join_rules::{AllowRule, JoinRule, RoomJoinRulesEventContent}, member::{MembershipState, RoomMemberEventContent}, }, - StateEventType, TimelineEventType, + StateEventType, StaticEventContent, TimelineEventType, }, room_version_rules::RoomVersionRules, state_res, CanonicalJsonObject, CanonicalJsonValue, EventId, @@ -502,6 +502,82 @@ pub(crate) async fn joined_members_route( })) } +/// Validates that an event returned from a remote server by `/make_*` +/// actually is a membership event with the expected fields. +/// +/// Without checking this, the remote server could use the remote membership +/// mechanism to trick our server into signing arbitrary malicious events. +pub(crate) fn validate_remote_member_event_stub( + membership: &MembershipState, + user_id: &UserId, + room_id: &RoomId, + event_stub: &CanonicalJsonObject, +) -> Result<()> { + let Some(event_type) = event_stub.get("type") else { + return Err(Error::BadServerResponse( + "Remote server returned member event with missing type field", + )); + }; + if event_type != &RoomMemberEventContent::TYPE { + return Err(Error::BadServerResponse( + "Remote server returned member event with invalid event type", + )); + } + + let Some(sender) = event_stub.get("sender") else { + return Err(Error::BadServerResponse( + "Remote server returned member event with missing sender field", + )); + }; + if sender != &user_id.as_str() { + return Err(Error::BadServerResponse( + "Remote server returned member event with incorrect sender", + )); + } + + let Some(state_key) = event_stub.get("state_key") else { + return Err(Error::BadServerResponse( + "Remote server returned member event with missing state_key field", + )); + }; + if state_key != &user_id.as_str() { + return Err(Error::BadServerResponse( + "Remote server returned member event with incorrect state_key", + )); + } + + let Some(event_room_id) = event_stub.get("room_id") else { + return Err(Error::BadServerResponse( + "Remote server returned member event with missing room_id field", + )); + }; + if event_room_id != &room_id.as_str() { + return Err(Error::BadServerResponse( + "Remote server returned member event with incorrect room_id", + )); + } + + let Some(content) = + event_stub.get("content").and_then(|content| content.as_object()) + else { + return Err(Error::BadServerResponse( + "Remote server returned member event with missing content field", + )); + }; + let Some(event_membership) = content.get("membership") else { + return Err(Error::BadServerResponse( + "Remote server returned member event with missing membership field", + )); + }; + if event_membership != &membership.as_str() { + return Err(Error::BadServerResponse( + "Remote server returned member event with incorrect membership", + )); + } + + Ok(()) +} + #[allow(clippy::too_many_lines)] #[tracing::instrument(skip(reason, _third_party_signed))] async fn join_room_by_id_helper( @@ -675,6 +751,14 @@ async fn join_room_by_id_helper( "Invalid make_join event json received from server.", ) })?; + + validate_remote_member_event_stub( + &MembershipState::Join, + sender_user, + room_id, + &join_event_stub, + )?; + let join_authorized_via_users_server = join_event_stub .get("content") .map(|s| { @@ -832,6 +916,13 @@ async fn join_room_by_id_helper( ) })?; + validate_remote_member_event_stub( + &MembershipState::Join, + sender_user, + room_id, + &join_event_stub, + )?; + let join_authorized_via_users_server = join_event_stub .get("content") .map(|s| { @@ -1657,6 +1748,13 @@ async fn remote_leave_room(user_id: &UserId, room_id: &RoomId) -> Result<()> { ) })?; + validate_remote_member_event_stub( + &MembershipState::Leave, + user_id, + room_id, + &leave_event_stub, + )?; + // TODO: Is origin needed? leave_event_stub.insert( "origin".to_owned(),