From ef4d2a7ed033f2e865addb4458785b81367102b6 Mon Sep 17 00:00:00 2001 From: link2xt Date: Fri, 29 Sep 2023 13:37:29 +0000 Subject: [PATCH 1/5] api!(python): use dc_contact_get_verifier_id() get_verifier() returns a Contact rather than an address now dc_contact_get_verifier_addr() is unused. --- python/src/deltachat/contact.py | 7 +++++-- python/tests/test_0_complex_or_slow.py | 8 ++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/python/src/deltachat/contact.py b/python/src/deltachat/contact.py index 7b1ea4f7b..e28a31178 100644 --- a/python/src/deltachat/contact.py +++ b/python/src/deltachat/contact.py @@ -75,9 +75,12 @@ class Contact: """Return True if the contact is verified.""" return lib.dc_contact_is_verified(self._dc_contact) == 2 - def get_verifier(self, contact): + def get_verifier(self, contact) -> Optional["Contact"]: """Return the address of the contact that verified the contact.""" - return from_dc_charpointer(lib.dc_contact_get_verifier_addr(contact._dc_contact)) + verifier_id = lib.dc_contact_get_verifier_id(contact._dc_contact) + if verifier_id == 0: + return None + return Contact(self.account, verifier_id) def get_profile_image(self) -> Optional[str]: """Get contact profile image. diff --git a/python/tests/test_0_complex_or_slow.py b/python/tests/test_0_complex_or_slow.py index 32d9b82a0..0f2000034 100644 --- a/python/tests/test_0_complex_or_slow.py +++ b/python/tests/test_0_complex_or_slow.py @@ -1,6 +1,7 @@ import sys import pytest +import deltachat as dc class TestGroupStressTests: @@ -149,9 +150,8 @@ def test_qr_verified_group_and_chatting(acfactory, lp): assert msg.is_encrypted() lp.sec("ac2: Check that ac2 verified ac1") - # If we verified the contact ourselves then verifier addr == contact addr ac2_ac1_contact = ac2.get_contacts()[0] - assert ac2.get_self_contact().get_verifier(ac2_ac1_contact) == ac1_addr + assert ac2.get_self_contact().get_verifier(ac2_ac1_contact).id == dc.const.DC_CONTACT_ID_SELF lp.sec("ac2: send message and let ac1 read it") chat2.send_text("world") @@ -176,9 +176,9 @@ def test_qr_verified_group_and_chatting(acfactory, lp): lp.sec("ac2: Check that ac1 verified ac3 for ac2") ac2_ac1_contact = ac2.get_contacts()[0] - assert ac2.get_self_contact().get_verifier(ac2_ac1_contact) == ac1_addr + assert ac2.get_self_contact().get_verifier(ac2_ac1_contact).id == dc.const.DC_CONTACT_ID_SELF ac2_ac3_contact = ac2.get_contacts()[1] - assert ac2.get_self_contact().get_verifier(ac2_ac3_contact) == ac1_addr + assert ac2.get_self_contact().get_verifier(ac2_ac3_contact).addr == ac1_addr lp.sec("ac2: send message and let ac3 read it") chat2.send_text("hi") From 532e9cb09aeecf5d3426a5086d138019c176125c Mon Sep 17 00:00:00 2001 From: link2xt Date: Tue, 12 Sep 2023 01:03:33 +0000 Subject: [PATCH 2/5] refactor: ignore public key argument in dc_preconfigure_keypair() Public key can be extracted from the secret key file. --- deltachat-ffi/deltachat.h | 2 +- deltachat-ffi/src/lib.rs | 6 +++--- python/src/deltachat/account.py | 4 ++-- python/src/deltachat/testplugin.py | 5 ++--- python/tests/test_3_offline.py | 5 ++--- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index e35f7710b..1ef60960d 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -824,7 +824,7 @@ void dc_maybe_network (dc_context_t* context); * @param context The context as created by dc_context_new(). * @param addr The e-mail address of the user. This must match the * configured_addr setting of the context as well as the UID of the key. - * @param public_data ASCII armored public key. + * @param public_data Ignored, actual public key is extracted from secret_data. * @param secret_data ASCII armored secret key. * @return 1 on success, 0 on failure. */ diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index d9aba5b96..42ebf06b3 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -29,7 +29,7 @@ use deltachat::contact::{Contact, ContactId, Origin}; use deltachat::context::Context; use deltachat::ephemeral::Timer as EphemeralTimer; use deltachat::imex::BackupProvider; -use deltachat::key::DcKey; +use deltachat::key::{DcKey, DcSecretKey}; use deltachat::message::MsgId; use deltachat::net::read_url_blob; use deltachat::qr_code_generator::{generate_backup_qr, get_securejoin_qr_svg}; @@ -805,7 +805,7 @@ pub unsafe extern "C" fn dc_maybe_network(context: *mut dc_context_t) { pub unsafe extern "C" fn dc_preconfigure_keypair( context: *mut dc_context_t, addr: *const libc::c_char, - public_data: *const libc::c_char, + _public_data: *const libc::c_char, secret_data: *const libc::c_char, ) -> i32 { if context.is_null() { @@ -815,8 +815,8 @@ pub unsafe extern "C" fn dc_preconfigure_keypair( let ctx = &*context; block_on(async move { let addr = tools::EmailAddress::new(&to_string_lossy(addr))?; - let public = key::SignedPublicKey::from_asc(&to_string_lossy(public_data))?.0; let secret = key::SignedSecretKey::from_asc(&to_string_lossy(secret_data))?.0; + let public = secret.split_public_key()?; let keypair = key::KeyPair { addr, public, diff --git a/python/src/deltachat/account.py b/python/src/deltachat/account.py index fde031564..b53bbe7a0 100644 --- a/python/src/deltachat/account.py +++ b/python/src/deltachat/account.py @@ -195,7 +195,7 @@ class Account: assert res != ffi.NULL, f"config value not found for: {name!r}" return from_dc_charpointer(res) - def _preconfigure_keypair(self, addr: str, public: str, secret: str) -> None: + def _preconfigure_keypair(self, addr: str, secret: str) -> None: """See dc_preconfigure_keypair() in deltachat.h. In other words, you don't need this. @@ -203,7 +203,7 @@ class Account: res = lib.dc_preconfigure_keypair( self._dc_context, as_dc_charpointer(addr), - as_dc_charpointer(public), + ffi.NULL, as_dc_charpointer(secret), ) if res == 0: diff --git a/python/src/deltachat/testplugin.py b/python/src/deltachat/testplugin.py index d736984b6..944982475 100644 --- a/python/src/deltachat/testplugin.py +++ b/python/src/deltachat/testplugin.py @@ -478,10 +478,9 @@ class ACFactory: except IndexError: pass else: - fname_pub = self.data.read_path(f"key/{keyname}-public.asc") fname_sec = self.data.read_path(f"key/{keyname}-secret.asc") - if fname_pub and fname_sec: - account._preconfigure_keypair(addr, fname_pub, fname_sec) + if fname_sec: + account._preconfigure_keypair(addr, fname_sec) return True print(f"WARN: could not use preconfigured keys for {addr!r}") diff --git a/python/tests/test_3_offline.py b/python/tests/test_3_offline.py index ad30b7f55..4b040c65a 100644 --- a/python/tests/test_3_offline.py +++ b/python/tests/test_3_offline.py @@ -67,10 +67,9 @@ class TestOfflineAccountBasic: def test_preconfigure_keypair(self, acfactory, data): ac = acfactory.get_unconfigured_account() - alice_public = data.read_path("key/alice-public.asc") alice_secret = data.read_path("key/alice-secret.asc") - assert alice_public and alice_secret - ac._preconfigure_keypair("alice@example.org", alice_public, alice_secret) + assert alice_secret + ac._preconfigure_keypair("alice@example.org", alice_secret) def test_getinfo(self, acfactory): ac1 = acfactory.get_unconfigured_account() From eb624e43c08608725aec28bfc198041e3fecdf89 Mon Sep 17 00:00:00 2001 From: link2xt Date: Sat, 30 Sep 2023 18:24:16 +0000 Subject: [PATCH 3/5] refactor: remove incomplete protected header code skipping Legacy Display Part The code removed is an incomplete implementation of skipping the Legacy Display Part specified in https://www.ietf.org/archive/id/draft-autocrypt-lamps-protected-headers-02.html#section-5.2 The code does not fully implement the specification, e.g. it does not check that there are exactly two parts. Delta Chat and Thunderbird are not adding this part anyway, and it is defined as "transitional" in the draft. This also removes misplaced warning "Ignoring nested protected headers" that is printed for every incoming Delta Chat message since commit 5690c48863cadddd0e9eb4a1f5510456053e9b17 which is part of the PR . --- src/mimeparser.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 0c73d2b6f..87882396c 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -804,18 +804,6 @@ impl MimeMessage { // Boxed future to deal with recursion async move { - if mail.ctype.params.get("protected-headers").is_some() { - if mail.ctype.mimetype == "text/rfc822-headers" { - warn!( - context, - "Protected headers found in text/rfc822-headers attachment: Will be ignored.", - ); - return Ok(false); - } - - warn!(context, "Ignoring nested protected headers"); - } - enum MimeS { Multiple, Single, @@ -852,7 +840,10 @@ impl MimeMessage { self.parse_mime_recursive(context, &mail, is_related).await } - MimeS::Single => self.add_single_part_if_known(context, mail, is_related).await, + MimeS::Single => { + self.add_single_part_if_known(context, mail, is_related) + .await + } } } .boxed() From 80ca59f152e4b280b281e34e90ba24cced79c893 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Tue, 12 Sep 2023 01:18:52 -0300 Subject: [PATCH 4/5] feat: Remove extra members from the local list in sake of group membership consistency (#3782) 9bd7ab72 brings a possibility of group membership inconsistency to the original Hocuri's algo described and implemented in e12e026b in sake of security so that nobody can add themselves to a group by forging "InReplyTo" and other headers. This commit fixes the problem by removing group members locally if we see a discrepancy with the "To" list in the received message as it is better for privacy than adding absent members locally. But it shouldn't be a big problem if somebody missed a member addition, because they will likely recreate the member list from the next received message. The problem occurs only if that "somebody" managed to reply earlier. Really, it's a problem for big groups with high message rate, but let it be for now. Also: - Query chat contacts from the db only once. - Update chat contacts in the only transaction, otherwise we can just break the chat contact list halfway. - Allow classic MUA messages to remove group members if a parent message is missing. Currently it doesn't matter because unrelated messages go to new ad-hoc groups, but let this logic be outside of apply_group_changes(). Just in case if there will be a MUA preserving "Chat-Group-ID" header f.e. --- src/receive_imf.rs | 119 +++++++++++++++++++++++---------------- src/receive_imf/tests.rs | 42 ++++++++++---- 2 files changed, 100 insertions(+), 61 deletions(-) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 220174157..0e0edde5f 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1661,7 +1661,7 @@ async fn apply_group_changes( } let mut send_event_chat_modified = false; - let mut removed_id = None; + let (mut removed_id, mut added_id) = (None, None); let mut better_msg = None; // True if a Delta Chat client has explicitly added our current primary address. @@ -1672,8 +1672,9 @@ async fn apply_group_changes( false }; - 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 mut chat_contacts = HashSet::from_iter(chat::get_chat_contacts(context, chat_id).await?); + let is_from_in_chat = + !chat_contacts.contains(&ContactId::SELF) || chat_contacts.contains(&from_id); // Reject group membership changes from non-members and old changes. let allow_member_list_changes = is_from_in_chat @@ -1683,12 +1684,10 @@ async fn apply_group_changes( // Whether to rebuild the member list from scratch. let recreate_member_list = { - // Recreate member list if the message comes from a MUA as these messages do _not_ set add/remove headers. - !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. - || self_added + // 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. + self_added || match mime_parser.get_header(HeaderDef::InReplyTo) { // If we don't know the referenced message, we missed some messages. // Maybe they added/removed members, so we need to recreate our member list. @@ -1714,14 +1713,8 @@ async fn apply_group_changes( Some(stock_str::msg_del_member_local(context, removed_addr, from_id).await) }; - if let Some(contact_id) = removed_id { - if allow_member_list_changes { - // Remove a single member from the chat. - if !recreate_member_list { - chat::remove_from_chat_contacts_table(context, chat_id, contact_id).await?; - send_event_chat_modified = true; - } - } else { + if removed_id.is_some() { + if !allow_member_list_changes { info!( context, "Ignoring removal of {removed_addr:?} from {chat_id}." @@ -1734,13 +1727,11 @@ async fn apply_group_changes( better_msg = Some(stock_str::msg_add_member_local(context, added_addr, from_id).await); if allow_member_list_changes { - // Add a single member to the chat. if !recreate_member_list { if let Some(contact_id) = Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await? { - chat::add_to_chat_contacts_table(context, chat_id, &[contact_id]).await?; - send_event_chat_modified = true; + added_id = Some(contact_id); } else { warn!(context, "Added {added_addr:?} has no contact id.") } @@ -1807,46 +1798,76 @@ async fn apply_group_changes( } } - // Recreate the member list. - if recreate_member_list { - // 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. - if mime_parser.has_chat_version() { + if allow_member_list_changes { + let mut new_members = HashSet::from_iter(to_ids.iter().copied()); + new_members.insert(ContactId::SELF); + if !from_id.is_special() { + new_members.insert(from_id); + } + + if !recreate_member_list { + let diff: HashSet = + chat_contacts.difference(&new_members).copied().collect(); + // 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. + if mime_parser.has_chat_version() { + // This is what provides group membership consistency: we remove group members + // locally if we see a discrepancy with the "To" list in the received message as it + // is better for privacy than adding absent members locally. But it shouldn't be a + // big problem if somebody missed a member addition, because they will likely + // recreate the member list from the next received message. The problem occurs only + // if that "somebody" managed to reply earlier. Really, it's a problem for big + // groups with high message rate, but let it be for now. + if !diff.is_empty() { + warn!(context, "Implicit removal of {diff:?} from chat {chat_id}."); + } + new_members = chat_contacts.difference(&diff).copied().collect(); + } else { + new_members.extend(diff); + } + } + if let Some(removed_id) = removed_id { + new_members.remove(&removed_id); + } + if let Some(added_id) = added_id { + new_members.insert(added_id); + } + if recreate_member_list { + info!( + context, + "Recreating chat {chat_id} member list with {new_members:?}.", + ); + } + + if new_members != chat_contacts { + let new_members_ref = &new_members; context .sql - .execute("DELETE FROM chats_contacts WHERE chat_id=?;", (chat_id,)) + .transaction(move |transaction| { + transaction + .execute("DELETE FROM chats_contacts WHERE chat_id=?", (chat_id,))?; + for contact_id in new_members_ref { + transaction.execute( + "INSERT INTO chats_contacts (chat_id, contact_id) VALUES(?, ?)", + (chat_id, contact_id), + )?; + } + Ok(()) + }) .await?; + chat_contacts = new_members; + send_event_chat_modified = true; } - - let mut members_to_add = HashSet::new(); - members_to_add.extend(to_ids); - members_to_add.insert(ContactId::SELF); - - if !from_id.is_special() { - members_to_add.insert(from_id); - } - - if let Some(removed_id) = removed_id { - members_to_add.remove(&removed_id); - } - - info!( - context, - "Recreating chat {chat_id} with members {members_to_add:?}." - ); - - chat::add_to_chat_contacts_table(context, chat_id, &Vec::from_iter(members_to_add)).await?; - send_event_chat_modified = true; } if let Some(avatar_action) = &mime_parser.group_avatar { - if !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await? { + if !chat_contacts.contains(&ContactId::SELF) { warn!( context, "Received group avatar update for group chat {chat_id} we are not a member of." ); - } else if !chat::is_contact_in_chat(context, chat_id, from_id).await? { + } else if !chat_contacts.contains(&from_id) { warn!( context, "Contact {from_id} attempts to modify group chat {chat_id} avatar without being a member.", diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 051707627..dba3d5305 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -3369,20 +3369,15 @@ async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> { alice.recv_msg(&bob.pop_sent_msg().await).await; - // bob didn't receive the addition of fiona, but alice doesn't overwrite her own - // contact list with the one from bob which only has three members instead of four. - assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4); - - // bob removes a member. - remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?; - - alice.recv_msg(&bob.pop_sent_msg().await).await; - - // Bobs chat only has two members after the removal of blue, because he still - // didn't receive the addition of fiona. But that doesn't overwrite alice' - // memberlist. + // Bob didn't receive the addition of Fiona, so Alice must remove Fiona from the members list + // back to make their group members view consistent. assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3); + // Just a dumb check for remove_contact_from_chat(). Let's have it in this only place. + remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?; + alice.recv_msg(&bob.pop_sent_msg().await).await; + assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 2); + Ok(()) } @@ -3514,6 +3509,29 @@ async fn test_mua_cant_remove() -> Result<()> { chat::get_chat_contacts(&alice, group_chat.id).await?.len(), 4 ); + + // But if the parent message is missing, the message must goto a new ad-hoc group. + let bob_removes = receive_imf( + &alice, + b"Subject: Re: Message from alice\r\n\ + From: \r\n\ + To: , \r\n\ + Date: Mon, 12 Dec 2022 14:32:40 +0000\r\n\ + Message-ID: \r\n\ + In-Reply-To: \r\n\ + \r\n\ + Hi back!\r\n", + false, + ) + .await? + .unwrap(); + assert_ne!(bob_removes.chat_id, alice_chat.id); + let group_chat = Chat::load_from_db(&alice, bob_removes.chat_id).await?; + assert_eq!(group_chat.typ, Chattype::Group); + assert_eq!( + chat::get_chat_contacts(&alice, group_chat.id).await?.len(), + 3, + ); Ok(()) } From 53230b6eb045e383d75584e91b12d6456d541764 Mon Sep 17 00:00:00 2001 From: link2xt Date: Sun, 1 Oct 2023 00:04:45 +0000 Subject: [PATCH 5/5] chore(cargo): update webpki to fix RUSTSEC-2023-0052 --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 608db7795..bf119faf0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5448,9 +5448,9 @@ dependencies = [ [[package]] name = "webpki" -version = "0.22.1" +version = "0.22.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f0e74f82d49d545ad128049b7e88f6576df2da6b02e9ce565c6f533be576957e" +checksum = "07ecc0cd7cac091bf682ec5efa18b1cff79d617b84181f38b3951dbe135f607f" dependencies = [ "ring", "untrusted",