From 0379bf370e16c8ded924aedcadcbcd7edacb4c44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Tue, 22 Oct 2024 23:39:17 +0200 Subject: [PATCH 1/5] Fix sending note to self as primary without secondary devices --- src/sender.rs | 49 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/src/sender.rs b/src/sender.rs index 56f31d0fb..e8cb48888 100644 --- a/src/sender.rs +++ b/src/sender.rs @@ -1,4 +1,3 @@ -use core::time; use std::{collections::HashSet, time::SystemTime}; use chrono::prelude::*; @@ -142,6 +141,12 @@ pub enum ThreadIdentifier { Group(GroupV2Id), } +#[derive(Debug)] +pub struct EncryptedMessages { + messages: Vec, + used_identity_key: IdentityKey, +} + impl MessageSender where S: ProtocolStore + SenderKeyStore + SessionStoreExt + Sync + Clone, @@ -533,13 +538,23 @@ where let content_bytes = content.encode_to_vec(); for _ in 0..4u8 { - let (messages, used_identity_key) = self + let Some(EncryptedMessages { + messages, + used_identity_key, + }) = self .create_encrypted_messages( &recipient, unidentified_access.map(|x| &x.certificate), &content_bytes, ) - .await?; + .await? + else { + // this can happen for example when a device is primary, without any secondaries + // and we send a message to ourselves (which is only a SyncMessage { sent: ... }) + // addressed to self + debug!("no messages were encrypted: this should rarely happen"); + break; + }; let messages = OutgoingPushMessages { destination: recipient.uuid, @@ -839,8 +854,7 @@ where recipient: &ServiceAddress, unidentified_access: Option<&SenderCertificate>, content: &[u8], - ) -> Result<(Vec, IdentityKey), MessageSenderError> - { + ) -> Result, MessageSenderError> { let mut messages = vec![]; let mut devices: HashSet = self @@ -925,15 +939,22 @@ where } } - let identity_key = self - .protocol_store - .get_identity(&recipient.to_protocol_address(DEFAULT_DEVICE_ID)) - .await? - .ok_or(MessageSenderError::UntrustedIdentity { - address: *recipient, - })?; - - Ok((messages, identity_key)) + if messages.is_empty() { + Ok(None) + } else { + Ok(Some(EncryptedMessages { + messages, + used_identity_key: self + .protocol_store + .get_identity( + &recipient.to_protocol_address(DEFAULT_DEVICE_ID), + ) + .await? + .ok_or(MessageSenderError::UntrustedIdentity { + address: *recipient, + })?, + })) + } } /// Equivalent to `getEncryptedMessage` From da8a07476bb48c58cf822773c2f24504298fb9d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Tue, 22 Oct 2024 23:55:04 +0200 Subject: [PATCH 2/5] Also add a guard to send SyncMessage only if is_multi_device is true --- src/sender.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/sender.rs b/src/sender.rs index e8cb48888..9e4091a45 100644 --- a/src/sender.rs +++ b/src/sender.rs @@ -354,6 +354,7 @@ where ) -> SendMessageResult { let content_body = message.into(); let message_to_self = recipient == &self.local_aci; + let is_multi_device = self.is_multi_device().await; use crate::proto::data_message::Flags; @@ -365,7 +366,7 @@ where }; // only send a sync message when sending to self and skip the rest of the process - if message_to_self { + if message_to_self && is_multi_device { debug!("sending note to self"); let sync_message = Self::create_multi_device_sent_transcript_content( @@ -408,7 +409,7 @@ where _ => false, }; - if needs_sync || self.is_multi_device().await { + if needs_sync || is_multi_device { debug!("sending multi-device sync message"); let sync_message = Self::create_multi_device_sent_transcript_content( @@ -544,7 +545,7 @@ where }) = self .create_encrypted_messages( &recipient, - unidentified_access.map(|x| &x.certificate), + uDnidentified_access.map(|x| &x.certificate), &content_bytes, ) .await? From 1de50b03c459cbb276764f45dbac219cde2f8858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Wed, 23 Oct 2024 10:06:58 +0200 Subject: [PATCH 3/5] Fix typo --- src/sender.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sender.rs b/src/sender.rs index 9e4091a45..cea28bcf2 100644 --- a/src/sender.rs +++ b/src/sender.rs @@ -545,7 +545,7 @@ where }) = self .create_encrypted_messages( &recipient, - uDnidentified_access.map(|x| &x.certificate), + unidentified_access.map(|x| &x.certificate), &content_bytes, ) .await? From fb5196a254c056a50cb65d0ab923c9e440cd11d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Wed, 23 Oct 2024 10:07:07 +0200 Subject: [PATCH 4/5] Change logging msg --- src/sender.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/sender.rs b/src/sender.rs index cea28bcf2..1a7e3f01c 100644 --- a/src/sender.rs +++ b/src/sender.rs @@ -6,7 +6,7 @@ use libsignal_protocol::{ ProtocolStore, SenderCertificate, SenderKeyStore, SignalProtocolError, }; use rand::{CryptoRng, Rng}; -use tracing::{debug, error, info, trace}; +use tracing::{debug, error, info, trace, warn}; use tracing_futures::Instrument; use uuid::Uuid; use zkgroup::GROUP_IDENTIFIER_LEN; @@ -553,7 +553,9 @@ where // this can happen for example when a device is primary, without any secondaries // and we send a message to ourselves (which is only a SyncMessage { sent: ... }) // addressed to self - debug!("no messages were encrypted: this should rarely happen"); + warn!( + "no messages were encrypted: this should not really happen and most likely implies a logic error." + ); break; }; From 5c88f30ced14cc146a10f51c033522541dc781b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Wed, 23 Oct 2024 15:19:31 +0200 Subject: [PATCH 5/5] Small cleanup --- src/sender.rs | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/src/sender.rs b/src/sender.rs index 1a7e3f01c..460e45900 100644 --- a/src/sender.rs +++ b/src/sender.rs @@ -131,6 +131,9 @@ pub enum MessageSenderError { #[error("Recipient not found: {addr:?}")] NotFound { addr: ServiceAddress }, + + #[error("no messages were encrypted: this should not really happen and most likely implies a logic error")] + NoMessagesToSend, } pub type GroupV2Id = [u8; GROUP_IDENTIFIER_LEN]; @@ -553,10 +556,7 @@ where // this can happen for example when a device is primary, without any secondaries // and we send a message to ourselves (which is only a SyncMessage { sent: ... }) // addressed to self - warn!( - "no messages were encrypted: this should not really happen and most likely implies a logic error." - ); - break; + return Err(MessageSenderError::NoMessagesToSend); }; let messages = OutgoingPushMessages { @@ -908,32 +908,24 @@ where messages.push(message); break; }, - Err( - e @ MessageSenderError::ServiceError( - ServiceError::SignalProtocolError( - SignalProtocolError::SessionNotFound(_), - ), + Err(MessageSenderError::ServiceError( + ServiceError::SignalProtocolError( + SignalProtocolError::SessionNotFound(addr), ), - ) => { - let MessageSenderError::ServiceError( - ServiceError::SignalProtocolError( - SignalProtocolError::SessionNotFound(addr), - ), - ) = &e - else { - // We can't bind to addr above, because we move into `e`. - unreachable!() - }; + )) => { // SessionNotFound is returned on certain session corruption. // Since delete_session *creates* a session if it doesn't exist, // the NotFound error is an indicator of session corruption. // Try to delete this session, if it gets succesfully deleted, retry. Otherwise, fail. tracing::warn!("Potential session corruption for {}, deleting session", addr); - match self.protocol_store.delete_session(addr).await { + match self.protocol_store.delete_session(&addr).await { Ok(()) => continue, - Err(_e) => { - tracing::warn!("Failed to delete session for {}, failing message. {}", addr, _e); - return Err(e); + Err(error) => { + tracing::warn!(%error, %addr, "failed to delete session"); + return Err( + SignalProtocolError::SessionNotFound(addr) + .into(), + ); }, } },