From d9e5e4d94dd6a56e9fc0c16dbf5ddd437e3f4bd6 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Thu, 28 Aug 2025 04:18:26 -0300 Subject: [PATCH 1/4] fix: Don't reverify contacts by SELF on receipt of a message from another device Also verify not yet verified contacts w/o setting a verifier for them (in the db it's stored as `verifier_id=id` though) because we don't know who verified them for another device. --- src/contact.rs | 13 +++++++++---- src/context.rs | 2 +- src/receive_imf.rs | 3 ++- src/receive_imf/receive_imf_tests.rs | 26 ++++++++++++++++++++++++++ src/securejoin.rs | 4 ++-- src/test_utils.rs | 2 +- 6 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/contact.rs b/src/contact.rs index 2eb8bda663..978e2f0db2 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1948,16 +1948,21 @@ pub(crate) async fn update_last_seen( } /// Marks contact `contact_id` as verified by `verifier_id`. +/// +/// `verifier_id == None` means that the verifier is unknown. pub(crate) async fn mark_contact_id_as_verified( context: &Context, contact_id: ContactId, - verifier_id: ContactId, + verifier_id: Option, ) -> Result<()> { + ensure_and_debug_assert_ne!(contact_id, ContactId::SELF,); ensure_and_debug_assert_ne!( - contact_id, + Some(contact_id), verifier_id, "Contact cannot be verified by self", ); + let update = verifier_id == Some(ContactId::SELF); + let verifier_id = verifier_id.unwrap_or(contact_id); context .sql .transaction(|transaction| { @@ -1983,8 +1988,8 @@ pub(crate) async fn mark_contact_id_as_verified( } transaction.execute( "UPDATE contacts SET verifier=?1 - WHERE id=?2 AND (verifier=0 OR ?1=?3)", - (verifier_id, contact_id, ContactId::SELF), + WHERE id=?2 AND (verifier=0 OR ?3)", + (verifier_id, contact_id, update), )?; Ok(()) }) diff --git a/src/context.rs b/src/context.rs index 0272a74c06..ba085f63e5 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1215,7 +1215,7 @@ impl Context { .await? .first() .context("Self reporting bot vCard does not contain a contact")?; - mark_contact_id_as_verified(self, contact_id, ContactId::SELF).await?; + mark_contact_id_as_verified(self, contact_id, Some(ContactId::SELF)).await?; let chat_id = ChatId::create_for_contact(self, contact_id).await?; chat_id diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 4e7bc78262..760a14379d 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -3691,12 +3691,13 @@ async fn mark_recipients_as_verified( if mimeparser.get_header(HeaderDef::ChatVerified).is_none() { return Ok(()); } + let verifier_id = Some(from_id).filter(|&id| id != ContactId::SELF); for to_id in to_ids.iter().filter_map(|&x| x) { if to_id == ContactId::SELF || to_id == from_id { continue; } - mark_contact_id_as_verified(context, to_id, from_id).await?; + mark_contact_id_as_verified(context, to_id, verifier_id).await?; ChatId::set_protection_for_contact(context, to_id, mimeparser.timestamp_sent).await?; } diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index 3feb8183f3..861bb984f3 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/src/receive_imf/receive_imf_tests.rs @@ -5154,6 +5154,32 @@ async fn test_unverified_member_msg() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_dont_reverify_by_self_on_outgoing_msg() -> Result<()> { + let mut tcm = TestContextManager::new(); + let a0 = &tcm.alice().await; + let a1 = &tcm.alice().await; + let bob = &tcm.bob().await; + let fiona = &tcm.fiona().await; + + let bob_chat_id = chat::create_group_chat(bob, ProtectionStatus::Protected, "Group").await?; + let qr = get_securejoin_qr(bob, Some(bob_chat_id)).await?; + tcm.exec_securejoin_qr(fiona, bob, &qr).await; + tcm.exec_securejoin_qr(a0, bob, &qr).await; + tcm.exec_securejoin_qr(a1, bob, &qr).await; + + let a0_chat_id = a0.get_last_msg().await.chat_id; + let a0_sent_msg = a0.send_text(a0_chat_id, "Hi").await; + a1.recv_msg(&a0_sent_msg).await; + let a1_bob_id = a1.add_or_lookup_contact_id(bob).await; + let a1_fiona = a1.add_or_lookup_contact(fiona).await; + assert_eq!( + a1_fiona.get_verifier_id(a1).await?.unwrap().unwrap(), + a1_bob_id + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_sanitize_filename_in_received() -> Result<()> { let alice = &TestContext::new_alice().await; diff --git a/src/securejoin.rs b/src/securejoin.rs index 4fff0eca28..7813c9de5e 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -210,7 +210,7 @@ async fn verify_sender_by_fingerprint( let contact = Contact::get_by_id(context, contact_id).await?; let is_verified = contact.fingerprint().is_some_and(|fp| &fp == fingerprint); if is_verified { - mark_contact_id_as_verified(context, contact_id, ContactId::SELF).await?; + mark_contact_id_as_verified(context, contact_id, Some(ContactId::SELF)).await?; } Ok(is_verified) } @@ -548,7 +548,7 @@ pub(crate) async fn observe_securejoin_on_other_device( return Ok(HandshakeMessage::Ignore); } - mark_contact_id_as_verified(context, contact_id, ContactId::SELF).await?; + mark_contact_id_as_verified(context, contact_id, Some(ContactId::SELF)).await?; ChatId::set_protection_for_contact(context, contact_id, mime_message.timestamp_sent).await?; diff --git a/src/test_utils.rs b/src/test_utils.rs index 6900cc0d24..664ce993cd 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -1420,7 +1420,7 @@ fn print_logevent(logevent: &LogEvent) { /// Saves the other account's public key as verified pub(crate) async fn mark_as_verified(this: &TestContext, other: &TestContext) { let contact_id = this.add_or_lookup_contact_id(other).await; - mark_contact_id_as_verified(this, contact_id, ContactId::SELF) + mark_contact_id_as_verified(this, contact_id, Some(ContactId::SELF)) .await .unwrap(); } From 0f8852e08f22da8d8153b12116df9e28165bbe03 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 29 Aug 2025 14:57:09 -0300 Subject: [PATCH 2/4] refactor: Check that verifier is verified in turn --- src/contact.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/contact.rs b/src/contact.rs index 978e2f0db2..d318346e88 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1975,16 +1975,21 @@ pub(crate) async fn mark_contact_id_as_verified( bail!("Non-key-contact {contact_id} cannot be verified"); } if verifier_id != ContactId::SELF { - let verifier_fingerprint: String = transaction.query_row( - "SELECT fingerprint FROM contacts WHERE id=?", - (verifier_id,), - |row| row.get(0), - )?; + let (verifier_fingerprint, verifier_verifier_id): (String, ContactId) = transaction + .query_row( + "SELECT fingerprint, verifier FROM contacts WHERE id=?", + (verifier_id,), + |row| Ok((row.get(0)?, row.get(1)?)), + )?; if verifier_fingerprint.is_empty() { bail!( "Contact {contact_id} cannot be verified by non-key-contact {verifier_id}" ); } + ensure!( + verifier_id == contact_id || verifier_verifier_id != ContactId::UNDEFINED, + "Contact {contact_id} cannot be verified by unverified contact {verifier_id}", + ); } transaction.execute( "UPDATE contacts SET verifier=?1 From 06232148227de5b432ef32d659b5dd19ee3dbde6 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 29 Aug 2025 15:14:36 -0300 Subject: [PATCH 3/4] fix: Don't verify contacts by others having an unknown verifier If this happens, mark the contact as verified by an unknown contact instead. This avoids introducing incorrect reverse chains: if the verifier itself has an unknown verifier, it may be `contact_id` actually (directly or indirectly) on the other device (which is needed for getting "verified by unknown contact" in the first place). --- src/contact.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/contact.rs b/src/contact.rs index d318346e88..09c5b889c7 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1962,7 +1962,7 @@ pub(crate) async fn mark_contact_id_as_verified( "Contact cannot be verified by self", ); let update = verifier_id == Some(ContactId::SELF); - let verifier_id = verifier_id.unwrap_or(contact_id); + let mut verifier_id = verifier_id.unwrap_or(contact_id); context .sql .transaction(|transaction| { @@ -1990,6 +1990,13 @@ pub(crate) async fn mark_contact_id_as_verified( verifier_id == contact_id || verifier_verifier_id != ContactId::UNDEFINED, "Contact {contact_id} cannot be verified by unverified contact {verifier_id}", ); + if verifier_verifier_id == verifier_id { + // Avoid introducing incorrect reverse chains: if the verifier itself has an + // unknown verifier, it may be `contact_id` actually (directly or indirectly) on + // the other device (which is needed for getting "verified by unknown contact" + // in the first place). + verifier_id = contact_id; + } } transaction.execute( "UPDATE contacts SET verifier=?1 From c10965c0747ab22586a8116fa2a5bf9f2e5c6607 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 29 Aug 2025 15:42:17 -0300 Subject: [PATCH 4/4] fix: Update verifier_id if it's "unknown" and new verifier has known verifier Now that the previous commit avoids creating incorrect reverse verification chains, we can do this. Sure, existing users' dbs aready have verification chains ending with "unknown" roots, but at least for new users updating `verifier_id` to a known verifier makes sense. --- src/contact.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/contact.rs b/src/contact.rs index 09c5b889c7..a9925a04f1 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1961,7 +1961,7 @@ pub(crate) async fn mark_contact_id_as_verified( verifier_id, "Contact cannot be verified by self", ); - let update = verifier_id == Some(ContactId::SELF); + let by_self = verifier_id == Some(ContactId::SELF); let mut verifier_id = verifier_id.unwrap_or(contact_id); context .sql @@ -2000,8 +2000,8 @@ pub(crate) async fn mark_contact_id_as_verified( } transaction.execute( "UPDATE contacts SET verifier=?1 - WHERE id=?2 AND (verifier=0 OR ?3)", - (verifier_id, contact_id, update), + WHERE id=?2 AND (verifier=0 OR verifier=id OR ?3)", + (verifier_id, contact_id, by_self), )?; Ok(()) })