fix: Create new Peerstate for unencrypted message with already known Autocrypt key, but a new address

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.

src/decrypt.rs

@@ -27,12 +27,8 @@ pub fn try_decrypt(
     private_keyring: &[SignedSecretKey],
     public_keyring_for_validate: &[SignedPublicKey],
 ) -> Result<Option<(Vec<u8>, HashSet<Fingerprint>)>> {
-    let encrypted_data_part = match get_autocrypt_mime(mail)
+    let Some(encrypted_data_part) = get_encrypted_mime(mail) else {
-        .or_else(|| get_mixed_up_mime(mail))
+        return Ok(None);
-        .or_else(|| get_attachment_mime(mail))
-    {
-        None => return Ok(None),
-        Some(res) => res,
     };
 
     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<Option<Peerstate>> {
     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 allow_aeap {
-        // If we know this fingerprint from another addr,
+            // If we know this fingerprint from another addr,
-        // we may want to do a transition from this other addr
+            // we may want to do a transition from this other addr
-        // (and keep its peerstate)
+            // (and keep its peerstate)
-        // For security reasons, for now, we only do a transition
+            // For security reasons, for now, we only do a transition
-        // if the fingerprint is verified.
+            // if the fingerprint is verified.
-        peerstate = Peerstate::from_verified_fingerprint_or_addr(
+            peerstate = Peerstate::from_verified_fingerprint_or_addr(
-            context,
+                context,
-            &header.public_key.fingerprint(),
+                &header.public_key.fingerprint(),
-            from,
+                from,
-        )
+            )
-        .await?;
+            .await?;
+        } else {
+            peerstate = Peerstate::from_addr(context, from).await?;
+        }
 
         if let Some(ref mut peerstate) = peerstate {
             if addr_cmp(&peerstate.addr, from) {

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(
+        self.save_to_db_ex(sql, None).await
-            "INSERT INTO acpeerstates (
+    }
-                last_seen,
+
-                last_seen_autocrypt,
+    /// Saves the peerstate to the database.
-                prefer_encrypted,
+    ///
-                public_key,
+    /// * `old_addr`: Old address of the peerstate in case of an AEAP transition.
-                gossip_timestamp,
+    pub(crate) async fn save_to_db_ex(&self, sql: &Sql, old_addr: Option<&str>) -> Result<()> {
-                gossip_key,
+        let trans_fn = |t: &mut rusqlite::Transaction| {
-                public_key_fingerprint,
+            if let Some(old_addr) = old_addr {
-                gossip_key_fingerprint,
+                // We are doing an AEAP transition to the new address and the SQL INSERT below will
-                verified_key,
+                // save the existing peerstate as belonging to this new address. We now need to
-                verified_key_fingerprint,
+                // delete the peerstate that belongs to the current address in case if the contact
-                verifier,
+                // later wants to move back to the current address. Otherwise the old entry will be
-                secondary_verified_key,
+                // just found and updated instead of doing AEAP.
-                secondary_verified_key_fingerprint,
+                t.execute("DELETE FROM acpeerstates WHERE addr=?", (old_addr,))?;
-                secondary_verifier,
+            }
-                backward_verified_key_id,
+            t.execute(
-                addr)
+                "INSERT INTO acpeerstates (
-                VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)
+                    last_seen,
-                ON CONFLICT (addr)
+                    last_seen_autocrypt,
-                DO UPDATE SET
+                    prefer_encrypted,
-                  last_seen = excluded.last_seen,
+                    public_key,
-                  last_seen_autocrypt = excluded.last_seen_autocrypt,
+                    gossip_timestamp,
-                  prefer_encrypted = excluded.prefer_encrypted,
+                    gossip_key,
-                  public_key = excluded.public_key,
+                    public_key_fingerprint,
-                  gossip_timestamp = excluded.gossip_timestamp,
+                    gossip_key_fingerprint,
-                  gossip_key = excluded.gossip_key,
+                    verified_key,
-                  public_key_fingerprint = excluded.public_key_fingerprint,
+                    verified_key_fingerprint,
-                  gossip_key_fingerprint = excluded.gossip_key_fingerprint,
+                    verifier,
-                  verified_key = excluded.verified_key,
+                    secondary_verified_key,
-                  verified_key_fingerprint = excluded.verified_key_fingerprint,
+                    secondary_verified_key_fingerprint,
-                  verifier = excluded.verifier,
+                    secondary_verifier,
-                  secondary_verified_key = excluded.secondary_verified_key,
+                    backward_verified_key_id,
-                  secondary_verified_key_fingerprint = excluded.secondary_verified_key_fingerprint,
+                    addr)
-                  secondary_verifier = excluded.secondary_verifier,
+                    VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)
-                  backward_verified_key_id = excluded.backward_verified_key_id",
+                    ON CONFLICT (addr)
-            (
+                    DO UPDATE SET
-                self.last_seen,
+                    last_seen = excluded.last_seen,
-                self.last_seen_autocrypt,
+                    last_seen_autocrypt = excluded.last_seen_autocrypt,
-                self.prefer_encrypt as i64,
+                    prefer_encrypted = excluded.prefer_encrypted,
-                self.public_key.as_ref().map(|k| k.to_bytes()),
+                    public_key = excluded.public_key,
-                self.gossip_timestamp,
+                    gossip_timestamp = excluded.gossip_timestamp,
-                self.gossip_key.as_ref().map(|k| k.to_bytes()),
+                    gossip_key = excluded.gossip_key,
-                self.public_key_fingerprint.as_ref().map(|fp| fp.hex()),
+                    public_key_fingerprint = excluded.public_key_fingerprint,
-                self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()),
+                    gossip_key_fingerprint = excluded.gossip_key_fingerprint,
-                self.verified_key.as_ref().map(|k| k.to_bytes()),
+                    verified_key = excluded.verified_key,
-                self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()),
+                    verified_key_fingerprint = excluded.verified_key_fingerprint,
-                self.verifier.as_deref().unwrap_or(""),
+                    verifier = excluded.verifier,
-                self.secondary_verified_key.as_ref().map(|k| k.to_bytes()),
+                    secondary_verified_key = excluded.secondary_verified_key,
-                self.secondary_verified_key_fingerprint
+                    secondary_verified_key_fingerprint = excluded.secondary_verified_key_fingerprint,
-                    .as_ref()
+                    secondary_verifier = excluded.secondary_verifier,
-                    .map(|fp| fp.hex()),
+                    backward_verified_key_id = excluded.backward_verified_key_id",
-                self.secondary_verifier.as_deref().unwrap_or(""),
+                (
-                self.backward_verified_key_id,
+                    self.last_seen,
-                &self.addr,
+                    self.last_seen_autocrypt,
-            ),
+                    self.prefer_encrypt as i64,
-        )
+                    self.public_key.as_ref().map(|k| k.to_bytes()),
-        .await?;
+                    self.gossip_timestamp,
-        Ok(())
+                    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.
+            // Check if the message is encrypted and signed correctly. If it's not encrypted, it's
-            // Although checking `from_is_signed` below is sufficient, let's play it safe.
+            // 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(())

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(())
+    }
 }

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();