From 4638c1276e76524cd6cdd9719b74b4b686a3d0f3 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 27 Dec 2024 22:20:44 -0300 Subject: [PATCH] fix: Ignore protected headers in outer message part (#6357) Delta Chat always adds protected headers to the inner encrypted or signed message, so if a protected header is only present in the outer part, it should be ignored because it's probably added by the server or somebody else. The exception is Subject because there are known cases when it's only present in the outer message part, e.g. an encrypted unsigned Thunderbird message. Also treat any Chat-* headers as protected. This fixes e.g. a case when the server injects a "Chat-Version" IMF header tricking Delta Chat into thinking that it's a chat message. Also handle "Auto-Submitted" and "Autocrypt-Setup-Message" as protected headers on the receiver side, this was apparently forgotten. --- src/decrypt.rs | 3 +- src/mimeparser.rs | 67 ++++++++++++---------------- src/mimeparser/mimeparser_tests.rs | 20 +++++++++ src/receive_imf/receive_imf_tests.rs | 18 ++++++++ 4 files changed, 68 insertions(+), 40 deletions(-) diff --git a/src/decrypt.rs b/src/decrypt.rs index 7be616c309..e3a8380e52 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -259,7 +259,8 @@ mod tests { let bob = TestContext::new_bob().await; receive_imf(&bob, attachment_mime, false).await?; let msg = bob.get_last_msg().await; - assert_eq!(msg.text, "Hello from Thunderbird!"); + // Subject should be prepended because the attachment doesn't have "Chat-Version". + assert_eq!(msg.text, "Hello, Bob! – Hello from Thunderbird!"); Ok(()) } diff --git a/src/mimeparser.rs b/src/mimeparser.rs index fe7afe15a2..80c7f0d057 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -419,26 +419,9 @@ impl MimeMessage { timestamp_sent = Self::get_timestamp_sent(&mail.headers, timestamp_sent, timestamp_rcvd); if !signatures.is_empty() { - // Remove unsigned opportunistically protected headers from messages considered - // Autocrypt-encrypted / displayed with padlock. // For "Subject" see . - for h in [ - HeaderDef::Subject, - HeaderDef::ChatGroupId, - HeaderDef::ChatGroupName, - HeaderDef::ChatGroupNameChanged, - HeaderDef::ChatGroupNameTimestamp, - HeaderDef::ChatGroupAvatar, - HeaderDef::ChatGroupMemberRemoved, - HeaderDef::ChatGroupMemberAdded, - HeaderDef::ChatGroupMemberTimestamps, - HeaderDef::ChatGroupPastMembers, - HeaderDef::ChatDelete, - HeaderDef::ChatEdit, - HeaderDef::ChatUserAvatar, - ] { - headers.remove(h.get_headername()); - } + // Other headers are removed by `MimeMessage::merge_headers()`. + headers.remove("subject"); } // let known protected headers from the decrypted @@ -1546,12 +1529,15 @@ impl MimeMessage { chat_disposition_notification_to: &mut Option, fields: &[mailparse::MailHeader<'_>], ) { + // Keep Subject so that it's displayed for signed-only messages. They are shown w/o a + // padlock anyway. + headers.retain(|k, _| !is_protected(k) || k == "subject"); for field in fields { // lowercasing all headers is technically not correct, but makes things work better let key = field.get_key().to_lowercase(); - if !headers.contains_key(&key) || // key already exists, only overwrite known types (protected headers) - is_known(&key) || key.starts_with("chat-") - { + // Don't overwrite unprotected headers, but overwrite protected ones because DKIM + // signature applies to the last headers. + if !headers.contains_key(&key) || is_protected(&key) { if key == HeaderDef::ChatDispositionNotificationTo.get_headername() { match addrparse_header(field) { Ok(addrlist) => { @@ -1966,23 +1952,26 @@ pub(crate) fn parse_message_id(ids: &str) -> Result { /// Returns true if the header overwrites outer header /// when it comes from protected headers. -fn is_known(key: &str) -> bool { - matches!( - key, - "return-path" - | "date" - | "from" - | "sender" - | "reply-to" - | "to" - | "cc" - | "bcc" - | "message-id" - | "in-reply-to" - | "references" - | "subject" - | "secure-join" - ) +fn is_protected(key: &str) -> bool { + key.starts_with("chat-") + || matches!( + key, + "return-path" + | "auto-submitted" + | "autocrypt-setup-message" + | "date" + | "from" + | "sender" + | "reply-to" + | "to" + | "cc" + | "bcc" + | "message-id" + | "in-reply-to" + | "references" + | "subject" + | "secure-join" + ) } /// Returns if the header is hidden and must be ignored in the IMF section. diff --git a/src/mimeparser/mimeparser_tests.rs b/src/mimeparser/mimeparser_tests.rs index 19a3095f60..27cf3d7359 100644 --- a/src/mimeparser/mimeparser_tests.rs +++ b/src/mimeparser/mimeparser_tests.rs @@ -1418,6 +1418,26 @@ async fn test_x_microsoft_original_message_id_precedence() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_extra_imf_chat_header() -> Result<()> { + let mut tcm = TestContextManager::new(); + let t = &tcm.alice().await; + let chat_id = t.get_self_chat().await.id; + + chat::send_text_msg(t, chat_id, "hi!".to_string()).await?; + let sent_msg = t.pop_sent_msg().await; + // Check removal of some nonexistent "Chat-*" header to protect the code from future breakages. + let payload = sent_msg + .payload + .replace("Message-ID:", "Chat-Forty-Two: 42\r\nMessage-ID:"); + let msg = MimeMessage::from_bytes(t, payload.as_bytes(), None) + .await + .unwrap(); + assert!(msg.headers.contains_key("chat-version")); + assert!(!msg.headers.contains_key("chat-forty-two")); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_long_in_reply_to() -> Result<()> { let t = TestContext::new_alice().await; diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index 4df492c679..a8d30112ea 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/src/receive_imf/receive_imf_tests.rs @@ -4052,6 +4052,24 @@ async fn test_unsigned_chat_group_hdr() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_ignore_protected_headers_in_outer_msg() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let bob_chat_id = tcm.send_recv_accept(alice, bob, "hi").await.chat_id; + send_text_msg(bob, bob_chat_id, "hi all!".to_string()).await?; + let mut sent_msg = bob.pop_sent_msg().await; + sent_msg.payload = sent_msg.payload.replace( + "Chat-Version:", + "Auto-Submitted: auto-generated\r\nChat-Version:", + ); + alice.recv_msg(&sent_msg).await; + let ab_contact = alice.add_or_lookup_contact(bob).await; + assert!(!ab_contact.is_bot()); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_sync_member_list_on_rejoin() -> Result<()> { let mut tcm = TestContextManager::new();