From 9bd7ab728035f44e6155e5254bd51f38d9493c97 Mon Sep 17 00:00:00 2001
From: iequidoo <dgreshilov@gmail.com>
Date: Sun, 10 Sep 2023 21:20:26 -0300
Subject: [PATCH] fix: apply_group_changes(): Forbid membership changes from
 possible non-members (#3782)

It can be not good for membership consistency if we missed a message adding a member, but improves
security because nobody can add themselves to a group from now on.
---
 src/receive_imf.rs       | 30 +++++++++++++++++-------------
 src/receive_imf/tests.rs | 14 +++++++++++---
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/src/receive_imf.rs b/src/receive_imf.rs
index 2179356d7..2e51cbaa1 100644
--- a/src/receive_imf.rs
+++ b/src/receive_imf.rs
@@ -1665,17 +1665,19 @@ async fn apply_group_changes(
             false
         };
 
-    // Whether to allow any changes to the member list at all. Just reject old group changes.
-    let allow_member_list_changes = chat_id
-        .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
-        .await?;
+    let is_from_in_chat = !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await?
+        || chat::is_contact_in_chat(context, chat_id, from_id).await?;
 
-    let is_from_in_chat = chat::is_contact_in_chat(context, chat_id, from_id).await?;
+    // Reject group membership changes from non-members and old changes.
+    let allow_member_list_changes = is_from_in_chat
+        && chat_id
+            .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
+            .await?;
 
     // Whether to rebuild the member list from scratch.
-    let recreate_member_list = allow_member_list_changes && {
+    let recreate_member_list = {
         // Recreate member list if the message comes from a MUA as these messages do _not_ set add/remove headers.
-        (is_from_in_chat && !mime_parser.has_chat_version())
+        !mime_parser.has_chat_version()
             // Always recreate membership list if SELF has been added. The older versions of DC
             // don't always set "In-Reply-To" to the latest message they sent, but to the latest
             // delivered message (so it's a race), so we have this heuristic here.
@@ -1686,6 +1688,14 @@ async fn apply_group_changes(
                 Some(reply_to) => rfc724_mid_exists(context, reply_to).await?.is_none(),
                 None => false,
             }
+    } && {
+        if !allow_member_list_changes {
+            info!(
+                context,
+                "Ignoring a try to recreate member list of {chat_id} by {from_id}.",
+            );
+        }
+        allow_member_list_changes
     };
 
     if let Some(removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) {
@@ -1792,12 +1802,6 @@ async fn apply_group_changes(
 
     // Recreate the member list.
     if recreate_member_list {
-        if !is_from_in_chat {
-            warn!(
-                context,
-                "Contact {from_id} modifies group chat {chat_id} member list possibly not being a member."
-            );
-        }
         // Only delete old contacts if the sender is not a classical MUA user:
         // Classical MUA users usually don't intend to remove users from an email
         // thread, so if they removed a recipient then it was probably by accident.
diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs
index dc9f91696..051707627 100644
--- a/src/receive_imf/tests.rs
+++ b/src/receive_imf/tests.rs
@@ -3636,7 +3636,7 @@ async fn test_recreate_member_list_on_missing_add_of_self() -> Result<()> {
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn test_recreate_member_list_on_missing_add_of_others() -> Result<()> {
+async fn test_keep_member_list_if_possibly_nomember() -> Result<()> {
     let alice = TestContext::new_alice().await;
     let bob = TestContext::new_bob().await;
     let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?;
@@ -3667,7 +3667,15 @@ async fn test_recreate_member_list_on_missing_add_of_others() -> Result<()> {
     send_text_msg(&fiona, fiona_chat_id, "hi".to_string()).await?;
     bob.recv_msg(&fiona.pop_sent_msg().await).await;
 
-    // Bob missed the message adding fiona, but must recreate the member list.
-    assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 3);
+    // Bob missed the message adding fiona, but mustn't recreate the member list.
+    assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2);
+    assert!(is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?);
+    let bob_alice_contact = Contact::create(
+        &bob,
+        "alice",
+        &alice.get_config(Config::Addr).await?.unwrap(),
+    )
+    .await?;
+    assert!(is_contact_in_chat(&bob, bob_chat_id, bob_alice_contact).await?);
     Ok(())
 }