diff --git a/CHANGELOG.md b/CHANGELOG.md index 533cea160..2967574e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ ### Changes +- If an email has multiple From addresses, handle this as if there was + no From address, to prevent from forgery attacks. Also, improve + handling of emails with invalid From addresses in general #3667 + ### API-Changes ### Fixes diff --git a/src/authres.rs b/src/authres.rs index afa5338f1..5620b1cb4 100644 --- a/src/authres.rs +++ b/src/authres.rs @@ -13,6 +13,8 @@ use once_cell::sync::Lazy; use crate::config::Config; use crate::context::Context; use crate::headerdef::HeaderDef; +use crate::mimeparser; +use crate::mimeparser::ParserErrorExt; use crate::tools::time; use crate::tools::EmailAddress; @@ -30,20 +32,23 @@ pub(crate) async fn handle_authres( mail: &ParsedMail<'_>, from: &str, message_time: i64, -) -> Result { +) -> mimeparser::ParserResult { let from_domain = match EmailAddress::new(from) { Ok(email) => email.domain, Err(e) => { - warn!(context, "invalid email {:#}", e); // This email is invalid, but don't return an error, we still want to // add a stub to the database so that it's not downloaded again - return Ok(DkimResults::default()); + return Err(anyhow::format_err!("invalid email {}: {:#}", from, e)).map_err_malformed(); } }; let authres = parse_authres_headers(&mail.get_headers(), &from_domain); - update_authservid_candidates(context, &authres).await?; - compute_dkim_results(context, authres, &from_domain, message_time).await + update_authservid_candidates(context, &authres) + .await + .map_err_sql()?; + compute_dkim_results(context, authres, &from_domain, message_time) + .await + .map_err_sql() } #[derive(Default, Debug)] @@ -574,7 +579,7 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ file.read_to_end(&mut bytes).await.unwrap(); let mail = mailparse::parse_mail(&bytes)?; - let from = &mimeparser::get_from(&mail.headers)[0].addr; + let from = &mimeparser::get_from(&mail.headers).unwrap().addr; let res = handle_authres(&t, &mail, from, time()).await?; assert!(res.allow_keychange); @@ -586,7 +591,7 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ file.read_to_end(&mut bytes).await.unwrap(); let mail = mailparse::parse_mail(&bytes)?; - let from = &mimeparser::get_from(&mail.headers)[0].addr; + let from = &mimeparser::get_from(&mail.headers).unwrap().addr; let res = handle_authres(&t, &mail, from, time()).await?; if !res.allow_keychange { @@ -637,9 +642,10 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ // Even if the format is wrong and parsing fails, handle_authres() shouldn't // return an Err because this would prevent the message from being added // to the database and downloaded again and again - let bytes = b"Authentication-Results: dkim="; + let bytes = b"From: invalid@from.com +Authentication-Results: dkim="; let mail = mailparse::parse_mail(bytes).unwrap(); - handle_authres(&t, &mail, "invalidfrom.com", time()) + handle_authres(&t, &mail, "invalid@rom.com", time()) .await .unwrap(); } diff --git a/src/decrypt.rs b/src/decrypt.rs index af4c18adb..cf3e66b98 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -4,7 +4,6 @@ use std::collections::HashSet; use anyhow::{Context as _, Result}; use mailparse::ParsedMail; -use mailparse::SingleInfo; use crate::aheader::Aheader; use crate::authres; @@ -14,6 +13,7 @@ use crate::context::Context; use crate::key::{DcKey, Fingerprint, SignedPublicKey, SignedSecretKey}; use crate::keyring::Keyring; use crate::log::LogExt; +use crate::mimeparser::{self, ParserErrorExt}; use crate::peerstate::Peerstate; use crate::pgp; @@ -56,18 +56,12 @@ pub async fn try_decrypt( .await } -pub async fn prepare_decryption( +pub(crate) async fn prepare_decryption( context: &Context, mail: &ParsedMail<'_>, - from: &[SingleInfo], + from: &str, message_time: i64, -) -> Result { - let from = if let Some(f) = from.first() { - &f.addr - } else { - return Ok(DecryptionInfo::default()); - }; - +) -> mimeparser::ParserResult { let autocrypt_header = Aheader::from_headers(from, &mail.headers) .ok_or_log_msg(context, "Failed to parse Autocrypt header") .flatten(); @@ -82,7 +76,8 @@ pub async fn prepare_decryption( // Disallowing keychanges is disabled for now: true, // dkim_results.allow_keychange, ) - .await?; + .await + .map_err_sql()?; Ok(DecryptionInfo { from: from.to_string(), diff --git a/src/imap.rs b/src/imap.rs index 5ce47ecda..bf5c9e312 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -56,6 +56,8 @@ use session::Session; use self::select_folder::NewlySelected; +pub(crate) const GENERATED_PREFIX: &str = "GEN_"; + #[derive(Debug, Display, Clone, Copy, PartialEq, Eq)] pub enum ImapActionResult { Failed, @@ -795,7 +797,7 @@ impl Imap { }; // Get the Message-ID or generate a fake one to identify the message in the database. - let message_id = prefetch_get_message_id(&headers).unwrap_or_else(create_id); + let message_id = prefetch_get_or_create_message_id(&headers); let target = match target_folder(context, folder, is_spam_folder, &headers).await? { Some(config) => match context.get_config(config).await? { @@ -1278,7 +1280,7 @@ impl Imap { let msg = fetch?; match get_fetch_headers(&msg) { Ok(headers) => { - if let Some(from) = mimeparser::get_from(&headers).first() { + if let Some(from) = mimeparser::get_from(&headers) { if context.is_self_addr(&from.addr).await? { result.extend(mimeparser::get_recipients(&headers)); } @@ -1448,7 +1450,7 @@ impl Imap { .context("we checked that message has body right above, but it has vanished")?; let is_seen = msg.flags().any(|flag| flag == Flag::Seen); - let rfc724_mid = if let Some(rfc724_mid) = &uid_message_ids.get(&server_uid) { + let rfc724_mid = if let Some(rfc724_mid) = uid_message_ids.get(&server_uid) { rfc724_mid } else { warn!( @@ -1456,7 +1458,7 @@ impl Imap { "No Message-ID corresponding to UID {} passed in uid_messsage_ids", server_uid ); - "" + continue; }; match receive_imf_inner( &context, @@ -1754,7 +1756,7 @@ async fn should_move_out_of_spam( } else { // No chat found. let (from_id, blocked_contact, _origin) = - from_field_to_contact_id(context, &mimeparser::get_from(headers), true).await?; + from_field_to_contact_id(context, mimeparser::get_from(headers).as_ref(), true).await?; if blocked_contact { // Contact is blocked, leave the message in spam. return Ok(false); @@ -1974,13 +1976,15 @@ fn get_fetch_headers(prefetch_msg: &Fetch) -> Result> } fn prefetch_get_message_id(headers: &[mailparse::MailHeader]) -> Option { - if let Some(message_id) = headers.get_header_value(HeaderDef::XMicrosoftOriginalMessageId) { - crate::mimeparser::parse_message_id(&message_id).ok() - } else if let Some(message_id) = headers.get_header_value(HeaderDef::MessageId) { - crate::mimeparser::parse_message_id(&message_id).ok() - } else { - None - } + headers + .get_header_value(HeaderDef::XMicrosoftOriginalMessageId) + .or_else(|| headers.get_header_value(HeaderDef::MessageId)) + .and_then(|msgid| mimeparser::parse_message_id(&msgid).ok()) +} + +pub(crate) fn prefetch_get_or_create_message_id(headers: &[mailparse::MailHeader]) -> String { + prefetch_get_message_id(headers) + .unwrap_or_else(|| format!("{}{}", GENERATED_PREFIX, create_id())) } /// Returns chat by prefetched headers. @@ -2038,7 +2042,7 @@ pub(crate) async fn prefetch_should_download( .is_some(); let (_from_id, blocked_contact, origin) = - from_field_to_contact_id(context, &mimeparser::get_from(headers), true).await?; + from_field_to_contact_id(context, mimeparser::get_from(headers).as_ref(), true).await?; // prevent_rename=true as this might be a mailing list message and in this case it would be bad if we rename the contact. // (prevent_rename is the last argument of from_field_to_contact_id()) diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 1a3490ad1..7dec08f53 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -21,7 +21,6 @@ use crate::events::EventType; use crate::format_flowed::unformat_flowed; use crate::headerdef::{HeaderDef, HeaderDefMap}; use crate::key::Fingerprint; -use crate::location; use crate::message::{self, Viewtype}; use crate::param::{Param, Params}; use crate::peerstate::Peerstate; @@ -29,6 +28,7 @@ use crate::simplify::{simplify, SimplifiedText}; use crate::stock_str; use crate::sync::SyncItems; use crate::tools::{get_filemeta, parse_receive_headers, truncate_by_lines}; +use crate::{location, tools}; /// A parsed MIME message. /// @@ -46,7 +46,7 @@ pub struct MimeMessage { /// Addresses are normalized and lowercased: pub recipients: Vec, - pub from: Vec, + pub from: SingleInfo, /// Whether the From address was repeated in the signed part /// (and we know that the signer intended to send from this address) pub from_is_signed: bool, @@ -157,21 +157,50 @@ impl Default for SystemMessage { const MIME_AC_SETUP_FILE: &str = "application/autocrypt-setup"; +#[derive(Debug, thiserror::Error)] +pub(crate) enum ParserError { + #[error("{}", _0)] + Malformed(anyhow::Error), + + #[error("{:#}", _0)] + Sql(anyhow::Error), +} + +pub(crate) type ParserResult = std::result::Result; + +pub(crate) trait ParserErrorExt +where + Self: std::marker::Sized, +{ + fn map_err_malformed(self) -> ParserResult; + fn map_err_sql(self) -> ParserResult; +} + +impl> ParserErrorExt for Result { + fn map_err_malformed(self) -> ParserResult { + self.map_err(|e| ParserError::Malformed(e.into())) + } + + fn map_err_sql(self) -> ParserResult { + self.map_err(|e| ParserError::Sql(e.into())) + } +} + impl MimeMessage { pub async fn from_bytes(context: &Context, body: &[u8]) -> Result { - MimeMessage::from_bytes_with_partial(context, body, None).await + Ok(MimeMessage::from_bytes_with_partial(context, body, None).await?) } /// Parse a mime message. /// /// If `partial` is set, it contains the full message size in bytes /// and `body` contains the header only. - pub async fn from_bytes_with_partial( + pub(crate) async fn from_bytes_with_partial( context: &Context, body: &[u8], partial: Option, - ) -> Result { - let mail = mailparse::parse_mail(body)?; + ) -> ParserResult { + let mail = mailparse::parse_mail(body).map_err_malformed()?; let message_time = mail .headers @@ -198,7 +227,7 @@ impl MimeMessage { ); // Parse hidden headers. - let mimetype = mail.ctype.mimetype.parse::()?; + let mimetype = mail.ctype.mimetype.parse::().map_err_malformed()?; if mimetype.type_() == mime::MULTIPART && mimetype.subtype().as_str() == "mixed" { if let Some(part) = mail.subparts.first() { for field in &part.headers { @@ -216,11 +245,14 @@ impl MimeMessage { headers.remove("secure-join-fingerprint"); headers.remove("chat-verified"); + let from = from.context("No from in message").map_err_malformed()?; + let mut decryption_info = + prepare_decryption(context, &mail, &from.addr, message_time).await?; + // Memory location for a possible decrypted message. let mut mail_raw = Vec::new(); let mut gossiped_addr = Default::default(); let mut from_is_signed = false; - let mut decryption_info = prepare_decryption(context, &mail, &from, message_time).await?; hop_info += "\n\n"; hop_info += &decryption_info.dkim_results.to_string(); @@ -233,7 +265,7 @@ impl MimeMessage { // autocrypt message. mail_raw = raw; - let decrypted_mail = mailparse::parse_mail(&mail_raw)?; + let decrypted_mail = mailparse::parse_mail(&mail_raw).map_err_malformed()?; if std::env::var(crate::DCC_MIME_DEBUG).is_ok() { info!(context, "decrypted message mime-body:"); println!("{}", String::from_utf8_lossy(&mail_raw)); @@ -247,7 +279,8 @@ impl MimeMessage { decrypted_mail.headers.get_all_values("Autocrypt-Gossip"); gossiped_addr = update_gossip_peerstates(context, message_time, &mail, gossip_headers) - .await?; + .await + .map_err_sql()?; } // let known protected headers from the decrypted @@ -255,7 +288,7 @@ impl MimeMessage { // Signature was checked for original From, so we // do not allow overriding it. - let mut signed_from = Vec::new(); + let mut signed_from = None; // We do not want to allow unencrypted subject in encrypted emails because the user might falsely think that the subject is safe. // See . @@ -270,23 +303,21 @@ impl MimeMessage { &mut chat_disposition_notification_to, &decrypted_mail.headers, ); - if let Some(signed_from) = signed_from.first() { - if let Some(from) = from.first() { - if addr_cmp(&signed_from.addr, &from.addr) { - from_is_signed = true; - } else { - // There is a From: header in the encrypted & - // signed 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 - // - OTOH, I can't come up with an attack scenario - // where this would be useful. - warn!( - context, - "From header in signed part does't match the outer one" - ); - } + if let Some(signed_from) = signed_from { + if addr_cmp(&signed_from.addr, &from.addr) { + from_is_signed = true; + } else { + // There is a From: header in the encrypted & + // signed 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 + // - OTOH, I can't come up with an attack scenario + // where this would be useful. + warn!( + context, + "From header in signed part does't match the outer one" + ); } } @@ -302,7 +333,10 @@ impl MimeMessage { // && decryption_info.dkim_results.allow_keychange { peerstate.degrade_encryption(message_time); - peerstate.save_to_db(&context.sql, false).await?; + peerstate + .save_to_db(&context.sql, false) + .await + .map_err_sql()?; } } (Ok(mail), HashSet::new(), false) @@ -346,11 +380,15 @@ impl MimeMessage { Some(org_bytes) => { parser .create_stub_from_partial_download(context, org_bytes) - .await?; + .await + .map_err_sql()?; } None => match mail { Ok(mail) => { - parser.parse_mime_recursive(context, &mail, false).await?; + parser + .parse_mime_recursive(context, &mail, false) + .await + .map_err_malformed()?; } Err(err) => { let msg_body = stock_str::cant_decrypt_msg_body(context).await; @@ -371,7 +409,7 @@ impl MimeMessage { parser.maybe_remove_bad_parts(); parser.maybe_remove_inline_mailinglist_footer(); parser.heuristically_parse_ndn(context).await; - parser.parse_headers(context).await?; + parser.parse_headers(context).await.map_err_malformed()?; // Disallowing keychanges is disabled for now // if !decryption_info.dkim_results.allow_keychange { @@ -389,11 +427,14 @@ impl MimeMessage { parser.decoded_data = mail_raw; } - crate::peerstate::maybe_do_aeap_transition(context, &mut decryption_info, &parser).await?; + crate::peerstate::maybe_do_aeap_transition(context, &mut decryption_info, &parser) + .await + .map_err_sql()?; if let Some(peerstate) = decryption_info.peerstate { peerstate .handle_fingerprint_change(context, message_time) - .await?; + .await + .map_err_sql()?; } Ok(parser) @@ -582,21 +623,18 @@ impl MimeMessage { // See if an MDN is requested from the other side if !self.decrypting_failed && !self.parts.is_empty() { if let Some(ref dn_to) = self.chat_disposition_notification_to { - if let Some(from) = self.from.get(0) { - // Check that the message is not outgoing. - if !context.is_self_addr(&from.addr).await? { - if from.addr.to_lowercase() == dn_to.addr.to_lowercase() { - if let Some(part) = self.parts.last_mut() { - part.param.set_int(Param::WantsMdn, 1); - } - } else { - warn!( - context, - "{} requested a read receipt to {}, ignoring", - from.addr, - dn_to.addr - ); + // Check that the message is not outgoing. + let from = &self.from.addr; + if !context.is_self_addr(from).await? { + if from.to_lowercase() == dn_to.addr.to_lowercase() { + if let Some(part) = self.parts.last_mut() { + part.param.set_int(Param::WantsMdn, 1); } + } else { + warn!( + context, + "{} requested a read receipt to {}, ignoring", from, dn_to.addr + ); } } } @@ -1226,7 +1264,7 @@ impl MimeMessage { context: &Context, headers: &mut HashMap, recipients: &mut Vec, - from: &mut Vec, + from: &mut Option, list_post: &mut Option, chat_disposition_notification_to: &mut Option, fields: &[mailparse::MailHeader<'_>], @@ -1255,7 +1293,7 @@ impl MimeMessage { *recipients = recipients_new; } let from_new = get_from(fields); - if !from_new.is_empty() { + if from_new.is_some() { *from = from_new; } let list_post_new = get_list_post(fields); @@ -1800,8 +1838,9 @@ pub(crate) fn get_recipients(headers: &[MailHeader]) -> Vec { } /// Returned addresses are normalized and lowercased. -pub(crate) fn get_from(headers: &[MailHeader]) -> Vec { - get_all_addresses_from_header(headers, |header_key| header_key == "from") +pub(crate) fn get_from(headers: &[MailHeader]) -> Option { + let all = get_all_addresses_from_header(headers, |header_key| header_key == "from"); + tools::single_value(all) } /// Returned addresses are normalized and lowercased. @@ -1877,35 +1916,35 @@ mod tests { let mimemsg = MimeMessage::from_bytes(&ctx, b"From: g@c.de\n\nhi") .await .unwrap(); - let contact = mimemsg.from.first().unwrap(); + let contact = mimemsg.from; assert_eq!(contact.addr, "g@c.de"); assert_eq!(contact.display_name, None); let mimemsg = MimeMessage::from_bytes(&ctx, b"From: g@c.de \n\nhi") .await .unwrap(); - let contact = mimemsg.from.first().unwrap(); + let contact = mimemsg.from; assert_eq!(contact.addr, "g@c.de"); assert_eq!(contact.display_name, None); let mimemsg = MimeMessage::from_bytes(&ctx, b"From: \n\nhi") .await .unwrap(); - let contact = mimemsg.from.first().unwrap(); + let contact = mimemsg.from; assert_eq!(contact.addr, "g@c.de"); assert_eq!(contact.display_name, None); let mimemsg = MimeMessage::from_bytes(&ctx, b"From: Goetz C \n\nhi") .await .unwrap(); - let contact = mimemsg.from.first().unwrap(); + let contact = mimemsg.from; assert_eq!(contact.addr, "g@c.de"); assert_eq!(contact.display_name, Some("Goetz C".to_string())); let mimemsg = MimeMessage::from_bytes(&ctx, b"From: \"Goetz C\" \n\nhi") .await .unwrap(); - let contact = mimemsg.from.first().unwrap(); + let contact = mimemsg.from; assert_eq!(contact.addr, "g@c.de"); assert_eq!(contact.display_name, Some("Goetz C".to_string())); @@ -1913,7 +1952,7 @@ mod tests { MimeMessage::from_bytes(&ctx, b"From: =?utf-8?q?G=C3=B6tz?= C \n\nhi") .await .unwrap(); - let contact = mimemsg.from.first().unwrap(); + let contact = mimemsg.from; assert_eq!(contact.addr, "g@c.de"); assert_eq!(contact.display_name, Some("Götz C".to_string())); @@ -1923,7 +1962,7 @@ mod tests { MimeMessage::from_bytes(&ctx, b"From: \"=?utf-8?q?G=C3=B6tz?= C\" \n\nhi") .await .unwrap(); - let contact = mimemsg.from.first().unwrap(); + let contact = mimemsg.from; assert_eq!(contact.addr, "g@c.de"); assert_eq!(contact.display_name, Some("Götz C".to_string())); } @@ -2155,14 +2194,9 @@ mod tests { test1\n\ "; - let mimeparser = MimeMessage::from_bytes(&context.ctx, &raw[..]) - .await - .unwrap(); + let mimeparser = MimeMessage::from_bytes_with_partial(&context.ctx, &raw[..], None).await; - let of = &mimeparser.from[0]; - assert_eq!(of.addr, "hello@one.org"); - - assert!(mimeparser.chat_disposition_notification_to.is_none()); + assert!(matches!(mimeparser, Err(ParserError::Malformed(_)))); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -2201,7 +2235,7 @@ mod tests { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_mimeparser_with_context() { let context = TestContext::new().await; - let raw = b"From: hello\n\ + let raw = b"From: hello@example.org\n\ Content-Type: multipart/mixed; boundary=\"==break==\";\n\ Subject: outer-subject\n\ Secure-Join-Group: no\n\ diff --git a/src/peerstate.rs b/src/peerstate.rs index 694d9e0e3..165fd05fc 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -617,10 +617,9 @@ pub async fn maybe_do_aeap_transition( mime_parser: &crate::mimeparser::MimeMessage, ) -> Result<()> { if let Some(peerstate) = &mut info.peerstate { - if let Some(from) = mime_parser.from.first() { - // If the from addr is different from the peerstate address we know, - // we may want to do an AEAP transition. - if !addr_cmp(&peerstate.addr, &from.addr) + // If the from addr is different from the peerstate address we know, + // we may want to do an AEAP transition. + if !addr_cmp(&peerstate.addr, &mime_parser.from.addr) // Check if it's a chat message; we do this to avoid // some accidental transitions if someone writes from multiple // addresses with an MUA. @@ -636,31 +635,30 @@ pub async fn maybe_do_aeap_transition( // to the attacker's address, allowing for easier phishing. && mime_parser.from_is_signed && info.message_time > peerstate.last_seen - { - // Add info messages to chats with this (verified) contact - // - peerstate - .handle_setup_change( - context, - info.message_time, - PeerstateChange::Aeap(info.from.clone()), - ) - .await?; + { + // Add info messages to chats with this (verified) contact + // + peerstate + .handle_setup_change( + context, + info.message_time, + PeerstateChange::Aeap(info.from.clone()), + ) + .await?; - 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.to_save = Some(ToSave::All); + 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.to_save = Some(ToSave::All); - // We don't know whether a peerstate with this address already existed, or a - // new one should be created, so just try both create=false and create=true, - // and if this fails, create=true, one will succeed (this is a very cold path, - // so performance doesn't really matter). - peerstate.save_to_db(&context.sql, true).await?; - peerstate.save_to_db(&context.sql, false).await?; - } + // We don't know whether a peerstate with this address already existed, or a + // new one should be created, so just try both create=false and create=true, + // and if this fails, create=true, one will succeed (this is a very cold path, + // so performance doesn't really matter). + peerstate.save_to_db(&context.sql, true).await?; + peerstate.save_to_db(&context.sql, false).await?; } } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index d1fe9c18b..2286c040c 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -13,7 +13,6 @@ use regex::Regex; use crate::chat::{self, Chat, ChatId, ChatIdBlocked, ProtectionStatus}; use crate::config::Config; use crate::constants::{Blocked, Chattype, ShowEmails, DC_CHAT_ID_TRASH}; -use crate::contact; use crate::contact::{ may_be_valid_addr, normalize_name, Contact, ContactId, Origin, VerifiedStatus, }; @@ -22,14 +21,14 @@ use crate::download::DownloadState; use crate::ephemeral::{stock_ephemeral_timer_changed, Timer as EphemeralTimer}; use crate::events::EventType; use crate::headerdef::{HeaderDef, HeaderDefMap}; -use crate::imap::markseen_on_imap_table; +use crate::imap::{markseen_on_imap_table, GENERATED_PREFIX}; use crate::location; use crate::log::LogExt; use crate::message::{ self, rfc724_mid_exists, Message, MessageState, MessengerMessage, MsgId, Viewtype, }; use crate::mimeparser::{ - parse_message_id, parse_message_ids, AvatarAction, MailinglistType, MimeMessage, SystemMessage, + parse_message_ids, AvatarAction, MailinglistType, MimeMessage, ParserError, SystemMessage, }; use crate::param::{Param, Params}; use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus}; @@ -37,7 +36,8 @@ use crate::reaction::{set_msg_reaction, Reaction}; use crate::securejoin::{self, handle_securejoin_handshake, observe_securejoin_on_other_device}; use crate::sql; use crate::stock_str; -use crate::tools::{create_id, extract_grpid_from_rfc724_mid, smeared_time}; +use crate::tools::{extract_grpid_from_rfc724_mid, smeared_time}; +use crate::{contact, imap}; /// This is the struct that is returned after receiving one email (aka MIME message). /// @@ -66,11 +66,7 @@ pub async fn receive_imf( seen: bool, ) -> Result> { let mail = parse_mail(imf_raw).context("can't parse mail")?; - let rfc724_mid = mail - .headers - .get_header_value(HeaderDef::MessageId) - .and_then(|msgid| parse_message_id(&msgid).ok()) - .unwrap_or_else(create_id); + let rfc724_mid = imap::prefetch_get_or_create_message_id(&mail.headers); receive_imf_inner(context, &rfc724_mid, imf_raw, seen, None, false).await } @@ -105,10 +101,33 @@ pub(crate) async fn receive_imf_inner( let mut mime_parser = match MimeMessage::from_bytes_with_partial(context, imf_raw, is_partial_download).await { - Err(err) => { + Err(ParserError::Malformed(err)) => { warn!(context, "receive_imf: can't parse MIME: {}", err); - return Ok(None); + + let msg_ids; + if !rfc724_mid.starts_with(GENERATED_PREFIX) { + let row_id = context + .sql + .execute( + "INSERT INTO msgs(rfc724_mid, chat_id) VALUES (?,?)", + paramsv![rfc724_mid, DC_CHAT_ID_TRASH], + ) + .await?; + msg_ids = vec![MsgId::new(u32::try_from(row_id)?)]; + } else { + return Ok(None); + // We don't have an rfc724_mid, there's no point in adding a trash entry + } + + return Ok(Some(ReceivedMsg { + chat_id: DC_CHAT_ID_TRASH, + state: MessageState::Undefined, + sort_timestamp: 0, + msg_ids, + needs_delete_job: false, + })); } + Err(ParserError::Sql(err)) => return Err(err), Ok(mime_parser) => mime_parser, }; @@ -141,7 +160,6 @@ pub(crate) async fn receive_imf_inner( None }; - // the function returns the number of created messages in the database let prevent_rename = mime_parser.is_mailinglist_message() || mime_parser.get_header(HeaderDef::Sender).is_some(); @@ -154,7 +172,7 @@ pub(crate) async fn receive_imf_inner( // If this is a mailing list email (i.e. list_id_header is some), don't change the displayname because in // a mailing list the sender displayname sometimes does not belong to the sender email address. let (from_id, _from_id_blocked, incoming_origin) = - from_field_to_contact_id(context, &mime_parser.from, prevent_rename).await?; + from_field_to_contact_id(context, Some(&mime_parser.from), prevent_rename).await?; let incoming = from_id != ContactId::SELF; @@ -168,7 +186,6 @@ pub(crate) async fn receive_imf_inner( } else { Origin::IncomingUnknownTo }, - prevent_rename, ) .await?; @@ -353,28 +370,33 @@ pub(crate) async fn receive_imf_inner( /// * `prevent_rename`: passed through to `add_or_lookup_contacts_by_address_list()` pub async fn from_field_to_contact_id( context: &Context, - from_address_list: &[SingleInfo], + from: Option<&SingleInfo>, prevent_rename: bool, ) -> Result<(ContactId, bool, Origin)> { - let from_ids = add_or_lookup_contacts_by_address_list( + let from = match from { + Some(f) => f, + None => { + warn!(context, "mail has an empty From header"); + return Ok((ContactId::UNDEFINED, false, Origin::Unknown)); + } + }; + + let display_name = if prevent_rename { + Some("") + } else { + from.display_name.as_deref() + }; + let from_id = add_or_lookup_contact_by_addr( context, - from_address_list, + display_name, + &from.addr, Origin::IncomingUnknownFrom, - prevent_rename, ) .await?; - if from_ids.contains(&ContactId::SELF) { + if from_id == ContactId::SELF { Ok((ContactId::SELF, false, Origin::OutgoingBcc)) - } else if !from_ids.is_empty() { - if from_ids.len() > 1 { - warn!( - context, - "mail has more than one From address, only using first: {:?}", from_address_list - ); - } - let from_id = from_ids.get(0).cloned().unwrap_or_default(); - + } else { let mut from_id_blocked = false; let mut incoming_origin = Origin::Unknown; if let Ok(contact) = Contact::load_from_db(context, from_id).await { @@ -382,13 +404,6 @@ pub async fn from_field_to_contact_id( incoming_origin = contact.origin; } Ok((from_id, from_id_blocked, incoming_origin)) - } else { - warn!( - context, - "mail has an empty From header: {:?}", from_address_list - ); - - Ok((ContactId::UNDEFINED, false, Origin::Unknown)) } } @@ -570,9 +585,10 @@ async fn add_parts( if chat.is_protected() { let s = stock_str::unknown_sender_for_chat(context).await; mime_parser.repl_msg_by_error(&s); - } else if let Some(from) = mime_parser.from.first() { + } else { // In non-protected chats, just mark the sender as overridden. Therefore, the UI will prepend `~` // to the sender's name, indicating to the user that he/she is not part of the group. + let from = &mime_parser.from; let name: &str = from.display_name.as_ref().unwrap_or(&from.addr); for part in mime_parser.parts.iter_mut() { part.param.set(Param::OverrideSenderDisplayname, name); @@ -637,11 +653,9 @@ async fn add_parts( // if contact renaming is prevented (for mailinglists and bots), // we use name from From:-header as override name if prevent_rename { - if let Some(from) = mime_parser.from.first() { - if let Some(name) = &from.display_name { - for part in mime_parser.parts.iter_mut() { - part.param.set(Param::OverrideSenderDisplayname, name); - } + if let Some(name) = &mime_parser.from.display_name { + for part in mime_parser.parts.iter_mut() { + part.param.set(Param::OverrideSenderDisplayname, name); } } } @@ -1801,10 +1815,8 @@ async fn create_or_lookup_mailinglist( // a usable name for these lists is in the `From` header // and we can detect these lists by a unique `ListId`-suffix. if listid.ends_with(".list-id.mcsv.net") { - if let Some(from) = mime_parser.from.first() { - if let Some(display_name) = &from.display_name { - name = display_name.clone(); - } + if let Some(display_name) = &mime_parser.from.display_name { + name = display_name.clone(); } } @@ -1824,18 +1836,15 @@ async fn create_or_lookup_mailinglist( // // this pattern is similar to mailchimp above, however, // with weaker conditions and does not overwrite existing names. - if name.is_empty() { - if let Some(from) = mime_parser.from.first() { - if from.addr.contains("noreply") - || from.addr.contains("no-reply") - || from.addr.starts_with("notifications@") - || from.addr.starts_with("newsletter@") - || listid.ends_with(".xt.local") - { - if let Some(display_name) = &from.display_name { - name = display_name.clone(); - } - } + if name.is_empty() + && (mime_parser.from.addr.contains("noreply") + || mime_parser.from.addr.contains("no-reply") + || mime_parser.from.addr.starts_with("notifications@") + || mime_parser.from.addr.starts_with("newsletter@") + || listid.ends_with(".xt.local")) + { + if let Some(display_name) = &mime_parser.from.display_name { + name = display_name.clone(); } } @@ -2233,7 +2242,6 @@ async fn add_or_lookup_contacts_by_address_list( context: &Context, address_list: &[SingleInfo], origin: Origin, - prevent_rename: bool, ) -> Result> { let mut contact_ids = HashSet::new(); for info in address_list.iter() { @@ -2241,11 +2249,7 @@ async fn add_or_lookup_contacts_by_address_list( if !may_be_valid_addr(addr) { continue; } - let display_name = if prevent_rename { - Some("") - } else { - info.display_name.as_deref() - }; + let display_name = info.display_name.as_deref(); contact_ids .insert(add_or_lookup_contact_by_addr(context, display_name, addr, origin).await?); } @@ -2288,7 +2292,7 @@ mod tests { async fn test_grpid_simple() { let context = TestContext::new().await; let raw = b"Received: (Postfix, from userid 1000); Mon, 4 Dec 2006 14:51:39 +0100 (CET)\n\ - From: hello\n\ + From: hello@example.org\n\ Subject: outer-subject\n\ In-Reply-To: \n\ References: \n\ @@ -2303,11 +2307,25 @@ mod tests { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn test_grpid_from_multiple() { + async fn test_bad_from() { let context = TestContext::new().await; let raw = b"Received: (Postfix, from userid 1000); Mon, 4 Dec 2006 14:51:39 +0100 (CET)\n\ From: hello\n\ Subject: outer-subject\n\ + In-Reply-To: \n\ + References: \n\ + \n\ + hello\x00"; + let mimeparser = MimeMessage::from_bytes_with_partial(&context.ctx, &raw[..], None).await; + assert!(matches!(mimeparser, Err(ParserError::Malformed(_)))); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_grpid_from_multiple() { + let context = TestContext::new().await; + let raw = b"Received: (Postfix, from userid 1000); Mon, 4 Dec 2006 14:51:39 +0100 (CET)\n\ + From: hello@example.org\n\ + Subject: outer-subject\n\ In-Reply-To: \n\ References: , \n\ \n\ @@ -2593,8 +2611,55 @@ mod tests { .unwrap(); let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap(); - // Check that the message was added to the database: - assert!(chats.get_msg_id(0).is_ok()); + // Check that the message is not shown to the user: + assert!(chats.is_empty()); + + // Check that the message was added to the db: + assert!(message::rfc724_mid_exists(context, "3924@example.com") + .await + .unwrap() + .is_some()); + } + + /// If there is no Message-Id header, we generate a random id. + /// But there is no point in adding a trash entry in the database + /// if the email is malformed (e.g. because `From` is missing) + /// with this random id we just generated. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_no_message_id_header() { + let t = TestContext::new_alice().await; + + let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap(); + assert!(chats.get_msg_id(0).is_err()); + + let received = receive_imf( + &t, + b"Received: (Postfix, from userid 1000); Mon, 4 Dec 2006 14:51:39 +0100 (CET)\n\ + To: bob@example.com\n\ + Subject: foo\n\ + Chat-Version: 1.0\n\ + Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ + \n\ + hello\n", + false, + ) + .await + .unwrap(); + dbg!(&received); + assert!(received.is_none()); + + assert!(!t + .sql + .exists( + "SELECT COUNT(*) FROM msgs WHERE chat_id=?;", + paramsv![DC_CHAT_ID_TRASH], + ) + .await + .unwrap()); + + let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap(); + // Check that the message is not shown to the user: + assert!(chats.is_empty()); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/src/tools.rs b/src/tools.rs index e3a29bc59..a9c0a47ea 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -670,6 +670,18 @@ pub(crate) fn parse_receive_headers(headers: &Headers) -> String { .join("\n") } +/// If `collection` contains exactly one element, return this element. +/// Otherwise, return None. +pub(crate) fn single_value(collection: impl IntoIterator) -> Option { + let mut iter = collection.into_iter(); + if let Some(value) = iter.next() { + if iter.next().is_none() { + return Some(value); + } + } + None +} + #[cfg(test)] mod tests { #![allow(clippy::indexing_slicing)]