From 4d2c261ff85f607c3af09be89044047dc84bdfec Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 19 May 2025 18:35:21 +0100 Subject: [PATCH] crypto: rewrite `check_sender_trust_requirement` to use `VerificationState` (#5044) There are two reasons for this. Firstly. we've already done a bunch of work to map `SenderData` into a `VerificationState`, and the decision tree from `VerificationState` to allow/reject is simpler than going from `SenderData`, even if we have to fudge it a bit to get the "legacy" flag. (Note that it allows us to get rid of an `unreachable!` panic.) Secondly, `VerficationState` represents the state of an *event*, whereas `SenderData` is about the session as a whole. A session can be fine, whilst events (claiming to be) encrypted with it can be suspect. What we want here is to check a specific message. Currently, this doesn't make any functional difference, but conceptually it's cleaner to check the `VerificationState`. Note that there are a bunch of tests for this method in `matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs`, called `test_decryption_trust_requirement`. --- crates/matrix-sdk-crypto/src/machine/mod.rs | 111 +++++++++++++------- 1 file changed, 72 insertions(+), 39 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index 36cf52c0f..6802c3c34 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -57,7 +57,7 @@ use tracing::trace; use tracing::{ debug, error, field::{debug, display}, - info, instrument, warn, Span, + info, instrument, trace, warn, Span, }; use vodozemac::{ megolm::{DecryptionError, SessionOrdering}, @@ -1834,53 +1834,86 @@ impl OlmMachine { } } - /// Check that the sender of a Megolm session satisfies the trust + /// Check that a Megolm event satisfies the sender trust /// requirement from the decryption settings. + /// + /// If the requirement is not satisfied, returns + /// [`MegolmError::SenderIdentityNotTrusted`]. fn check_sender_trust_requirement( &self, session: &InboundGroupSession, encryption_info: &EncryptionInfo, trust_requirement: &TrustRequirement, ) -> MegolmResult<()> { - /// Get the error from the encryption information. - fn encryption_info_to_error(encryption_info: &EncryptionInfo) -> MegolmResult<()> { - // When this is called, the verification state *must* be unverified, - // otherwise the sender_data would have been SenderVerified - let VerificationState::Unverified(verification_level) = - &encryption_info.verification_state - else { - unreachable!("inconsistent verification state"); - }; + trace!( + verification_state = ?encryption_info.verification_state, + ?trust_requirement, "check_sender_trust_requirement", + ); + + // VerificationState::Verified is acceptable for all TrustRequirement levels, so + // let's get that out of the way + let verification_level = match &encryption_info.verification_state { + VerificationState::Verified => return Ok(()), + VerificationState::Unverified(verification_level) => verification_level, + }; + + let ok = match trust_requirement { + TrustRequirement::Untrusted => true, + + TrustRequirement::CrossSignedOrLegacy => { + // `VerificationLevel::UnsignedDevice` and `VerificationLevel::None` correspond + // to `SenderData::DeviceInfo` and `SenderData::UnknownDevice` + // respectively, and those cases may be acceptable if the reason + // for the lack of data is that the sessions were established + // before we started collecting SenderData. + let legacy_session = match session.sender_data { + SenderData::DeviceInfo { legacy_session, .. } => legacy_session, + SenderData::UnknownDevice { legacy_session, .. } => legacy_session, + _ => false, + }; + + // In the CrossSignedOrLegacy case the following rules apply: + // + // 1. Identities we have not yet verified can be decrypted regardless of the + // legacy state of the session. + // 2. Devices that aren't signed by the owning identity of the device can only + // be decrypted if it's a legacy session. + // 3. If we have no information about the device, we should only decrypt if it's + // a legacy session. + // 4. Anything else, should throw an error. + match (verification_level, legacy_session) { + // Case 1 + (VerificationLevel::UnverifiedIdentity, _) => true, + + // Case 2 + (VerificationLevel::UnsignedDevice, true) => true, + + // Case 3 + (VerificationLevel::None(_), true) => true, + + // Case 4 + (VerificationLevel::VerificationViolation, _) + | (VerificationLevel::UnsignedDevice, false) + | (VerificationLevel::None(_), false) => false, + } + } + + // If cross-signing of identities is required, the only acceptable unverified case + // is when the identity is signed but not yet verified by us. + TrustRequirement::CrossSigned => match verification_level { + VerificationLevel::UnverifiedIdentity => true, + + VerificationLevel::VerificationViolation + | VerificationLevel::UnsignedDevice + | VerificationLevel::None(_) => false, + }, + }; + + if ok { + Ok(()) + } else { Err(MegolmError::SenderIdentityNotTrusted(verification_level.clone())) } - - match trust_requirement { - TrustRequirement::Untrusted => Ok(()), - - TrustRequirement::CrossSignedOrLegacy => match &session.sender_data { - // Reject if the sender was previously verified, but changed - // their identity and is not verified any more. - SenderData::VerificationViolation(..) => Err( - MegolmError::SenderIdentityNotTrusted(VerificationLevel::VerificationViolation), - ), - SenderData::SenderUnverified(..) => Ok(()), - SenderData::SenderVerified(..) => Ok(()), - SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()), - SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()), - _ => encryption_info_to_error(encryption_info), - }, - - TrustRequirement::CrossSigned => match &session.sender_data { - // Reject if the sender was previously verified, but changed - // their identity and is not verified any more. - SenderData::VerificationViolation(..) => Err( - MegolmError::SenderIdentityNotTrusted(VerificationLevel::VerificationViolation), - ), - SenderData::SenderUnverified(..) => Ok(()), - SenderData::SenderVerified(..) => Ok(()), - _ => encryption_info_to_error(encryption_info), - }, - } } /// Attempt to retrieve an inbound group session from the store.