fix: Don't add SELF to unencrypted chat created from encrypted message (#7661)

I.e. create a non-replyable ad-hoc group in such cases. Unencrypted replies to encrypted messages
are a security issue and "Composing a Reply Message" from RFC 9787 and "Replying and Forwarding
Guidance" from RFC 9788 forbid such replies.

src/receive_imf.rs

@@ -196,7 +196,7 @@ async fn insert_tombstone(context: &Context, rfc724_mid: &str) -> Result<MsgId>
 async fn get_to_and_past_contact_ids(
     context: &Context,
     mime_parser: &MimeMessage,
-    chat_assignment: &ChatAssignment,
+    chat_assignment: &mut ChatAssignment,
     parent_message: &Option<Message>,
     incoming_origin: Origin,
 ) -> Result<(Vec<Option<ContactId>>, Vec<Option<ContactId>>)> {
@@ -385,32 +385,6 @@ async fn get_to_and_past_contact_ids(
                 // mapped it to a key contact.
                 // This is an encrypted 1:1 chat.
                 to_ids = pgp_to_ids
-            } else if let Some(chat_id) = chat_id {
-                to_ids = match mime_parser.was_encrypted() {
-                    true => {
-                        lookup_key_contacts_by_address_list(
-                            context,
-                            &mime_parser.recipients,
-                            to_member_fingerprints,
-                            Some(chat_id),
-                        )
-                        .await?
-                    }
-                    false => {
-                        add_or_lookup_contacts_by_address_list(
-                            context,
-                            &mime_parser.recipients,
-                            if !mime_parser.incoming {
-                                Origin::OutgoingTo
-                            } else if incoming_origin.is_known() {
-                                Origin::IncomingTo
-                            } else {
-                                Origin::IncomingUnknownTo
-                            },
-                        )
-                        .await?
-                    }
-                }
             } else {
                 let ids = match mime_parser.was_encrypted() {
                     true => {
@@ -418,7 +392,7 @@ async fn get_to_and_past_contact_ids(
                             context,
                             &mime_parser.recipients,
                             to_member_fingerprints,
-                            None,
+                            chat_id,
                         )
                         .await?
                     }
@@ -426,13 +400,20 @@ async fn get_to_and_past_contact_ids(
                 };
                 if mime_parser.was_encrypted() && !ids.contains(&None)
                 // Prefer creating PGP chats if there are any key-contacts. At least this prevents
-                // from replying unencrypted.
+                // from replying unencrypted. Otherwise downgrade to a non-replyable ad-hoc group.
                 || ids
                     .iter()
                     .any(|&c| c.is_some() && c != Some(ContactId::SELF))
                 {
                     to_ids = ids;
                 } else {
+                    if mime_parser.was_encrypted() {
+                        warn!(
+                            context,
+                            "No key-contact looked up. Downgrading to AdHocGroup."
+                        );
+                        *chat_assignment = ChatAssignment::AdHocGroup;
+                    }
                     to_ids = add_or_lookup_contacts_by_address_list(
                         context,
                         &mime_parser.recipients,
@@ -634,12 +615,12 @@ pub(crate) async fn receive_imf_inner(
     .await?
     .filter(|p| Some(p.id) != replace_msg_id);
 
-    let chat_assignment =
+    let mut chat_assignment =
         decide_chat_assignment(context, &mime_parser, &parent_message, rfc724_mid, from_id).await?;
     let (to_ids, past_ids) = get_to_and_past_contact_ids(
         context,
         &mime_parser,
-        &chat_assignment,
+        &mut chat_assignment,
         &parent_message,
         incoming_origin,
     )
@@ -2698,6 +2679,9 @@ async fn lookup_or_create_adhoc_group(
     let to_ids: Vec<ContactId> = to_ids.iter().filter_map(|x| *x).collect();
     let mut contact_ids = BTreeSet::<ContactId>::from_iter(to_ids.iter().copied());
     contact_ids.insert(from_id);
+    if mime_parser.was_encrypted() {
+        contact_ids.remove(&ContactId::SELF);
+    }
     let trans_fn = |t: &mut rusqlite::Transaction| {
         t.pragma_update(None, "query_only", "0")?;
         t.execute(
@@ -3099,7 +3083,9 @@ async fn apply_group_changes(
             if !from_id.is_special() {
                 new_members.insert(from_id);
             }
-
+            if mime_parser.was_encrypted() && chat.grpid.is_empty() {
+                new_members.remove(&ContactId::SELF);
+            }
             context
                 .sql
                 .transaction(|transaction| {
@@ -3173,6 +3159,10 @@ async fn apply_group_changes(
                 new_members.remove(&removed_id);
             }
 
+            if mime_parser.was_encrypted() && chat.grpid.is_empty() {
+                new_members.remove(&ContactId::SELF);
+            }
+
             if new_members != chat_contacts {
                 chat::update_chat_contacts_table(
                     context,
@@ -3814,11 +3804,15 @@ async fn create_adhoc_group(
     to_ids: &[ContactId],
     grpname: &str,
 ) -> Result<Option<(ChatId, Blocked)>> {
-    let mut member_ids: Vec<ContactId> = to_ids.to_vec();
+    let mut member_ids: Vec<ContactId> = to_ids
-    if !member_ids.contains(&(from_id)) {
+        .iter()
+        .copied()
+        .filter(|&id| id != ContactId::SELF)
+        .collect();
+    if from_id != ContactId::SELF && !member_ids.contains(&from_id) {
         member_ids.push(from_id);
     }
-    if !member_ids.contains(&(ContactId::SELF)) {
+    if !mime_parser.was_encrypted() {
         member_ids.push(ContactId::SELF);
     }
 

src/receive_imf/receive_imf_tests.rs

@@ -4,8 +4,9 @@ use tokio::fs;
 
 use super::*;
 use crate::chat::{
-    ChatItem, ChatVisibility, add_contact_to_chat, add_to_chat_contacts_table, create_group,
+    CantSendReason, ChatItem, ChatVisibility, add_contact_to_chat, add_to_chat_contacts_table,
-    get_chat_contacts, get_chat_msgs, is_contact_in_chat, remove_contact_from_chat, send_text_msg,
+    create_group, get_chat_contacts, get_chat_msgs, is_contact_in_chat, remove_contact_from_chat,
+    send_text_msg,
 };
 use crate::chatlist::Chatlist;
 use crate::constants::DC_GCL_FOR_FORWARDING;
@@ -5106,6 +5107,42 @@ async fn test_dont_verify_by_verified_by_unknown() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_recv_outgoing_msg_before_securejoin() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let a0 = &tcm.alice().await;
+    let a1 = &tcm.alice().await;
+    let bob = &tcm.bob().await;
+
+    tcm.execute_securejoin(bob, a0).await;
+    let chat_id_a0_bob = a0.create_chat_id(bob).await;
+    let sent_msg = a0.send_text(chat_id_a0_bob, "Hi").await;
+    let msg_a1 = a1.recv_msg(&sent_msg).await;
+    assert!(msg_a1.get_showpadlock());
+    let chat_a1 = Chat::load_from_db(a1, msg_a1.chat_id).await?;
+    assert_eq!(chat_a1.typ, Chattype::Group);
+    assert!(!chat_a1.is_encrypted(a1).await?);
+    assert_eq!(
+        chat::get_chat_contacts(a1, chat_a1.id).await?,
+        [a1.add_or_lookup_address_contact_id(bob).await]
+    );
+    assert_eq!(
+        chat_a1.why_cant_send(a1).await?,
+        Some(CantSendReason::NotAMember)
+    );
+
+    let sent_msg = a0.send_text(chat_id_a0_bob, "Hi again").await;
+    let msg_a1 = a1.recv_msg(&sent_msg).await;
+    assert!(msg_a1.get_showpadlock());
+    assert_eq!(msg_a1.chat_id, chat_a1.id);
+    let chat_a1 = Chat::load_from_db(a1, msg_a1.chat_id).await?;
+    assert_eq!(
+        chat_a1.why_cant_send(a1).await?,
+        Some(CantSendReason::NotAMember)
+    );
+    Ok(())
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_sanitize_filename_in_received() -> Result<()> {
     let alice = &TestContext::new_alice().await;
@@ -5365,17 +5402,15 @@ async fn test_encrypted_adhoc_group_message() -> Result<()> {
     assert_eq!(chat.is_encrypted(bob).await?, false);
 
     let contact_ids = get_chat_contacts(bob, chat.id).await?;
-    assert_eq!(contact_ids.len(), 3);
+    assert_eq!(contact_ids.len(), 2);
-    assert!(chat.is_self_in_chat(bob).await?);
+    assert!(!chat.is_self_in_chat(bob).await?);
 
     // Since the group is unencrypted, all contacts have
     // to be address-contacts.
     for contact_id in contact_ids {
         let contact = Contact::get_by_id(bob, contact_id).await?;
-        if contact_id != ContactId::SELF {
         assert_eq!(contact.is_key_contact(), false);
     }
-    }
 
     // `from_id` of the message corresponds to key-contact of Alice
     // even though the message is assigned to unencrypted chat.