fix: Switch to original Hocuri's group membership consistency algo (#3782)(#4624) #4646

iequidoo · 2023-08-31T19:46:09Z

If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
(i.e. use the member list of the incoming message) (because we assume that we missed some messages
& have a wrong group state).
If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
member.
If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:

Remove checks for the presense of ContactId::SELF in the group. Thus all recipients of a message
take the same decision about group membership changes, no matter if they are in the group
currently. This fixes a situation when a recipient thinks it's not a member because it missed a
message about its addition before.
NOTE: But always recreate membership list if SELF has been added. The current/older versions of DC
don't always set "In-Reply-To" to the latest message they sent (it depends on timings), so we need
this heuristic currently.
Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
sender isn't in the group as per our view, because we missed some messages and our view may be
stale.

iequidoo · 2023-09-01T02:51:53Z

test_synchronize_member_list_on_group_rejoin() fails. The problem is that the current logic wrt setting In-Reply-To header has a race, if i change the test as follows:

--- a/python/tests/test_0_complex_or_slow.py                                                                                                                                                                         
+++ b/python/tests/test_0_complex_or_slow.py                                                                                                                                                                         
@@ -104,9 +104,11 @@ def test_synchronize_member_list_on_group_rejoin(self, acfactory, lp):                                                                                                                          
                                                                                                                                                                                                                     
         lp.sec("ac2: wait for a message about removal from the chat")                                                                                                                                               
         msg = ac2._evtracker.wait_next_incoming_message()                                                                                                                                                           
+        msg = ac3._evtracker.wait_next_incoming_message()                                                                                                                                                           
                                                                                                                                                                                                                     
         lp.sec("ac1: removing ac3")                                                                                                                                                                                 
         chat.remove_contact(ac3)                                                                                                                                                                                    
+        msg = ac3._evtracker.wait_next_incoming_message()                                                                                                                                                           
                                                                                                                                                                                                                     
         lp.sec("ac1: adding ac2 back")                                                                                                                                                                              
         # Group is promoted, message is sent automatically

, then it passes. Otherwise ac2 receives a message containing in In-Reply-To the message about the removal of itself (not ac3!), i.e. the known message, and doesn't rebuild the group member list. I'll make a workaround for that, otherwise the new algo won't work with the current/older versions of DC.

iequidoo · 2023-09-01T03:38:03Z

src/receive_imf.rs

+            // Always recreate membership list if SELF has been added. The current/older versions of
+            // DC don't always set "In-Reply-To" to the latest message they sent (it depends on
+            // timings), so we have this heuristic here.
+            || self_added


So, i need to bring back this check for SELF-addition, but it's the only place where this heuristic is needed. If In-Reply-To will always point to the latest message, this check could be removed someday.

iequidoo · 2023-09-02T00:36:28Z

Added 2 tests. Both fail for the previous algo

link2xt · 2023-09-05T12:47:56Z

Reference to the algorithm: #3782 (comment)

#4624 is already fixed with #4638

link2xt · 2023-09-05T12:57:18Z

src/receive_imf.rs

-        } else {
-            match mime_parser.get_header(HeaderDef::InReplyTo) {
+        (is_from_in_chat && !mime_parser.has_chat_version())
+            // Always recreate membership list if SELF has been added. The current/older versions of


What does "current/older versions" means, isn't it the same as "all versions"?

"current/older" doesn't include "future" versions :) I'm going to debug this. Looks like we're setting In-Reply-To to the Message-Id of the last successfully sent message, and the race occurs while the previous message is still being sent. If so, probably we need to carry in the new message a diff (wrt Chat-Group-Member-Added/Removed headers) to the referenced message, not to the message that is still being sent, i.e. to accumulate changes

But once it is merged, the comment becomes outdated because it becomes the "current" version.

The bug is the same or related to #4676 I guess, maybe better merge a fix for it first so there could be a reference to exact version which is buggy.

Looks related, yes, but not sure it's the same. Will try to reproduce #4676 with the fix i added here. It's a couple of LOCs, but i should move it to a separate commit

iequidoo · 2023-09-06T00:24:29Z

src/chat.rs

@@ -1051,8 +1051,11 @@ impl ChatId {
                    self,
                    MessageState::OutPreparing,
                    MessageState::OutDraft,
-                    MessageState::OutPending,


So, this makes the w/a with self_added check necessary only for the older DC versions. Checked by hand with test_0_complex_or_slow.py::TestGroupStressTests::test_synchronize_member_list_on_group_rejoin

…d messages The new message for which `parent_query()` is done may assume that it will be received in a context affected by those messages, e.g. they could add new members to a group and the new message will contain them in "To:". Anyway recipients must be prepared to orphaned references.

…3782)(#4624) - If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list (i.e. use the member list of the incoming message) (because we assume that we missed some messages & have a wrong group state). - If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this member. - If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member. That means: - Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message take the same decision about group membership changes, no matter if they are in the group currently. This fixes a situation when a recipient thinks it's not a member because it missed a message about its addition before. NOTE: But 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 need this heuristic currently. - Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the sender isn't in the group as per our view, because we missed some messages and our view may be stale.

link2xt

LGTM, especially the tests make sense so there is some progress/improvement.

There is a small comment regarding the possibility of easily adding self to the group from which the user was previously removed, but I suggest merging this PR as is and only trying to fix this in a follow up PR if at all to avoid "security" discussions from blocking this PR and keeping it unmerged.

link2xt · 2023-09-08T15:09:12Z

src/receive_imf.rs

            warn!(
                context,
-                "Contact {from_id} attempts to modify group chat {chat_id} member list without being a member."
+                "Contact {from_id} modifies group chat {chat_id} member list possibly not being a member."


Allowing someone who is not a member of the chat to modify it seems a bit dangerous.

This condition (recreate_member_list && !is_from_in_chat) is true if either self_added is true (which simply means that there is a ChatGroupMemberAdded header with our address, can be easily forged) or there is an InReplyTo pointing to unknown Message-ID (could also be easily forged, just put random string into InReplyTo).

Probably not a big deal because user can keep sending messages to all old group members anyway (e.g. from Thunderbird it is easy), the only security implication is that once someone replies to the chat they will leak who was added to the group after the user was removed.

Before this PR it's also possible for non-members of the chat to modify it, but not to recreate the member list completely. F.e. using the Chat-Group-Member-Added header. Was it done intentionally, @Septias, maybe you know? Maybe we should think more and either allow this or prohibit completely

Looks like it's just a missed check. But we can merge this PR which just makes the behaviour consistent and then open another one prohibiting chat modifications by non-members completely, e.g. just adding an additional check directly to allow_member_list_changes, to continue the discussion

iequidoo requested review from Septias, link2xt and Hocuri August 31, 2023 19:47

iequidoo mentioned this pull request Aug 31, 2023

fix: Allow membership changes by a MUA if we're not in the group (#4624) #4638

Merged

iequidoo marked this pull request as ready for review August 31, 2023 19:51

iequidoo marked this pull request as draft September 1, 2023 00:50

iequidoo commented Sep 1, 2023

View reviewed changes

iequidoo marked this pull request as ready for review September 1, 2023 03:38

iequidoo force-pushed the iequidoo/hocuri-group-consistency-algo branch from 7623e2f to 1ab69a9 Compare September 2, 2023 00:34

iequidoo force-pushed the iequidoo/hocuri-group-consistency-algo branch 2 times, most recently from 3583215 to 7beb51a Compare September 2, 2023 01:05

link2xt reviewed Sep 5, 2023

View reviewed changes

link2xt mentioned this pull request Sep 5, 2023

New group shows up if someone replies to a group message with status "failed" #4676

Closed

iequidoo force-pushed the iequidoo/hocuri-group-consistency-algo branch from 7beb51a to 8086a55 Compare September 6, 2023 00:20

iequidoo commented Sep 6, 2023

View reviewed changes

iequidoo force-pushed the iequidoo/hocuri-group-consistency-algo branch from 8086a55 to a012082 Compare September 6, 2023 00:47

iequidoo force-pushed the iequidoo/hocuri-group-consistency-algo branch from a012082 to 73aa56b Compare September 6, 2023 01:28

link2xt approved these changes Sep 8, 2023

View reviewed changes

iequidoo merged commit e12e026 into stable Sep 9, 2023
35 checks passed

iequidoo deleted the iequidoo/hocuri-group-consistency-algo branch September 9, 2023 19:22

iequidoo mentioned this pull request Sep 11, 2023

fix: apply_group_changes(): Forbid membership changes from possible non-members (#3782) #4695

Merged

link2xt mentioned this pull request Nov 9, 2023

fix: do not apply group changes to special chats #4964

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Switch to original Hocuri's group membership consistency algo (#3782)(#4624) #4646

fix: Switch to original Hocuri's group membership consistency algo (#3782)(#4624) #4646

iequidoo commented Aug 31, 2023 •

edited

Loading

iequidoo commented Sep 1, 2023

iequidoo Sep 1, 2023

iequidoo commented Sep 2, 2023

link2xt commented Sep 5, 2023

link2xt Sep 5, 2023

iequidoo Sep 5, 2023 •

edited

Loading

link2xt Sep 5, 2023

iequidoo Sep 6, 2023

iequidoo Sep 6, 2023

link2xt left a comment

link2xt Sep 8, 2023

iequidoo Sep 9, 2023

iequidoo Sep 9, 2023 •

edited

Loading

fix: Switch to original Hocuri's group membership consistency algo (#3782)(#4624) #4646

fix: Switch to original Hocuri's group membership consistency algo (#3782)(#4624) #4646

Conversation

iequidoo commented Aug 31, 2023 • edited Loading

iequidoo commented Sep 1, 2023

iequidoo Sep 1, 2023

Choose a reason for hiding this comment

iequidoo commented Sep 2, 2023

link2xt commented Sep 5, 2023

link2xt Sep 5, 2023

Choose a reason for hiding this comment

iequidoo Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

link2xt Sep 5, 2023

Choose a reason for hiding this comment

iequidoo Sep 6, 2023

Choose a reason for hiding this comment

iequidoo Sep 6, 2023

Choose a reason for hiding this comment

link2xt left a comment

Choose a reason for hiding this comment

link2xt Sep 8, 2023

Choose a reason for hiding this comment

iequidoo Sep 9, 2023

Choose a reason for hiding this comment

iequidoo Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

iequidoo commented Aug 31, 2023 •

edited

Loading

iequidoo Sep 5, 2023 •

edited

Loading

iequidoo Sep 9, 2023 •

edited

Loading