feat: add enforce_outer_from_key_alignment config for bots using addresses as identity

If enabled (default is disabled) the processing of encrypted messages changes:

- the outer From address must be listed as a relay address in the signing key,
  otherwise the message is trashed,

- the sender contact keeps the outer From address instead of adopting
  the one from the encrypted part.

Otherwise an attacker sending from their own relay-enforced address,
with their own unmodified key, puts the victim's address into the
encrypted From header and the contact address becomes the victim's.

Arguably a fix: Autocrypt headers from the encrypted part are now accepted without
comparing their address to the outer From, so that a newly added relay address
is imported from the message before the check.

src/config.rs

@@ -477,6 +477,15 @@ pub enum Config {
     /// and incoming unencrypted messages are not fetched and not processed.
     #[strum(props(default = "1"))]
     ForceEncryption,
+
+    /// If the From address in the encrypted part differs from the outer one,
+    /// keep the outer address as the contact address and trash the message
+    /// unless the outer address is a relay address of the sender's key.
+    ///
+    /// Only the outer From is enforced by the sender's relay,
+    /// so only it can be used as an identity.
+    #[strum(props(default = "0"))]
+    EnforceOuterFromKeyAlignment,
 }
 
 impl Config {

src/context.rs

@@ -1037,6 +1037,12 @@ impl Context {
                 .await?
                 .to_string(),
         );
+        res.insert(
+            "enforce_outer_from_key_alignment",
+            self.get_config_bool(Config::EnforceOuterFromKeyAlignment)
+                .await?
+                .to_string(),
+        );
 
         let elapsed = time_elapsed(&self.creation_time);
         res.insert("uptime", duration_to_str(elapsed));

src/mimeparser.rs

@@ -29,6 +29,7 @@ use crate::key::{self, DcKey, Fingerprint, SignedPublicKey};
 use crate::log::warn;
 use crate::message::{self, Message, MsgId, Viewtype, get_vcard_summary, set_msg_failed};
 use crate::param::{Param, Params};
+use crate::pgp::addresses_from_public_key;
 use crate::simplify::{SimplifiedText, simplify};
 use crate::sync::SyncItems;
 use crate::tools::{get_filemeta, parse_receive_headers, time, truncate_msg_text, validate_id};
@@ -339,6 +340,7 @@ impl MimeMessage {
         let from_is_not_self_addr = !context.is_self_addr(&from.addr).await?;
 
         let mut aheader_values = mail.headers.get_all_values(HeaderDef::Autocrypt.into());
+        let mut aheader_protected = false;
 
         let mut pre_message = if mail
             .headers
@@ -380,6 +382,7 @@ impl MimeMessage {
                     .get_all_values(HeaderDef::Autocrypt.into());
                 if !protected_aheader_values.is_empty() {
                     aheader_values = protected_aheader_values;
+                    aheader_protected = true;
                 }
 
                 expected_sender_fingerprint = expected_sender_fp;
@@ -405,7 +408,9 @@ impl MimeMessage {
             // See `get_all_addresses_from_header()` for why we take the last valid header.
             for val in aheader_values.iter().rev() {
                 autocrypt_header = match Aheader::from_str(val) {
-                    Ok(header) if addr_cmp(&header.addr, &from.addr) => Some(header),
+                    Ok(header) if aheader_protected || addr_cmp(&header.addr, &from.addr) => {
+                        Some(header)
+                    }
                     Ok(header) => {
                         warn!(
                             context,
@@ -525,8 +530,6 @@ impl MimeMessage {
             // let known protected headers from the decrypted
             // part override the unencrypted top-level
 
-            // Signature was checked for original From, so we
-            // do not allow overriding it.
             let mut inner_from = None;
 
             MimeMessage::merge_headers(
@@ -552,27 +555,49 @@ impl MimeMessage {
             }
 
             if let Some(inner_from) = inner_from {
-                if !addr_cmp(&inner_from.addr, &from.addr) {
+                if addr_cmp(&inner_from.addr, &from.addr) {
+                    from = inner_from;
+                } else {
                     // There is a From: header in the encrypted
                     // part, but it doesn't match the outer one.
                     // This _might_ be because the sender's mail server
                     // replaced the sending address, e.g. in a mailing list.
                     // Or it's because someone is doing some replay attack.
-                    // Resending encrypted messages via mailing lists
-                    // without reencrypting is not useful anyway,
-                    // so we return an error below.
                     warn!(
                         context,
                         "From header in encrypted part doesn't match the outer one",
                     );
 
-                    // Return an error from the parser.
-                    // This will result in creating a tombstone
-                    // and no further message processing
-                    // as if the MIME structure is broken.
-                    bail!("From header is forged");
+                    // If there are no valid signatures,
+                    // possibly because we don't have the public key,
+                    // the message will be associated with the address-contact.
+                    // If the address is possibly forged, we trash the message.
+                    if signatures.is_empty() {
+                        // Return an error from the parser.
+                        // This will result in creating a tombstone
+                        // and no further message processing
+                        // as if the MIME structure is broken.
+                        bail!("From header is forged");
+                    }
+
+                    if context
+                        .get_config_bool(Config::EnforceOuterFromKeyAlignment)
+                        .await?
+                    {
+                        ensure!(
+                            public_keyring.iter().any(|public_key| {
+                                signatures.contains_key(&public_key.dc_fingerprint())
+                                    && addresses_from_public_key(public_key).is_some_and(|relays| {
+                                        relays.iter().any(|relay| addr_cmp(relay, &from.addr))
+                                    })
+                            }),
+                            "Outer From address {:?} is not aligned with the sender's key",
+                            from.addr
+                        );
+                    } else {
+                        from = inner_from;
+                    }
                 }
-                from = inner_from;
             }
         }
         if signatures.is_empty() {

src/receive_imf/receive_imf_tests.rs

@@ -3456,6 +3456,8 @@ async fn test_prefer_encrypt_mutual_if_encrypted() -> Result<()> {
     Ok(())
 }
 
+/// Tests reception of encrypted and signed message with forged From header
+/// when the signature cannot be checked because the public key is not available.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_forged_from_and_no_valid_signatures() -> Result<()> {
     let mut tcm = TestContextManager::new();
@@ -4718,6 +4720,13 @@ async fn test_no_op_member_added_is_trash() -> Result<()> {
     Ok(())
 }
 
+/// Tests reception of a message with a valid signature and forged From header.
+///
+/// The message is accepted because the sender contact is associated with the key
+/// rather than the address.
+///
+/// If `enforce_outer_from_key_alignment` is set, the message is trashed instead
+/// because the outer From is not a relay address of the sender's key.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_forged_from() -> Result<()> {
     let mut tcm = TestContextManager::new();
@@ -4732,8 +4741,108 @@ async fn test_forged_from() -> Result<()> {
         .payload
         .replace("bob@example.net", "notbob@example.net");
 
-    let msg = alice.recv_msg_opt(&sent_msg).await;
-    assert!(msg.is_none());
+    let msg = alice.recv_msg(&sent_msg).await;
+    assert_eq!(msg.text, "hi!");
+    assert!(msg.get_showpadlock());
+    let contact = Contact::get_by_id(&alice, msg.from_id).await?;
+    assert!(contact.is_key_contact());
+
+    // We take the address from the encrypted part
+    // and send replies there.
+    assert_eq!(contact.get_addr(), "bob@example.net");
+
+    alice
+        .set_config_bool(Config::EnforceOuterFromKeyAlignment, true)
+        .await?;
+    chat::send_text_msg(&bob, bob_chat_id, "hi again!".to_string()).await?;
+    let mut sent_msg = bob.pop_sent_msg().await;
+    sent_msg.payload = sent_msg
+        .payload
+        .replace("bob@example.net", "notbob@example.net");
+    assert!(alice.recv_msg_opt(&sent_msg).await.is_none());
+
+    Ok(())
+}
+
+/// Tests that a forged From header in the encrypted part
+/// does not change the address of the sender contact
+/// if `enforce_outer_from_key_alignment` is set.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_forged_inner_from() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = tcm.alice().await;
+    let bob = tcm.bob().await;
+
+    alice
+        .set_config_bool(Config::EnforceOuterFromKeyAlignment, true)
+        .await?;
+    let bob_chat_id = tcm.send_recv_accept(&alice, &bob, "hi").await.chat_id;
+
+    // Bob claims the victim address in the encrypted part,
+    // while his relay keeps the outer From at his own address.
+    tcm.change_addr(&bob, "victim@example.org").await;
+    chat::send_text_msg(&bob, bob_chat_id, "hi!".to_string()).await?;
+    let mut sent_msg = bob.pop_sent_msg().await;
+    sent_msg.payload = sent_msg
+        .payload
+        .replace("victim@example.org", "bob@example.net");
+
+    let msg = alice.recv_msg(&sent_msg).await;
+    let contact = Contact::get_by_id(&alice, msg.from_id).await?;
+    assert_eq!(contact.get_addr(), "bob@example.net");
+
+    Ok(())
+}
+
+/// Tests that a sender key without relay addresses is accepted
+/// as long as the outer From matches the one in the encrypted part.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_outer_from_key_without_relays() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = tcm.alice().await;
+    let bob = tcm.bob().await;
+
+    alice
+        .set_config_bool(Config::EnforceOuterFromKeyAlignment, true)
+        .await?;
+    bob.sql
+        .execute("UPDATE transports SET is_published=0", ())
+        .await?;
+    bob.self_public_key.lock().await.take();
+
+    let msg = tcm.send_recv(&bob, &alice, "hi!").await;
+    let contact = Contact::get_by_id(&alice, msg.from_id).await?;
+    assert_eq!(contact.get_addr(), "bob@example.net");
+
+    Ok(())
+}
+
+/// Tests that a relay address added to the key after the recipient learned it
+/// is accepted as outer From, because the updated key is imported
+/// from the Autocrypt header of the encrypted part first.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_outer_from_updated_key() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = tcm.alice().await;
+    let bob = tcm.bob().await;
+
+    alice
+        .set_config_bool(Config::EnforceOuterFromKeyAlignment, true)
+        .await?;
+    tcm.send_recv_accept(&bob, &alice, "hello").await;
+    let bob_chat_id = bob.create_chat(&alice).await.id;
+
+    tcm.add_transport(&bob, "bob@relay-new.example").await;
+    chat::send_text_msg(&bob, bob_chat_id, "hi!".to_string()).await?;
+    let mut sent_msg = bob.pop_sent_msg().await;
+    sent_msg.payload = sent_msg
+        .payload
+        .replace("bob@example.net", "bob@relay-new.example");
+
+    let msg = alice.recv_msg(&sent_msg).await;
+    let contact = Contact::get_by_id(&alice, msg.from_id).await?;
+    assert_eq!(contact.get_addr(), "bob@relay-new.example");
+
     Ok(())
 }
 

src/test_utils.rs

@@ -213,13 +213,14 @@ impl TestContextManager {
         to.recv_msg(&sent).await
     }
 
-    pub async fn change_addr(&self, test_context: &TestContext, new_addr: &str) {
+    /// Adds a transport for `new_addr` without changing the primary self address.
+    pub async fn add_transport(&self, test_context: &TestContext, new_addr: &str) {
         self.section(&format!(
-            "{} changes her self address and reconfigures",
-            test_context.name()
+            "{} adds a transport for {}",
+            test_context.name(),
+            new_addr
         ));
 
-        // Insert a transport for the new address.
         test_context.sql
           .execute(
             "INSERT OR IGNORE INTO transports (addr, entered_param, configured_param) VALUES (?, ?, ?)",
@@ -230,6 +231,18 @@ impl TestContextManager {
               ),
           ).await.unwrap();
 
+        // Drop self key from cache so regen will list `new_addr` as a relay address.
+        test_context.self_public_key.lock().await.take();
+    }
+
+    pub async fn change_addr(&self, test_context: &TestContext, new_addr: &str) {
+        self.section(&format!(
+            "{} changes her self address and reconfigures",
+            test_context.name()
+        ));
+
+        self.add_transport(test_context, new_addr).await;
+
         test_context.set_primary_self_addr(new_addr).await.unwrap();
         // ensure_secret_key_exists() is called during configure
         key::ensure_secret_key_exists(test_context).await.unwrap();