From 7ad3c70b687cca5d0f6212a66f3572111b405a78 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Wed, 12 Jun 2024 11:09:08 -0300 Subject: [PATCH] feat: Don't reveal profile data to a not yet verified contact (#5166) Follow-up to b77131159392daa0bfb8e2ce99c219e1f7bd4823. Since that commit names are not revealed in verified chats, but during verification (i.e. SecureJoin) they are still sent unencrypted. Moreover, all profile data mustn't be sent even encrypted before the contact verification, i.e. before "v{c,g}-request-with-auth". That was done for the selfavatar in 304e902fcec36ce8fde263aa7a45759ed9a2fdaf, now it's done for From/To names and the self-status as well. Moreover, "v{c,g}-request" and "v{c,g}-auth-required" messages are deleted right after processing, so other devices won't see the received profile data anyway. --- src/mimefactory.rs | 66 ++++++++++++++++++++++++++++++---------------- src/securejoin.rs | 40 +++++++++++++++++++++------- src/test_utils.rs | 8 +----- 3 files changed, 76 insertions(+), 38 deletions(-) diff --git a/src/mimefactory.rs b/src/mimefactory.rs index a81d3f1ed..a156622c9 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -142,6 +142,7 @@ struct MessageHeaders { impl<'a> MimeFactory<'a> { pub async fn from_msg(context: &Context, msg: &'a Message) -> Result> { let chat = Chat::load_from_db(context, msg.chat_id).await?; + let attach_profile_data = Self::should_attach_profile_data(msg); let from_addr = context.get_primary_self_addr().await?; let config_displayname = context @@ -152,7 +153,11 @@ impl<'a> MimeFactory<'a> { if let Some(override_name) = msg.param.get(Param::OverrideSenderDisplayname) { (override_name.to_string(), Some(config_displayname)) } else { - (config_displayname, None) + let name = match attach_profile_data { + true => config_displayname, + false => "".to_string(), + }; + (name, None) }; let mut recipients = Vec::with_capacity(5); @@ -186,7 +191,11 @@ impl<'a> MimeFactory<'a> { for row in rows { let (authname, addr, id) = row?; if !recipients_contain_addr(&recipients, &addr) { - recipients.push((authname, addr)); + let name = match attach_profile_data { + true => authname, + false => "".to_string(), + }; + recipients.push((name, addr)); } recipient_ids.insert(id); } @@ -220,16 +229,20 @@ impl<'a> MimeFactory<'a> { }, ) .await?; + let selfstatus = match attach_profile_data { + true => context + .get_config(Config::Selfstatus) + .await? + .unwrap_or_default(), + false => "".to_string(), + }; let attach_selfavatar = Self::should_attach_selfavatar(context, msg).await; let factory = MimeFactory { from_addr, from_displayname, sender_displayname, - selfstatus: context - .get_config(Config::Selfstatus) - .await? - .unwrap_or_default(), + selfstatus, recipients, timestamp: msg.timestamp_sort, loaded: Loaded::Message { chat }, @@ -396,31 +409,35 @@ impl<'a> MimeFactory<'a> { } } - async fn should_attach_selfavatar(context: &Context, msg: &Message) -> bool { - let cmd = msg.param.get_cmd(); - (cmd != SystemMessage::SecurejoinMessage || { + fn should_attach_profile_data(msg: &Message) -> bool { + msg.param.get_cmd() != SystemMessage::SecurejoinMessage || { let step = msg.param.get(Param::Arg).unwrap_or_default(); - // Don't attach selfavatar at the earlier SecureJoin steps: + // Don't attach profile data at the earlier SecureJoin steps: // - The corresponding messages, i.e. "v{c,g}-request" and "v{c,g}-auth-required" are - // deleted right after processing, so other devices won't see the avatar. + // deleted right after processing, so other devices won't see the avatar etc. // - It's also good for privacy because the contact isn't yet verified and these // messages are auto-sent unlike usual unencrypted messages. step == "vg-request-with-auth" || step == "vc-request-with-auth" || step == "vg-member-added" || step == "vc-contact-confirm" - }) && match chat::shall_attach_selfavatar(context, msg.chat_id).await { - Ok(should) => should, - Err(err) => { - warn!( - context, - "should_attach_selfavatar: cannot get selfavatar state: {err:#}." - ); - false - } } } + async fn should_attach_selfavatar(context: &Context, msg: &Message) -> bool { + Self::should_attach_profile_data(msg) + && match chat::shall_attach_selfavatar(context, msg.chat_id).await { + Ok(should) => should, + Err(err) => { + warn!( + context, + "should_attach_selfavatar: cannot get selfavatar state: {err:#}." + ); + false + } + } + } + fn grpimage(&self) -> Option { match &self.loaded { Loaded::Message { chat } => { @@ -480,7 +497,11 @@ impl<'a> MimeFactory<'a> { return Ok(format!("Re: {}", remove_subject_prefix(last_subject))); } - let self_name = &match context.get_config(Config::Displayname).await? { + let self_name = match Self::should_attach_profile_data(self.msg) { + true => context.get_config(Config::Displayname).await?, + false => None, + }; + let self_name = &match self_name { Some(name) => name, None => context.get_config(Config::Addr).await?.unwrap_or_default(), }; @@ -708,7 +729,8 @@ impl<'a> MimeFactory<'a> { .unwrap(), ); } - if is_encrypted && verified { + if is_encrypted && verified || self.msg.param.get_cmd() == SystemMessage::SecurejoinMessage + { headers.unprotected.insert( 0, Header::new_with_value( diff --git a/src/securejoin.rs b/src/securejoin.rs index d643b2d68..a706dd5ff 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -802,6 +802,9 @@ mod tests { let alice = tcm.alice().await; let alice_addr = &alice.get_config(Config::Addr).await.unwrap().unwrap(); let bob = tcm.bob().await; + bob.set_config(Config::Displayname, Some("Bob Examplenet")) + .await + .unwrap(); alice .set_config(Config::VerifiedOneOnOneChats, Some("1")) .await @@ -824,6 +827,11 @@ mod tests { // Step 1: Generate QR-code, ChatId(0) indicates setup-contact let qr = get_securejoin_qr(&alice.ctx, None).await.unwrap(); + // We want Bob to learn Alice's name from their messages, not from the QR code. + alice + .set_config(Config::Displayname, Some("Alice Exampleorg")) + .await + .unwrap(); // Step 2: Bob scans QR-code, sends vc-request join_securejoin(&bob.ctx, &qr).await.unwrap(); @@ -895,6 +903,7 @@ mod tests { // Check Bob sent the right message. let sent = bob.pop_sent_msg().await; + assert!(!sent.payload.contains("Bob Examplenet")); let mut msg = alice.parse_msg(&sent).await; let vc_request_with_auth_ts_sent = msg .get_header(HeaderDef::Date) @@ -946,6 +955,7 @@ mod tests { .await .unwrap(); assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), false); + assert_eq!(contact_bob.get_authname(), ""); if case == SetupContactCase::CheckProtectionTimestamp { SystemTime::shift(Duration::from_secs(3600)); @@ -954,6 +964,10 @@ mod tests { // Step 5+6: Alice receives vc-request-with-auth, sends vc-contact-confirm alice.recv_msg_trash(&sent).await; assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), true); + let contact_bob = Contact::get_by_id(&alice.ctx, contact_bob_id) + .await + .unwrap(); + assert_eq!(contact_bob.get_authname(), "Bob Examplenet"); // exactly one one-to-one chat should be visible for both now // (check this before calling alice.create_chat() explicitly below) @@ -981,8 +995,19 @@ mod tests { } } + // Make sure Alice hasn't yet sent their name to Bob. + let contact_alice_id = Contact::lookup_id_by_addr(&bob.ctx, alice_addr, Origin::Unknown) + .await + .expect("Error looking up contact") + .expect("Contact not found"); + let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id) + .await + .unwrap(); + assert_eq!(contact_alice.get_authname(), ""); + // Check Alice sent the right message to Bob. let sent = alice.pop_sent_msg().await; + assert!(!sent.payload.contains("Alice Exampleorg")); let msg = bob.parse_msg(&sent).await; assert!(msg.was_encrypted()); assert_eq!( @@ -991,18 +1016,15 @@ mod tests { ); // Bob should not yet have Alice verified - let contact_alice_id = Contact::lookup_id_by_addr(&bob.ctx, alice_addr, Origin::Unknown) - .await - .expect("Error looking up contact") - .expect("Contact not found"); - let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id) - .await - .unwrap(); - assert_eq!(contact_bob.is_verified(&bob.ctx).await.unwrap(), false); + assert_eq!(contact_alice.is_verified(&bob.ctx).await.unwrap(), false); // Step 7: Bob receives vc-contact-confirm bob.recv_msg_trash(&sent).await; assert_eq!(contact_alice.is_verified(&bob.ctx).await.unwrap(), true); + let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id) + .await + .unwrap(); + assert_eq!(contact_alice.get_authname(), "Alice Exampleorg"); if case != SetupContactCase::SecurejoinWaitTimeout { // Later we check that the timeout message isn't added to the already protected chat. @@ -1119,7 +1141,7 @@ mod tests { // Alice should not yet have Bob verified let (contact_bob_id, _modified) = Contact::add_or_lookup( &alice.ctx, - "Bob", + "", &ContactAddress::new("bob@example.net")?, Origin::ManuallyCreated, ) diff --git a/src/test_utils.rs b/src/test_utils.rs index 4d0aee65a..d179785c8 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -566,18 +566,12 @@ impl TestContext { /// Returns the [`Contact`] for the other [`TestContext`], creating it if necessary. pub async fn add_or_lookup_contact(&self, other: &TestContext) -> Contact { - let name = other - .ctx - .get_config(Config::Displayname) - .await - .unwrap_or_default() - .unwrap_or_default(); let primary_self_addr = other.ctx.get_primary_self_addr().await.unwrap(); let addr = ContactAddress::new(&primary_self_addr).unwrap(); // MailinglistAddress is the lowest allowed origin, we'd prefer to not modify the // origin when creating this contact. let (contact_id, modified) = - Contact::add_or_lookup(self, &name, &addr, Origin::MailinglistAddress) + Contact::add_or_lookup(self, "", &addr, Origin::MailinglistAddress) .await .expect("add_or_lookup"); match modified {