diff --git a/src/decrypt.rs b/src/decrypt.rs index 475f98196..bfc238311 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -27,12 +27,8 @@ pub fn try_decrypt( private_keyring: &[SignedSecretKey], public_keyring_for_validate: &[SignedPublicKey], ) -> Result, HashSet)>> { - let encrypted_data_part = match get_autocrypt_mime(mail) - .or_else(|| get_mixed_up_mime(mail)) - .or_else(|| get_attachment_mime(mail)) - { - None => return Ok(None), - Some(res) => res, + let Some(encrypted_data_part) = get_encrypted_mime(mail) else { + return Ok(None); }; decrypt_part( @@ -91,7 +87,7 @@ pub(crate) async fn prepare_decryption( }; let dkim_results = handle_authres(context, mail, from, message_time).await?; - + let allow_aeap = get_encrypted_mime(mail).is_some(); let peerstate = get_autocrypt_peerstate( context, from, @@ -99,6 +95,7 @@ pub(crate) async fn prepare_decryption( message_time, // Disallowing keychanges is disabled for now: true, // dkim_results.allow_keychange, + allow_aeap, ) .await?; @@ -127,6 +124,13 @@ pub struct DecryptionInfo { pub(crate) dkim_results: authres::DkimResults, } +/// Returns a reference to the encrypted payload of a message. +fn get_encrypted_mime<'a, 'b>(mail: &'a ParsedMail<'b>) -> Option<&'a ParsedMail<'b>> { + get_autocrypt_mime(mail) + .or_else(|| get_mixed_up_mime(mail)) + .or_else(|| get_attachment_mime(mail)) +} + /// Returns a reference to the encrypted payload of a ["Mixed /// Up"][pgpmime-message-mangling] message. /// @@ -293,24 +297,28 @@ pub(crate) async fn get_autocrypt_peerstate( autocrypt_header: Option<&Aheader>, message_time: i64, allow_change: bool, + allow_aeap: bool, ) -> Result> { let allow_change = allow_change && !context.is_self_addr(from).await?; let mut peerstate; // Apply Autocrypt header if let Some(header) = autocrypt_header { - // 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) - // 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, - ) - .await?; + if allow_aeap { + // If we know this fingerprint from another addr, + // we may want to do a transition from this other addr + // (and keep its peerstate) + // 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, + ) + .await?; + } else { + peerstate = Peerstate::from_addr(context, from).await?; + } if let Some(ref mut peerstate) = peerstate { if addr_cmp(&peerstate.addr, from) { diff --git a/src/peerstate.rs b/src/peerstate.rs index 609dde121..9d69b2cea 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -1,5 +1,7 @@ //! # [Autocrypt Peer State](https://autocrypt.org/level1.html#peer-state-management) module. +use std::mem; + use anyhow::{Context as _, Error, Result}; use num_traits::FromPrimitive; @@ -223,9 +225,10 @@ impl Peerstate { FROM acpeerstates \ WHERE verified_key_fingerprint=? \ OR addr=? COLLATE NOCASE \ - ORDER BY verified_key_fingerprint=? DESC, last_seen DESC LIMIT 1;"; + ORDER BY verified_key_fingerprint=? DESC, addr=? COLLATE NOCASE DESC, \ + last_seen DESC LIMIT 1;"; let fp = fingerprint.hex(); - Self::from_stmt(context, query, (&fp, &addr, &fp)).await + Self::from_stmt(context, query, (&fp, addr, &fp, addr)).await } async fn from_stmt( @@ -523,65 +526,82 @@ impl Peerstate { /// Saves the peerstate to the database. pub async fn save_to_db(&self, sql: &Sql) -> Result<()> { - sql.execute( - "INSERT INTO acpeerstates ( - last_seen, - last_seen_autocrypt, - prefer_encrypted, - public_key, - gossip_timestamp, - gossip_key, - public_key_fingerprint, - gossip_key_fingerprint, - verified_key, - verified_key_fingerprint, - verifier, - secondary_verified_key, - secondary_verified_key_fingerprint, - secondary_verifier, - backward_verified_key_id, - addr) - VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) - ON CONFLICT (addr) - DO UPDATE SET - last_seen = excluded.last_seen, - last_seen_autocrypt = excluded.last_seen_autocrypt, - prefer_encrypted = excluded.prefer_encrypted, - public_key = excluded.public_key, - gossip_timestamp = excluded.gossip_timestamp, - gossip_key = excluded.gossip_key, - public_key_fingerprint = excluded.public_key_fingerprint, - gossip_key_fingerprint = excluded.gossip_key_fingerprint, - verified_key = excluded.verified_key, - verified_key_fingerprint = excluded.verified_key_fingerprint, - verifier = excluded.verifier, - secondary_verified_key = excluded.secondary_verified_key, - secondary_verified_key_fingerprint = excluded.secondary_verified_key_fingerprint, - secondary_verifier = excluded.secondary_verifier, - backward_verified_key_id = excluded.backward_verified_key_id", - ( - self.last_seen, - self.last_seen_autocrypt, - self.prefer_encrypt as i64, - self.public_key.as_ref().map(|k| k.to_bytes()), - self.gossip_timestamp, - self.gossip_key.as_ref().map(|k| k.to_bytes()), - self.public_key_fingerprint.as_ref().map(|fp| fp.hex()), - self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()), - self.verified_key.as_ref().map(|k| k.to_bytes()), - self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()), - self.verifier.as_deref().unwrap_or(""), - self.secondary_verified_key.as_ref().map(|k| k.to_bytes()), - self.secondary_verified_key_fingerprint - .as_ref() - .map(|fp| fp.hex()), - self.secondary_verifier.as_deref().unwrap_or(""), - self.backward_verified_key_id, - &self.addr, - ), - ) - .await?; - Ok(()) + self.save_to_db_ex(sql, None).await + } + + /// Saves the peerstate to the database. + /// + /// * `old_addr`: Old address of the peerstate in case of an AEAP transition. + pub(crate) async fn save_to_db_ex(&self, sql: &Sql, old_addr: Option<&str>) -> Result<()> { + let trans_fn = |t: &mut rusqlite::Transaction| { + if let Some(old_addr) = old_addr { + // We are doing an AEAP transition to the new address and the SQL INSERT below will + // save the existing peerstate as belonging to this new address. We now need to + // delete the peerstate that belongs to the current address in case if the contact + // later wants to move back to the current address. Otherwise the old entry will be + // just found and updated instead of doing AEAP. + t.execute("DELETE FROM acpeerstates WHERE addr=?", (old_addr,))?; + } + t.execute( + "INSERT INTO acpeerstates ( + last_seen, + last_seen_autocrypt, + prefer_encrypted, + public_key, + gossip_timestamp, + gossip_key, + public_key_fingerprint, + gossip_key_fingerprint, + verified_key, + verified_key_fingerprint, + verifier, + secondary_verified_key, + secondary_verified_key_fingerprint, + secondary_verifier, + backward_verified_key_id, + addr) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) + ON CONFLICT (addr) + DO UPDATE SET + last_seen = excluded.last_seen, + last_seen_autocrypt = excluded.last_seen_autocrypt, + prefer_encrypted = excluded.prefer_encrypted, + public_key = excluded.public_key, + gossip_timestamp = excluded.gossip_timestamp, + gossip_key = excluded.gossip_key, + public_key_fingerprint = excluded.public_key_fingerprint, + gossip_key_fingerprint = excluded.gossip_key_fingerprint, + verified_key = excluded.verified_key, + verified_key_fingerprint = excluded.verified_key_fingerprint, + verifier = excluded.verifier, + secondary_verified_key = excluded.secondary_verified_key, + secondary_verified_key_fingerprint = excluded.secondary_verified_key_fingerprint, + secondary_verifier = excluded.secondary_verifier, + backward_verified_key_id = excluded.backward_verified_key_id", + ( + self.last_seen, + self.last_seen_autocrypt, + self.prefer_encrypt as i64, + self.public_key.as_ref().map(|k| k.to_bytes()), + self.gossip_timestamp, + self.gossip_key.as_ref().map(|k| k.to_bytes()), + self.public_key_fingerprint.as_ref().map(|fp| fp.hex()), + self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()), + self.verified_key.as_ref().map(|k| k.to_bytes()), + self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()), + self.verifier.as_deref().unwrap_or(""), + self.secondary_verified_key.as_ref().map(|k| k.to_bytes()), + self.secondary_verified_key_fingerprint + .as_ref() + .map(|fp| fp.hex()), + self.secondary_verifier.as_deref().unwrap_or(""), + self.backward_verified_key_id, + &self.addr, + ), + )?; + Ok(()) + }; + sql.transaction(trans_fn).await } /// Returns the address that verified the contact @@ -739,14 +759,16 @@ pub(crate) async fn maybe_do_aeap_transition( // some accidental transitions if someone writes from multiple // addresses with an MUA. && mime_parser.has_chat_version() - // Check if the message is signed correctly. - // Although checking `from_is_signed` below is sufficient, let's play it safe. + // Check if the message is encrypted and signed correctly. If it's not encrypted, it's + // probably from a new contact sharing the same key. && !mime_parser.signatures.is_empty() // Check if the From: address was also in the signed part of the email. // Without this check, an attacker could replay a message from Alice // to Bob. Then Bob's device would do an AEAP transition from Alice's // to the attacker's address, allowing for easier phishing. && mime_parser.from_is_signed + // DC avoids sending messages with the same timestamp, that's why `>` is here unlike in + // `Peerstate::apply_header()`. && info.message_time > peerstate.last_seen { let info = &mut mime_parser.decryption_info; @@ -761,13 +783,16 @@ pub(crate) async fn maybe_do_aeap_transition( ) .await?; + let old_addr = mem::take(&mut peerstate.addr); peerstate.addr = info.from.clone(); let header = info.autocrypt_header.as_ref().context( "Internal error: Tried to do an AEAP transition without an autocrypt header??", )?; peerstate.apply_header(header, info.message_time); - peerstate.save_to_db(&context.sql).await?; + peerstate + .save_to_db_ex(&context.sql, Some(&old_addr)) + .await?; } Ok(()) diff --git a/src/securejoin.rs b/src/securejoin.rs index 355205f73..0f1a7483d 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -759,11 +759,13 @@ mod tests { use crate::chatlist::Chatlist; use crate::constants::Chattype; use crate::contact::ContactAddress; + use crate::imex::{imex, ImexMode}; use crate::receive_imf::receive_imf; use crate::stock_str::chat_protection_enabled; use crate::test_utils::get_chat_msg; use crate::test_utils::{TestContext, TestContextManager}; use crate::tools::{EmailAddress, SystemTime}; + use std::collections::HashSet; use std::time::Duration; #[derive(PartialEq)] @@ -1430,4 +1432,55 @@ First thread."#; let contact_alice = Contact::get_by_id(&bob, contact_alice_id).await.unwrap(); assert_eq!(contact_alice.is_verified(&bob).await.unwrap(), true); } + + /// An unencrypted message with already known Autocrypt key, but sent from another address, + /// means that it's rather a new contact sharing the same key than the existing one changed its + /// address, otherwise it would already have our key to encrypt. + /// + /// This is a regression test for a bug where DC wrongly executed AEAP in this case. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_shared_bobs_key() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let bob_addr = &bob.get_config(Config::Addr).await?.unwrap(); + + tcm.execute_securejoin(bob, alice).await; + + let export_dir = tempfile::tempdir().unwrap(); + imex(bob, ImexMode::ExportSelfKeys, export_dir.path(), None).await?; + let bob2 = &TestContext::new().await; + let bob2_addr = "bob2@example.net"; + bob2.configure_addr(bob2_addr).await; + imex(bob2, ImexMode::ImportSelfKeys, export_dir.path(), None).await?; + + tcm.execute_securejoin(bob2, alice).await; + + let bob3 = &TestContext::new().await; + let bob3_addr = "bob3@example.net"; + bob3.configure_addr(bob3_addr).await; + imex(bob3, ImexMode::ImportSelfKeys, export_dir.path(), None).await?; + tcm.send_recv(bob3, alice, "hi Alice!").await; + let msg = tcm.send_recv(alice, bob3, "hi Bob3!").await; + assert!(msg.get_showpadlock()); + + let mut bob_ids = HashSet::new(); + bob_ids.insert( + Contact::lookup_id_by_addr(alice, bob_addr, Origin::Unknown) + .await? + .unwrap(), + ); + bob_ids.insert( + Contact::lookup_id_by_addr(alice, bob2_addr, Origin::Unknown) + .await? + .unwrap(), + ); + bob_ids.insert( + Contact::lookup_id_by_addr(alice, bob3_addr, Origin::Unknown) + .await? + .unwrap(), + ); + assert_eq!(bob_ids.len(), 3); + Ok(()) + } } diff --git a/src/tests/aeap.rs b/src/tests/aeap.rs index db7cedd5f..4ab2e085e 100644 --- a/src/tests/aeap.rs +++ b/src/tests/aeap.rs @@ -177,7 +177,7 @@ async fn check_aeap_transition( // Already add the new contact to one of the groups. // We can then later check that the contact isn't in the group twice. - let already_new_contact = Contact::create(&bob, "Alice", "fiona@example.net") + let already_new_contact = Contact::create(&bob, "Alice", ALICE_NEW_ADDR) .await .unwrap(); if verified { @@ -282,7 +282,10 @@ async fn check_that_transition_worked( new_contact, ContactId::SELF ); - assert!(members.contains(&new_contact)); + assert!( + members.contains(&new_contact), + "Group {group} lacks {new_contact}" + ); assert!(members.contains(&ContactId::SELF)); let info_msg = get_last_info_msg(bob, *group).await.unwrap();