diff --git a/src/api/client/membership/join.rs b/src/api/client/membership/join.rs index cbb825068..60dbe8fa9 100644 --- a/src/api/client/membership/join.rs +++ b/src/api/client/membership/join.rs @@ -584,7 +584,7 @@ async fn join_room_by_id_helper_remote( return state; }, }; - if !pdu_fits(&mut value.clone()) { + if !pdu_fits(&value) { warn!( "dropping incoming PDU {event_id} in room {room_id} from room join because \ it exceeds 65535 bytes or is otherwise too large." diff --git a/src/service/rooms/event_handler/handle_incoming_pdu.rs b/src/service/rooms/event_handler/handle_incoming_pdu.rs index da142f356..549a3341c 100644 --- a/src/service/rooms/event_handler/handle_incoming_pdu.rs +++ b/src/service/rooms/event_handler/handle_incoming_pdu.rs @@ -127,12 +127,12 @@ pub async fn handle_incoming_pdu<'a>( if let Ok(pdu_id) = self.services.timeline.get_pdu_id(event_id).await { return Ok(Some(pdu_id)); } - if !pdu_fits(&mut value.clone()) { + if !pdu_fits(&value) { warn!( "dropping incoming PDU {event_id} in room {room_id} from {origin} because it \ exceeds 65535 bytes or is otherwise too large." ); - return Err!(Request(TooLarge("PDU is too large"))); + return Err!(Request(TooLarge("PDU {event_id} is too large"))); } trace!("processing incoming PDU from {origin} for room {room_id} with event id {event_id}"); diff --git a/src/service/rooms/event_handler/handle_outlier_pdu.rs b/src/service/rooms/event_handler/handle_outlier_pdu.rs index b39946eca..87bf363ee 100644 --- a/src/service/rooms/event_handler/handle_outlier_pdu.rs +++ b/src/service/rooms/event_handler/handle_outlier_pdu.rs @@ -1,8 +1,7 @@ use std::collections::{BTreeMap, HashMap, hash_map}; use conduwuit::{ - Err, Event, PduEvent, Result, debug, debug_info, debug_warn, err, implement, state_res, - trace, warn, + Err, Event, PduEvent, Result, debug, debug_info, debug_warn, err, implement, state_res, trace, }; use futures::future::ready; use ruma::{ @@ -11,7 +10,6 @@ use ruma::{ }; use super::{check_room_id, get_room_version_id, to_room_version}; -use crate::rooms::timeline::pdu_fits; #[implement(super::Service)] #[allow(clippy::too_many_arguments)] @@ -27,18 +25,9 @@ pub(super) async fn handle_outlier_pdu<'a, Pdu>( where Pdu: Event + Send + Sync, { - if !pdu_fits(&mut value.clone()) { - warn!( - "dropping incoming PDU {event_id} in room {room_id} from {origin} because it \ - exceeds 65535 bytes or is otherwise too large." - ); - return Err!(Request(TooLarge("PDU is too large"))); - } // 1. Remove unsigned field value.remove("unsigned"); - // TODO: For RoomVersion6 we must check that Raw<..> is canonical do we anywhere?: https://matrix.org/docs/spec/rooms/v6#canonical-json - // 2. Check signatures, otherwise drop // 3. check content hash, redact if doesn't match let room_version_id = get_room_version_id(create_event)?; diff --git a/src/service/rooms/timeline/create.rs b/src/service/rooms/timeline/create.rs index 40e41b084..34bf93c45 100644 --- a/src/service/rooms/timeline/create.rs +++ b/src/service/rooms/timeline/create.rs @@ -22,33 +22,18 @@ use serde_json::value::{RawValue, to_raw_value}; use super::RoomMutexGuard; -pub fn pdu_fits(owned_obj: &mut CanonicalJsonObject) -> bool { +#[must_use] +pub fn pdu_fits(owned_obj: &CanonicalJsonObject) -> bool { // room IDs, event IDs, senders, types, and state keys must all be <= 255 bytes - if let Some(CanonicalJsonValue::String(room_id)) = owned_obj.get("room_id") { - if room_id.len() > 255 { - return false; - } - } - if let Some(CanonicalJsonValue::String(event_id)) = owned_obj.get("event_id") { - if event_id.len() > 255 { - return false; - } - } - if let Some(CanonicalJsonValue::String(sender)) = owned_obj.get("sender") { - if sender.len() > 255 { - return false; - } - } - if let Some(CanonicalJsonValue::String(kind)) = owned_obj.get("type") { - if kind.len() > 255 { - return false; - } - } - if let Some(CanonicalJsonValue::String(state_key)) = owned_obj.get("state_key") { - if state_key.len() > 255 { - return false; + const STANDARD_KEYS: [&str; 5] = ["room_id", "event_id", "sender", "type", "state_key"]; + for key in STANDARD_KEYS { + if let Some(CanonicalJsonValue::String(v)) = owned_obj.get(key) { + if v.len() > 255 { + return false; + } } } + // Now check the full PDU size match serde_json::to_string(owned_obj) { | Ok(s) => s.len() <= 65535, @@ -259,14 +244,9 @@ pub async fn create_hash_and_sign_event( let mut pdu_json = utils::to_canonical_object(&pdu).map_err(|e| { err!(Request(BadJson(warn!("Failed to convert PDU to canonical JSON: {e}")))) })?; - - // room v3 and above removed the "event_id" field from remote PDU format - match room_version_id { - | RoomVersionId::V1 | RoomVersionId::V2 => {}, - | _ => { - pdu_json.remove("event_id"); - }, - } + pdu_json.remove("event_id"); + // We need to pop unsigned temporarily for the size check + let unsigned_tmp = pdu_json.remove_entry("unsigned"); trace!("hashing and signing event {}", pdu.event_id); if let Err(e) = self @@ -276,24 +256,28 @@ pub async fn create_hash_and_sign_event( { return match e { | Error::Signatures(ruma::signatures::Error::PduSize) => { - Err!(Request(TooLarge("Message/PDU is too long (exceeds 65535 bytes)"))) + // Ruma does do a PDU size check when generating the content hash, but it + // doesn't do it very correctly. + Err!(Request(TooLarge("PDU is too long large (exceeds 65535 bytes)"))) }, | _ => Err!(Request(Unknown(warn!("Signing event failed: {e}")))), }; } + // Verify that the *full* PDU isn't over 64KiB. + if !pdu_fits(&pdu_json) { + return Err!(Request(TooLarge("PDU is too long large (exceeds 65535 bytes)"))); + } + // Re-insert unsigned data + if let Some((key, value)) = unsigned_tmp { + pdu_json.insert(key, value); + } + // Generate event id pdu.event_id = gen_event_id(&pdu_json, &room_version_id)?; pdu_json.insert("event_id".into(), CanonicalJsonValue::String(pdu.event_id.clone().into())); - // Verify that the *full* PDU isn't over 64KiB. - // Ruma only validates that it's under 64KiB before signing and hashing. - // Has to be cloned to prevent mutating pdu_json itself :( - if !pdu_fits(&mut pdu_json.clone()) { - // feckin huge PDU mate - return Err!(Request(TooLarge("Message/PDU is too long (exceeds 65535 bytes)"))); - } // Check with the policy server - if room_id.is_some() { + if let Some(room_id) = pdu.room_id() { trace!( "Checking event in room {} with policy server", pdu.room_id.as_ref().map_or("None", |id| id.as_str()) @@ -301,7 +285,7 @@ pub async fn create_hash_and_sign_event( match self .services .event_handler - .ask_policy_server(&pdu, &mut pdu_json, pdu.room_id().expect("has room ID"), false) + .ask_policy_server(&pdu, &mut pdu_json, room_id, false) .await { | Ok(true) => {},