diff --git a/src/e2ee.rs b/src/e2ee.rs index 200111adb..011e8b8c0 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -1,8 +1,9 @@ //! End-to-end encryption support. +use std::collections::BTreeSet; use std::io::Cursor; -use anyhow::{format_err, Context as _, Result}; +use anyhow::{bail, Result}; use mail_builder::mime::MimePart; use num_traits::FromPrimitive; @@ -42,70 +43,76 @@ impl EncryptHelper { } /// Determines if we can and should encrypt. - /// - /// `e2ee_guaranteed` should be set to true for replies to encrypted messages (as required by - /// Autocrypt Level 1, version 1.1) and for messages sent in protected groups. - /// - /// Returns an error if `e2ee_guaranteed` is true, but one or more keys are missing. pub(crate) async fn should_encrypt( &self, context: &Context, - e2ee_guaranteed: bool, peerstates: &[(Option, String)], ) -> Result { let is_chatmail = context.is_chatmail().await?; - let missing_peerstate_addr = peerstates.iter().find_map(|(peerstate, addr)| { + for (peerstate, _addr) in peerstates { if let Some(peerstate) = peerstate { - if is_chatmail - || e2ee_guaranteed - || peerstate.prefer_encrypt != EncryptPreference::Reset - { - return None; + // For chatmail we ignore the encryption preference, + // because we can either send encrypted or not at all. + if is_chatmail || peerstate.prefer_encrypt != EncryptPreference::Reset { + continue; } } - Some(addr) - }); - if let Some(addr) = missing_peerstate_addr { - if e2ee_guaranteed { - return Err(format_err!( - "Peerstate for {addr:?} missing, cannot encrypt" - )); - } + return Ok(false); } - Ok(missing_peerstate_addr.is_none()) + Ok(true) } - /// Tries to encrypt the passed in `mail`. - pub async fn encrypt( - self, + /// Constructs a vector of public keys for given peerstates. + /// + /// In addition returns the set of recipient addresses + /// for which there is no key available. + /// + /// Returns an error if there are recipients + /// other than self, but no recipient keys are available. + pub(crate) fn encryption_keyring( + &self, context: &Context, verified: bool, - mail_to_encrypt: MimePart<'static>, - peerstates: Vec<(Option, String)>, - compress: bool, - ) -> Result { - let mut keyring: Vec = Vec::new(); + peerstates: &[(Option, String)], + ) -> Result<(Vec, BTreeSet)> { + // Encrypt to self unconditionally, + // even for a single-device setup. + let mut keyring = vec![self.public_key.clone()]; + let mut missing_key_addresses = BTreeSet::new(); + + if peerstates.is_empty() { + return Ok((keyring, missing_key_addresses)); + } let mut verifier_addresses: Vec<&str> = Vec::new(); - for (peerstate, addr) in peerstates - .iter() - .filter_map(|(state, addr)| state.clone().map(|s| (s, addr))) - { - let key = peerstate - .take_key(verified) - .with_context(|| format!("proper enc-key for {addr} missing, cannot encrypt"))?; - keyring.push(key); - verifier_addresses.push(addr); + for (peerstate, addr) in peerstates { + if let Some(peerstate) = peerstate { + if let Some(key) = peerstate.clone().take_key(verified) { + keyring.push(key); + verifier_addresses.push(addr); + } else { + warn!(context, "Encryption key for {addr} is missing."); + missing_key_addresses.insert(addr.clone()); + } + } else { + warn!(context, "Peerstate for {addr} is missing."); + missing_key_addresses.insert(addr.clone()); + } } - // Encrypt to self. - keyring.push(self.public_key.clone()); + debug_assert!( + !keyring.is_empty(), + "At least our own key is in the keyring" + ); + if keyring.len() <= 1 { + bail!("No recipient keys are available, cannot encrypt"); + } // Encrypt to secondary verified keys // if we also encrypt to the introducer ("verifier") of the key. if verified { - for (peerstate, _addr) in &peerstates { + for (peerstate, _addr) in peerstates { if let Some(peerstate) = peerstate { if let (Some(key), Some(verifier)) = ( peerstate.secondary_verified_key.as_ref(), @@ -119,6 +126,17 @@ impl EncryptHelper { } } + Ok((keyring, missing_key_addresses)) + } + + /// Tries to encrypt the passed in `mail`. + pub async fn encrypt( + self, + context: &Context, + keyring: Vec, + mail_to_encrypt: MimePart<'static>, + compress: bool, + ) -> Result { let sign_key = load_self_secret_key(context).await?; let mut raw_message = Vec::new(); @@ -315,22 +333,17 @@ Sent with my Delta Chat Messenger: https://delta.chat"; let encrypt_helper = EncryptHelper::new(&t).await.unwrap(); let ps = new_peerstates(EncryptPreference::NoPreference); - assert!(encrypt_helper.should_encrypt(&t, true, &ps).await?); - // Own preference is `Mutual` and we have the peer's key. - assert!(encrypt_helper.should_encrypt(&t, false, &ps).await?); + assert!(encrypt_helper.should_encrypt(&t, &ps).await?); let ps = new_peerstates(EncryptPreference::Reset); - assert!(encrypt_helper.should_encrypt(&t, true, &ps).await?); - assert!(!encrypt_helper.should_encrypt(&t, false, &ps).await?); + assert!(!encrypt_helper.should_encrypt(&t, &ps).await?); let ps = new_peerstates(EncryptPreference::Mutual); - assert!(encrypt_helper.should_encrypt(&t, true, &ps).await?); - assert!(encrypt_helper.should_encrypt(&t, false, &ps).await?); + assert!(encrypt_helper.should_encrypt(&t, &ps).await?); // test with missing peerstate let ps = vec![(None, "bob@foo.bar".to_string())]; - assert!(encrypt_helper.should_encrypt(&t, true, &ps).await.is_err()); - assert!(!encrypt_helper.should_encrypt(&t, false, &ps).await?); + assert!(!encrypt_helper.should_encrypt(&t, &ps).await?); Ok(()) } diff --git a/src/message.rs b/src/message.rs index db61dcf6f..44b6e9b5c 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1732,6 +1732,10 @@ pub async fn delete_msgs_ex( !delete_for_all || msg.from_id == ContactId::SELF, "Can delete only own messages for others" ); + ensure!( + !delete_for_all || msg.get_showpadlock(), + "Cannot request deletion of unencrypted message for others" + ); modified_chat_ids.insert(msg.chat_id); deleted_rfc724_mid.push(msg.rfc724_mid.clone()); diff --git a/src/mimefactory.rs b/src/mimefactory.rs index df1d20f4f..262634d5a 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -86,7 +86,9 @@ pub struct MimeFactory { /// Vector of pairs of recipient name and address that goes into the `To` field. /// /// The list of actual message recipient addresses may be different, - /// e.g. if members are hidden for broadcast lists. + /// e.g. if members are hidden for broadcast lists + /// or if the keys for some recipients are missing + /// and encrypted message cannot be sent to them. to: Vec<(String, String)>, /// Vector of pairs of past group member names and addresses. @@ -784,9 +786,7 @@ impl MimeFactory { let peerstates = self.peerstates_for_recipients(context).await?; let is_encrypted = !self.should_force_plaintext() - && encrypt_helper - .should_encrypt(context, e2ee_guaranteed, &peerstates) - .await?; + && (e2ee_guaranteed || encrypt_helper.should_encrypt(context, &peerstates).await?); let is_securejoin_message = if let Loaded::Message { msg, .. } = &self.loaded { msg.param.get_cmd() == SystemMessage::SecurejoinMessage } else { @@ -982,14 +982,23 @@ impl MimeFactory { Loaded::Mdn { .. } => true, }; + let (encryption_keyring, missing_key_addresses) = + encrypt_helper.encryption_keyring(context, verified, &peerstates)?; + // XXX: additional newline is needed // to pass filtermail at // let encrypted = encrypt_helper - .encrypt(context, verified, message, peerstates, compress) + .encrypt(context, encryption_keyring, message, compress) .await? + "\n"; + // Remove recipients for which the key is missing. + if !missing_key_addresses.is_empty() { + self.recipients + .retain(|addr| !missing_key_addresses.contains(addr)); + } + // Set the appropriate Content-Type for the outer message MimePart::new( "multipart/encrypted; protocol=\"application/pgp-encrypted\"", diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index e60afe083..f93ce57fb 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/src/receive_imf/receive_imf_tests.rs @@ -4924,11 +4924,10 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> { let fiona_addr = fiona.get_config(Config::Addr).await?.unwrap(); mark_as_verified(alice, fiona).await; let alice_fiona_id = alice.add_or_lookup_contact(fiona).await.id; - assert!(add_contact_to_chat(alice, group_id, alice_fiona_id) - .await - .is_err()); - // Sending the message failed, - // but member is added to the chat locally already. + add_contact_to_chat(alice, group_id, alice_fiona_id).await?; + + // The message is not sent to Bob, + // but member is added to the chat locally anyway. assert!(is_contact_in_chat(alice, group_id, alice_fiona_id).await?); let msg = alice.get_last_msg_in(group_id).await; assert!(msg.is_info()); @@ -4937,10 +4936,6 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> { stock_str::msg_add_member_local(alice, &fiona_addr, ContactId::SELF).await ); - // Now the chat has a message "You added member fiona@example.net. [INFO] !!" (with error) that - // may be confusing, but if the error is displayed in UIs, it's more or less ok. This is not a - // normal scenario anyway. - remove_contact_from_chat(alice, group_id, alice_bob_id).await?; assert!(!is_contact_in_chat(alice, group_id, alice_bob_id).await?); let msg = alice.get_last_msg_in(group_id).await; @@ -4949,7 +4944,6 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> { msg.get_text(), stock_str::msg_del_member_local(alice, &bob_addr, ContactId::SELF,).await ); - assert!(msg.error().is_some()); Ok(()) } diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index 4b8e9f072..0f8dd2794 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -1,7 +1,9 @@ use anyhow::Result; use pretty_assertions::assert_eq; -use crate::chat::{self, add_contact_to_chat, Chat, ProtectionStatus}; +use crate::chat::{ + self, add_contact_to_chat, remove_contact_from_chat, send_msg, Chat, ProtectionStatus, +}; use crate::chatlist::Chatlist; use crate::config::Config; use crate::constants::{Chattype, DC_GCL_FOR_FORWARDING}; @@ -953,6 +955,70 @@ async fn test_no_unencrypted_name_if_encrypted() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_verified_lost_member_added() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let fiona = &tcm.fiona().await; + + tcm.execute_securejoin(bob, alice).await; + tcm.execute_securejoin(fiona, alice).await; + + let alice_chat_id = alice + .create_group_with_members(ProtectionStatus::Protected, "Group", &[bob]) + .await; + let alice_sent = alice.send_text(alice_chat_id, "Hi!").await; + let bob_chat_id = bob.recv_msg(&alice_sent).await.chat_id; + assert_eq!(chat::get_chat_contacts(bob, bob_chat_id).await?.len(), 2); + + // Attempt to add member, but message is lost. + let fiona_id = alice.add_or_lookup_contact(fiona).await.id; + add_contact_to_chat(alice, alice_chat_id, fiona_id).await?; + alice.pop_sent_msg().await; + + let alice_sent = alice.send_text(alice_chat_id, "Hi again!").await; + bob.recv_msg(&alice_sent).await; + assert_eq!(chat::get_chat_contacts(bob, bob_chat_id).await?.len(), 3); + + bob_chat_id.accept(bob).await?; + let sent = bob.send_text(bob_chat_id, "Hello!").await; + let sent_msg = Message::load_from_db(bob, sent.sender_msg_id).await?; + assert_eq!(sent_msg.get_showpadlock(), true); + + // The message will not be sent to Fiona. + // Test that Fiona will not be able to decrypt it. + let fiona_rcvd = fiona.recv_msg(&sent).await; + assert_eq!(fiona_rcvd.get_showpadlock(), false); + assert_eq!( + fiona_rcvd.get_text(), + "[...] – [This message was encrypted for another setup.]" + ); + + // Advance the time so Alice does not leave at the same second + // as the group was created. + SystemTime::shift(std::time::Duration::from_secs(100)); + + // Alice leaves the chat. + remove_contact_from_chat(alice, alice_chat_id, ContactId::SELF).await?; + assert_eq!( + chat::get_chat_contacts(alice, alice_chat_id).await?.len(), + 2 + ); + bob.recv_msg(&alice.pop_sent_msg().await).await; + + // Now only Bob and Fiona are in the chat. + assert_eq!(chat::get_chat_contacts(bob, bob_chat_id).await?.len(), 2); + + // Bob cannot send messages anymore because there are no recipients + // other than self for which Bob has the key. + let mut msg = Message::new_text("No key for Fiona".to_string()); + let result = send_msg(bob, bob_chat_id, &mut msg).await; + assert!(result.is_err()); + + Ok(()) +} + // ============== Helper Functions ============== async fn assert_verified(this: &TestContext, other: &TestContext, protected: ProtectionStatus) {