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(),