Mirrors/chatmail-core
1
0
Fork 0
mirror of https://github.com/chatmail/core.git synced 2026-04-27 02:16:29 +03:00
Code Issues Packages Projects Releases Wiki Activity

(AEAP) Revert #3491, instead only replace contacts in verified groups (#3510)

Browse Source 
#3491 introduced a bug that your address is only replaced in the first group you write to, which was rather hard to fix. In order to be able to release something, we agreed to revert it and instead only replace the contacts in verified groups (and in broadcast lists, if the signing key is verified).

Highlights:

* Revert "Only do the AEAP transition in the chat where it happened"

This reverts commit 22f4cd7b79.

* Only do the transition for verified groups (and broadcast lists)

To be exact, only do the transition if the signing key fingerpring is
verified. And only do it in verified groups and broadcast lists

* Slightly adapt string to this change

* Changelog
This commit is contained in:
Hocuri
2022-07-16 23:03:34 +02:00
committed by GitHub
parent 5f779ca9b2
commit 9f2100deee
6 changed files with 138 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@@ -6,6 +6,7 @@
- handle drafts from mailto links in scanned QR #3492
- do not overflow ratelimiter leaky bucket #3496
- (AEAP) Add device message after you changed your address #3505
- (AEAP) Revert #3491, instead only replace contacts in verified groups #3510
### Fixes
- don't squash text parts of NDN into attachments #3497

3
deltachat-ffi/deltachat.h
View File

@@ -6377,8 +6377,9 @@ void dc_event_unref(dc_event_t* event);
/// Otherwise you might miss messages of contacts who did not get your new
/// address yet." + the link to the AEAP blog post
///
/// The UIs have to add the link:
/// As soon as there is a post about AEAP, the UIs should add it:
/// set_stock_translation(123, getString(aeap_explanation) + "\n\n" + AEAP_BLOG_LINK)
///
/// Used in a device message that explains AEAP.
#define DC_STR_AEAP_EXPLANATION_AND_LINK 123

6
src/e2ee.rs
View File

@@ -149,11 +149,13 @@ pub(crate) async fn get_autocrypt_peerstate(
// Apply Autocrypt header
if let Some(header) = autocrypt_header {
// The "from_nongossiped_fingerprint" part is for AEAP:
// The "from_verified_fingerprint" part is for AEAP:
// If we know this fingerprint from another addr,
// we may want to do a transition from this other addr
// (and keep its peerstate)
peerstate = Peerstate::from_nongossiped_fingerprint_or_addr(
// For security reasons, for now, we only do a transition
// if the fingerprint is verified.
peerstate = Peerstate::from_verified_fingerprint_or_addr(
context,
&header.public_key.fingerprint(),
from,

129
src/peerstate.rs
View File

@@ -4,13 +4,12 @@ use std::collections::HashSet;
use std::fmt;
use crate::aheader::{Aheader, EncryptPreference};
use crate::chat::{self, is_contact_in_chat, Chat, ChatId};
use crate::chat::{self, is_contact_in_chat, Chat};
use crate::chatlist::Chatlist;
use crate::constants::Chattype;
use crate::contact::{addr_cmp, Contact, Origin};
use crate::context::Context;
use crate::events::EventType;
use crate::headerdef::HeaderDef;
use crate::key::{DcKey, Fingerprint, SignedPublicKey};
use crate::message::Message;
use crate::mimeparser::SystemMessage;
@@ -166,7 +165,7 @@ impl Peerstate {
Self::from_stmt(context, query, paramsv![fp, fp, fp]).await
}
pub async fn from_nongossiped_fingerprint_or_addr(
pub async fn from_verified_fingerprint_or_addr(
context: &Context,
fingerprint: &Fingerprint,
addr: &str,
@@ -175,9 +174,9 @@ impl Peerstate {
gossip_timestamp, gossip_key, public_key_fingerprint, gossip_key_fingerprint, \
verified_key, verified_key_fingerprint \
FROM acpeerstates \
WHERE public_key_fingerprint=? \
WHERE verified_key_fingerprint=? \
OR addr=? COLLATE NOCASE \
ORDER BY public_key_fingerprint=? DESC, last_seen DESC LIMIT 1;";
ORDER BY verified_key_fingerprint=? DESC, last_seen DESC LIMIT 1;";
let fp = fingerprint.hex();
Self::from_stmt(context, query, paramsv![fp, addr, fp]).await
}
@@ -517,12 +516,22 @@ impl Peerstate {
.with_context(|| format!("contact with peerstate.addr {:?} not found", &self.addr))?;
let chats = Chatlist::try_load(context, 0, None, Some(contact_id)).await?;
let msg = match &change {
PeerstateChange::FingerprintChange => {
stock_str::contact_setup_changed(context, self.addr.clone()).await
}
PeerstateChange::Aeap(new_addr) => {
let old_contact = Contact::load_from_db(context, contact_id).await?;
stock_str::aeap_addr_changed(
context,
old_contact.get_display_name(),
&self.addr,
new_addr,
)
.await
}
};
for (chat_id, msg_id) in chats.iter() {
let msg = match &change {
PeerstateChange::FingerprintChange => {
stock_str::contact_setup_changed(context, self.addr.clone()).await
}
};
let timestamp_sort = if let Some(msg_id) = msg_id {
let lastmsg = Message::load_from_db(context, *msg_id).await?;
lastmsg.timestamp_sort
@@ -536,6 +545,36 @@ impl Peerstate {
.await?
.unwrap_or(0)
};
if let PeerstateChange::Aeap(new_addr) = &change {
let chat = Chat::load_from_db(context, *chat_id).await?;
if chat.typ == Chattype::Group && !chat.is_protected() {
// Don't add an info_msg to the group, in order not to make the user think
// that the address was automatically replaced in the group.
continue;
}
// For security reasons, for now, we only do the AEAP transition if the fingerprint
// is verified (that's what from_verified_fingerprint_or_addr() does).
// In order to not have inconsistent group membership state, we then only do the
// transition in verified groups and in broadcast lists.
if (chat.typ == Chattype::Group && chat.is_protected())
|| chat.typ == Chattype::Broadcast
{
chat::remove_from_chat_contacts_table(context, *chat_id, contact_id).await?;
let (new_contact_id, _) =
Contact::add_or_lookup(context, "", new_addr, Origin::IncomingUnknownFrom)
.await?;
if !is_contact_in_chat(context, *chat_id, new_contact_id).await? {
chat::add_to_chat_contacts_table(context, *chat_id, new_contact_id).await?;
}
context.emit_event(EventType::ChatModified(*chat_id));
}
}
chat::add_info_msg_with_cmd(
context,
*chat_id,
@@ -596,63 +635,15 @@ pub async fn maybe_do_aeap_transition(
&& mime_parser.from_is_signed
&& info.message_time > peerstate.last_seen
{
// Add an info messages to the chat
let contact_id = context
.sql
.query_get_value(
"SELECT id FROM contacts WHERE addr=? COLLATE NOCASE;",
paramsv![peerstate.addr],
// Add info messages to chats with this (verified) contact
//
peerstate
.handle_setup_change(
context,
info.message_time,
PeerstateChange::Aeap(info.from.clone()),
)
.await?
.with_context(|| {
format!("contact with addr {:?} not found", &peerstate.addr)
})?;
let old_contact = Contact::load_from_db(context, contact_id).await?;
let msg = stock_str::aeap_addr_changed(
context,
old_contact.get_display_name(),
&peerstate.addr,
&from.addr,
)
.await;
let grpid = match mime_parser.get_header(HeaderDef::ChatGroupId) {
Some(h) => h,
None => return Ok(()),
};
let (chat_id, protected, _blocked) =
match chat::get_chat_id_by_grpid(context, grpid).await? {
Some(s) => s,
None => return Ok(()),
};
if protected && !peerstate.has_verified_key(&mime_parser.signatures) {
return Ok(());
}
chat::add_info_msg(context, chat_id, &msg, info.message_time).await?;
let (new_contact_id, _) =
Contact::add_or_lookup(context, "", &from.addr, Origin::IncomingUnknownFrom)
.await?;
let chat = Chat::load_from_db(context, chat_id).await?;
if chat.typ == Chattype::Group || chat.typ == Chattype::Broadcast {
chat::remove_from_chat_contacts_table(context, chat_id, contact_id).await?;
if !is_contact_in_chat(context, chat_id, new_contact_id).await? {
chat::add_to_chat_contacts_table(context, chat_id, new_contact_id).await?;
}
context.emit_event(EventType::ChatModified(chat_id));
}
// Create a chat with the new address with the same blocked-level as the old chat
let new_chat_id =
ChatId::create_for_contact_with_blocked(context, new_contact_id, chat.blocked)
.await?;
chat::add_info_msg(context, new_chat_id, &msg, info.message_time).await?;
.await?;
peerstate.addr = info.from.clone();
let header = info.autocrypt_header.as_ref().context(
@@ -678,9 +669,9 @@ enum PeerstateChange {
/// The contact's public key fingerprint changed, likely because
/// the contact uses a new device and didn't transfer their key.
FingerprintChange,
// /// The contact changed their address to the given new address
// /// (Automatic Email Address Porting).
// Aeap(String), (currently unused)
/// The contact changed their address to the given new address
/// (Automatic Email Address Porting).
Aeap(String),
}
/// Removes duplicate peerstates from `acpeerstates` database table.

2
src/stock_str.rs
View File

@@ -337,7 +337,7 @@ pub enum StockMessage {
AeapAddrChanged = 122,
#[strum(props(
fallback = "You changed your email address from %1$s to %2$s.\n\nIf you now send a message to a group, contacts there will automatically replace the old with your new address.\n\nIt's highly advised to set up your old email provider to forward all emails to your new email address. Otherwise you might miss messages of contacts who did not get your new address yet."
fallback = "You changed your email address from %1$s to %2$s.\n\nIf you now send a message to a verified group, contacts there will automatically replace the old with your new address.\n\nIt's highly advised to set up your old email provider to forward all emails to your new email address. Otherwise you might miss messages of contacts who did not get your new address yet."
))]
AeapExplanationAndLink = 123,
}

101
src/tests/aeap.rs
View File

@@ -69,24 +69,24 @@ Message w/out In-Reply-To
}
enum ChatForTransition {
// OneToOne,
OneToOne,
GroupChat,
VerifiedGroup,
}
use ChatForTransition::*;
// #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
// async fn test_aeap_transition_0() {
// check_aeap_transition(OneToOne, false, false).await;
// }
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_0() {
check_aeap_transition(OneToOne, false, false).await;
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_1() {
check_aeap_transition(GroupChat, false, false).await;
}
// #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
// async fn test_aeap_transition_0_verified() {
// check_aeap_transition(OneToOne, true, false).await;
// }
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_0_verified() {
check_aeap_transition(OneToOne, true, false).await;
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_1_verified() {
check_aeap_transition(GroupChat, true, false).await;
@@ -96,18 +96,18 @@ async fn test_aeap_transition_2_verified() {
check_aeap_transition(VerifiedGroup, true, false).await;
}
// #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
// async fn test_aeap_transition_0_bob_knew_new_addr() {
// check_aeap_transition(OneToOne, false, true).await;
// }
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_0_bob_knew_new_addr() {
check_aeap_transition(OneToOne, false, true).await;
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_1_bob_knew_new_addr() {
check_aeap_transition(GroupChat, false, true).await;
}
// #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
// async fn test_aeap_transition_0_verified_bob_knew_new_addr() {
// check_aeap_transition(OneToOne, true, true).await;
// }
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_0_verified_bob_knew_new_addr() {
check_aeap_transition(OneToOne, true, true).await;
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_aeap_transition_1_verified_bob_knew_new_addr() {
check_aeap_transition(GroupChat, true, true).await;
@@ -184,9 +184,11 @@ async fn check_aeap_transition(
let already_new_contact = Contact::create(&bob, "Alice", "fiona@example.net")
.await
.unwrap();
chat::add_contact_to_chat(&bob, groups[0], already_new_contact)
.await
.unwrap();
if verified {
chat::add_contact_to_chat(&bob, groups[2], already_new_contact)
.await
.unwrap();
}
// groups 0 and 2 stay unpromoted (i.e. local
// on Bob's device, Alice doesn't know about them)
@@ -206,7 +208,7 @@ async fn check_aeap_transition(
tcm.section("Alice sends another message to Bob, this time from her new addr");
// No matter which chat Alice sends to, the transition should be done in all groups
let chat_to_send = match chat_for_transition {
// OneToOne => alice.create_chat(&bob).await.id,
OneToOne => alice.create_chat(&bob).await.id,
GroupChat => group1_alice,
VerifiedGroup => group3_alice.expect("No verified group"),
};
@@ -219,8 +221,7 @@ async fn check_aeap_transition(
tcm.section("Check that the AEAP transition worked");
check_that_transition_worked(
// The transition is only done in the chat where the message was sent for now:
&[recvd.chat_id],
&groups[2..],
&alice,
"alice@example.org",
ALICE_NEW_ADDR,
@@ -228,6 +229,7 @@ async fn check_aeap_transition(
&bob,
)
.await;
check_no_transition_done(&groups[0..2], "alice@example.org", &bob).await;
// Assert that the autocrypt header is also applied to the peerstate
// if the address changed
@@ -247,8 +249,7 @@ async fn check_aeap_transition(
assert_eq!(recvd.text.unwrap(), "Hello from my old addr!");
check_that_transition_worked(
// The transition is only done in the chat where the message was sent for now:
&[recvd.chat_id],
&groups[2..],
&alice,
// Note that "alice@example.org" and ALICE_NEW_ADDR are switched now:
ALICE_NEW_ADDR,
@@ -276,11 +277,19 @@ async fn check_that_transition_worked(
let members = chat::get_chat_contacts(bob, *group).await.unwrap();
// In all the groups, exactly Bob and Alice's new number are members.
// (and Alice's new number isn't in there twice)
assert_eq!(members.len(), 2);
assert_eq!(
members.len(),
2,
"Group {} has members {:?}, but should have members {:?} and {:?}",
group,
&members,
new_contact,
ContactId::SELF
);
assert!(members.contains(&new_contact));
assert!(members.contains(&ContactId::SELF));
let info_msg = get_last_info_msg(bob, *group).await;
let info_msg = get_last_info_msg(bob, *group).await.unwrap();
let expected_text =
stock_str::aeap_addr_changed(bob, name, old_alice_addr, new_alice_addr).await;
assert_eq!(info_msg.text.unwrap(), expected_text);
@@ -293,6 +302,36 @@ async fn check_that_transition_worked(
}
}
async fn check_no_transition_done(groups: &[ChatId], old_alice_addr: &str, bob: &TestContext) {
let old_contact = Contact::lookup_id_by_addr(bob, old_alice_addr, contact::Origin::Unknown)
.await
.unwrap()
.unwrap();
for group in groups {
let members = chat::get_chat_contacts(bob, *group).await.unwrap();
// In all the groups, exactly Bob and Alice's _old_ number are members.
assert_eq!(
members.len(),
2,
"Group {} has members {:?}, but should have members {:?} and {:?}",
group,
&members,
old_contact,
ContactId::SELF
);
assert!(members.contains(&old_contact));
assert!(members.contains(&ContactId::SELF));
let last_info_msg = get_last_info_msg(bob, *group).await;
assert!(
last_info_msg.is_none(),
"{:?} shouldn't be there (or it's an unrelated info msg)",
last_info_msg
);
}
}
async fn mark_as_verified(this: &TestContext, other: &TestContext) {
let other_addr = other.get_primary_self_addr().await.unwrap();
let mut peerstate = peerstate::Peerstate::from_addr(this, &other_addr)
@@ -307,16 +346,16 @@ async fn mark_as_verified(this: &TestContext, other: &TestContext) {
peerstate.save_to_db(&this.sql, false).await.unwrap();
}
async fn get_last_info_msg(t: &TestContext, chat_id: ChatId) -> Message {
async fn get_last_info_msg(t: &TestContext, chat_id: ChatId) -> Option<Message> {
let msgs = chat::get_chat_msgs(&t.ctx, chat_id, constants::DC_GCM_INFO_ONLY)
.await
.unwrap();
let msg_id = if let chat::ChatItem::Message { msg_id } = msgs.last().unwrap() {
let msg_id = if let chat::ChatItem::Message { msg_id } = msgs.last()? {
msg_id
} else {
panic!("Wrong item type");
return None;
};
Message::load_from_db(&t.ctx, *msg_id).await.unwrap()
Some(Message::load_from_db(&t.ctx, *msg_id).await.unwrap())
}
/// Test that an attacker - here Fiona - can't replay a message sent by Alice