diff --git a/src/authentication_results_handling.rs b/src/authentication_results_handling.rs index b4b288c7c..ebeac9d5d 100644 --- a/src/authentication_results_handling.rs +++ b/src/authentication_results_handling.rs @@ -6,6 +6,7 @@ use std::collections::HashSet; use anyhow::{Context as _, Result}; use mailparse::MailHeaderMap; +use crate::config::Config; use crate::context::Context; use crate::headerdef::HeaderDef; @@ -39,15 +40,23 @@ pub(crate) fn parse_authentication_results( // Some(f) => &f.addr, // None => return Ok(HashMap::new()), // }; // TODO - let sender_domain = crate::tools::EmailAddress::new(from)?.domain; + let sender_domain = EmailAddress::new(from)?.domain; let mut header_map: HashMap> = HashMap::new(); for header_value in headers.get_all_values(HeaderDef::AuthenticationResults.into()) { // TODO there could be a comment [CFWS] before the self domain. Do we care? Probably not. - let authserv_id = header_value - .split_whitespace() - .next() - .context("Empty header")?; // TODO do we really want to return Err here if it's empty + let mut authserv_id = header_value.split(';').next().context("Empty header")?; // TODO do we really want to return Err here if it's empty + if authserv_id.contains(char::is_whitespace) { + // Outlook violates the RFC by not adding an authserv-id at all, which we notice + // because there is whitespace in the first identifier before the ';'. + // Authentication-Results-parsing still works securely because they remove incoming + // Authentication-Results headers. + // Just use an arbitrary authserv-id, it will work for Outlook, and in general, + // with providers not implementing the RFC correctly, someone can trick us + // into thinking that an incoming email is DKIM-correct, anyway. + // TODO is this comment understandable? + authserv_id = "invalidAuthservId" + } header_map .entry(authserv_id.to_string()) .or_default() @@ -84,6 +93,9 @@ fn authresults_dkim_passed(headers: &[String], sender_domain: &str) -> Result Result) -> HashSet<&str> { + config + .as_deref() + .map(|c| c.split_whitespace().collect()) + .unwrap_or_default() +} + // TODO this is only half of the algorithm we thought of; we also wanted to save how sure we are // about the authserv id. Like, a same-domain email is more trustworthy. pub(crate) async fn update_authservid_candidates( @@ -104,28 +123,20 @@ pub(crate) async fn update_authservid_candidates( return Ok(()); } - let ids_config; - if let Some(ids_config_temp) = context - .get_config(crate::config::Config::AuthservIdCandidates) - .await? - { - ids_config = ids_config_temp; - let old_ids: HashSet<_> = ids_config.split(' ').collect(); - if !old_ids.is_empty() { - new_ids = old_ids.intersection(&new_ids).copied().collect(); - } + let old_config = context.get_config(Config::AuthservIdCandidates).await?; + let old_ids = parse_authservid_candidates_config(&old_config); + if !old_ids.is_empty() { + new_ids = old_ids.intersection(&new_ids).copied().collect(); } // If there were no AuthservIdCandidates previously, just start with // the ones from the incoming email - let new_config = new_ids.into_iter().collect::>().join(" "); - context - .set_config( - crate::config::Config::AuthservIdCandidates, - Some(&new_config), - ) - .await?; - + if old_ids != new_ids { + let new_config = new_ids.into_iter().collect::>().join(" "); + context + .set_config(Config::AuthservIdCandidates, Some(&new_config)) + .await?; + } Ok(()) } @@ -138,16 +149,19 @@ pub(crate) async fn should_allow_keychange( ) -> Result { // TODO code duplication with update_authservid_candidates() let mut dkim_passed = true; // TODO what do we want to do if there are multiple or no authservid candidates? - if let Some(ids_config) = context - .get_config(crate::config::Config::AuthservIdCandidates) - .await? - { - let ids = ids_config.split(' ').filter(|s| !s.is_empty()); - dbg!(&ids_config); - if let Some(authserv_id) = tools::single_value(ids) { - // dbg!(&authentication_results, &ids_config); - // TODO unwrap - dkim_passed = authentication_results.get(authserv_id).unwrap().dkim_passed; + + // If the authentication results are empty, then our provider doesn't add them + // and an attacker could just add their own Authentication-Results, making us + // think that DKIM passed. So, in this case, we can as well assume that DKIM passed. + if !authentication_results.is_empty() { + if let Some(ids_config) = context.get_config(Config::AuthservIdCandidates).await? { + let ids = ids_config.split(' ').filter(|s| !s.is_empty()); + println!("{}", &ids_config); + if let Some(authserv_id) = tools::single_value(ids) { + dbg!(&authentication_results, &ids_config); + // TODO unwrap + dkim_passed = authentication_results.get(authserv_id).unwrap().dkim_passed; + } } } @@ -176,19 +190,19 @@ pub(crate) async fn should_allow_keychange( #[cfg(test)] mod tests { + #![allow(clippy::indexing_slicing)] use tokio::fs; use tokio::io::AsyncReadExt; use super::*; - use crate::headerdef::HeaderDefMap; - use crate::test_utils::*; + use crate::mimeparser; + use crate::test_utils::TestContext; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_parse_authentication_results() -> Result<()> { let t = TestContext::new().await; t.configure_addr("alice@gmx.net").await; - let bytes = b"From: info@slack.com -Authentication-Results: gmx.net; dkim=pass header.i=@slack.com + let bytes = b"Authentication-Results: gmx.net; dkim=pass header.i=@slack.com Authentication-Results: gmx.net; dkim=pass header.i=@amazonses.com"; let mail = mailparse::parse_mail(bytes)?; let actual = parse_authentication_results(&mail.get_headers(), "info@slack.com").unwrap(); @@ -201,6 +215,52 @@ Authentication-Results: gmx.net; dkim=pass header.i=@amazonses.com"; .into() ); + let bytes = b"Authentication-Results: gmx.net; dkim=pass header.i=@amazonses.com"; + let mail = mailparse::parse_mail(bytes)?; + let actual = parse_authentication_results(&mail.get_headers(), "info@slack.com").unwrap(); + assert_eq!( + actual, + [( + "gmx.net".to_string(), + AuthenticationResults { dkim_passed: false } + )] + .into() + ); + + // Weird Authentication-Results from Outlook without an authserv-id + let bytes = b"Authentication-Results: spf=pass (sender IP is 40.92.73.85) + smtp.mailfrom=hotmail.com; dkim=pass (signature was verified) + header.d=hotmail.com;dmarc=pass action=none + header.from=hotmail.com;compauth=pass reason=100"; + let mail = mailparse::parse_mail(bytes)?; + let actual = + parse_authentication_results(&mail.get_headers(), "alice@hotmail.com").unwrap(); + // At this point, the most important thing to test is that there are no + // authserv-ids with whitespace in them. + assert_eq!( + actual, + [( + "invalidAuthservId".to_string(), + AuthenticationResults { dkim_passed: true } + )] + .into() + ); + + // Usually, MUAs put their Authentication-Results to the top, so if in doubt, + // headers from the top should be preferred + let bytes = b"Authentication-Results: gmx.net; dkim=none header.i=@slack.com +Authentication-Results: gmx.net; dkim=pass header.i=@slack.com"; + let mail = mailparse::parse_mail(bytes)?; + let actual = parse_authentication_results(&mail.get_headers(), "info@slack.com").unwrap(); + assert_eq!( + actual, + [( + "gmx.net".to_string(), + AuthenticationResults { dkim_passed: false } + )] + .into() + ); + // TODO test that foreign Auth-Res headers are ignored // check_parse_authentication_results_combination( @@ -257,6 +317,37 @@ Authentication-Results: gmx.net; dkim=pass header.i=@amazonses.com"; Ok(()) } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_update_authservid_candidates() -> Result<()> { + let t = TestContext::new_alice().await; + + update_authservid_candidates_test(&t, &["mx3.messagingengine.com"]).await; + let candidates = t.get_config(Config::AuthservIdCandidates).await?.unwrap(); + assert_eq!(candidates, "mx3.messagingengine.com"); + + update_authservid_candidates_test(&t, &["mx4.messagingengine.com"]).await; + let candidates = t.get_config(Config::AuthservIdCandidates).await?.unwrap(); + assert_eq!(candidates, ""); + + // "mx4.messagingengine.com" seems to be the new authserv-id, DC should accept it + update_authservid_candidates_test(&t, &["mx4.messagingengine.com"]).await; + let candidates = t.get_config(Config::AuthservIdCandidates).await?.unwrap(); + assert_eq!(candidates, "mx4.messagingengine.com"); + + Ok(()) + } + + /// TODO document + async fn update_authservid_candidates_test(context: &Context, incoming_ids: &[&str]) { + let map = incoming_ids + .iter() + // update_authservid_candidates() only looks at the keys of the HashMap argument, + // so just provide some arbitrary values + .map(|id| (id.to_string(), AuthenticationResults { dkim_passed: true })) + .collect(); + update_authservid_candidates(context, &map).await.unwrap() + } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_realworld_authentication_results() -> Result<()> { let mut dir = fs::read_dir("test-data/message/dkimchecks-2022-09-28/") @@ -272,29 +363,22 @@ Authentication-Results: gmx.net; dkim=pass header.i=@amazonses.com"; while let Some(entry) = dir.next_entry().await.unwrap() { let mut file = fs::File::open(entry.path()).await?; - println!("{:?}", entry.path()); bytes.clear(); file.read_to_end(&mut bytes).await.unwrap(); if bytes.is_empty() { continue; } + println!("{:?}", entry.path()); let mail = mailparse::parse_mail(&bytes)?; - // TODO code duplication with create_decryption_info() - let from = mail - .headers - .get_header(HeaderDef::From_) - .and_then(|from_addr| mailparse::addrparse_header(from_addr).ok()) - .and_then(|from| from.extract_single_info()) - .map(|from| from.addr) - .unwrap_or_default(); + let from = &mimeparser::get_from(&mail.headers)[0].addr; // TODO code duplication with create_decryption_info() let authentication_results = - parse_authentication_results(&mail.get_headers(), &from)?; + parse_authentication_results(&mail.get_headers(), from)?; update_authservid_candidates(&t, &authentication_results).await?; let allow_keychange = - should_allow_keychange(&t, &authentication_results, &from).await?; + should_allow_keychange(&t, &authentication_results, from).await?; assert!(allow_keychange); diff --git a/src/decrypt.rs b/src/decrypt.rs index b28b69d18..1ef89dd8d 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -5,6 +5,7 @@ use std::collections::HashSet; use anyhow::{Context as _, Result}; use mailparse::ParsedMail; +use mailparse::SingleInfo; use crate::aheader::Aheader; use crate::authentication_results_handling::parse_authentication_results; @@ -12,8 +13,7 @@ use crate::authentication_results_handling::should_allow_keychange; use crate::authentication_results_handling::update_authservid_candidates; use crate::contact::addr_cmp; use crate::context::Context; -use crate::headerdef::HeaderDef; -use crate::headerdef::HeaderDefMap; + use crate::key::{DcKey, Fingerprint, SignedPublicKey, SignedSecretKey}; use crate::keyring::Keyring; use crate::log::LogExt; @@ -59,30 +59,34 @@ pub async fn try_decrypt( .await } -pub async fn create_decryption_info( +pub async fn prepare_decryption( context: &Context, mail: &ParsedMail<'_>, + from: &[SingleInfo], message_time: i64, ) -> Result { - let from = mail - .headers - .get_header(HeaderDef::From_) - .and_then(|from_addr| mailparse::addrparse_header(from_addr).ok()) - .and_then(|from| from.extract_single_info()) - .map(|from| from.addr) - .unwrap_or_default(); + let from = if let Some(f) = from.first() { + &f.addr + } else { + return Ok(DecryptionInfo { + from: "".to_string(), + autocrypt_header: None, + peerstate: None, + message_time, + }); + }; - let autocrypt_header = Aheader::from_headers(&from, &mail.headers) + let autocrypt_header = Aheader::from_headers(from, &mail.headers) .ok_or_log_msg(context, "Failed to parse Autocrypt header") .flatten(); - let authentication_results = parse_authentication_results(&mail.get_headers(), &from)?; + let authentication_results = parse_authentication_results(&mail.get_headers(), from)?; update_authservid_candidates(context, &authentication_results).await?; - let allow_keychange = should_allow_keychange(context, &authentication_results, &from).await?; + let allow_keychange = should_allow_keychange(context, &authentication_results, from).await?; let peerstate = get_autocrypt_peerstate( context, - &from, + from, autocrypt_header.as_ref(), message_time, allow_keychange, @@ -90,7 +94,7 @@ pub async fn create_decryption_info( .await?; Ok(DecryptionInfo { - from, + from: from.to_string(), autocrypt_header, peerstate, message_time, diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 3d31e915d..3baaf096b 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -16,7 +16,7 @@ use crate::blob::BlobObject; use crate::constants::{DC_DESIRED_TEXT_LINES, DC_DESIRED_TEXT_LINE_LEN}; use crate::contact::{addr_cmp, addr_normalize, ContactId}; use crate::context::Context; -use crate::decrypt::{create_decryption_info, try_decrypt}; +use crate::decrypt::{prepare_decryption, try_decrypt}; use crate::dehtml::dehtml; use crate::events::EventType; use crate::format_flowed::unformat_flowed; @@ -222,7 +222,7 @@ impl MimeMessage { let mut mail_raw = Vec::new(); let mut gossiped_addr = Default::default(); let mut from_is_signed = false; - let mut decryption_info = create_decryption_info(context, &mail, message_time).await?; + let mut decryption_info = prepare_decryption(context, &mail, &from, message_time).await?; // `signatures` is non-empty exactly if the message was encrypted and correctly signed. let (mail, signatures, warn_empty_signature) = @@ -1822,7 +1822,7 @@ mod tests { test_utils::TestContext, }; use mailparse::ParsedMail; - use tokio::{fs, io::AsyncReadExt}; + impl AvatarAction { pub fn is_change(&self) -> bool {