diff --git a/src/config.rs b/src/config.rs index b55be91c64..f793af8ae1 100644 --- a/src/config.rs +++ b/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 { diff --git a/src/context.rs b/src/context.rs index 3b94c4e027..76fd7e2cd5 100644 --- a/src/context.rs +++ b/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)); diff --git a/src/mimeparser.rs b/src/mimeparser.rs index eda7f8540a..0661a8a161 100644 --- a/src/mimeparser.rs +++ b/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() { diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index cc6c8af5b5..061281c790 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/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(()) } diff --git a/src/test_utils.rs b/src/test_utils.rs index f5f245c8a3..7f0c94eeb8 100644 --- a/src/test_utils.rs +++ b/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();