diff --git a/src/authres_handling.rs b/src/authres_handling.rs index 8492169ca..dde69569c 100644 --- a/src/authres_handling.rs +++ b/src/authres_handling.rs @@ -3,6 +3,7 @@ use std::borrow::Cow; use std::collections::HashSet; +use std::fmt; use anyhow::Result; use mailparse::MailHeaderMap; @@ -27,14 +28,14 @@ pub(crate) async fn handle_authres( context: &Context, mail: &ParsedMail<'_>, from: &str, -) -> Result { +) -> Result { 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(false); + return Ok(DkimResults::default()); } }; @@ -44,6 +45,27 @@ pub(crate) async fn handle_authres( Ok(allow_keychange) } +#[derive(Default, Debug)] +pub(crate) struct DkimResults { + pub dkim_passed: bool, + pub dkim_works: bool, + pub allow_keychange: bool, +} + +impl fmt::Display for DkimResults { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + write!( + fmt, + "DKIM Results: Passed={}, Works={}, Allow_Keychange={}", + self.dkim_passed, self.dkim_works, self.allow_keychange + )?; + if !self.allow_keychange { + write!(fmt, " KEYCHANGES NOT ALLOWED!!!!")?; + } + Ok(()) + } +} + type AuthservId = String; #[derive(Debug, PartialEq)] @@ -59,12 +81,12 @@ enum DkimResult { Nothing, } -type AuthenticationResults = Vec<(AuthservId, DkimResult)>; +type ParsedAuthresHeaders = Vec<(AuthservId, DkimResult)>; fn parse_authres_headers( headers: &mailparse::headers::Headers<'_>, from_domain: &str, -) -> AuthenticationResults { +) -> ParsedAuthresHeaders { let mut res = Vec::new(); for header_value in headers.get_all_values(HeaderDef::AuthenticationResults.into()) { let header_value = remove_comments(&header_value); @@ -163,7 +185,7 @@ fn parse_one_authres_header(header_value: &str, from_domain: &str) -> DkimResult /// See [`handle_authres`]. async fn update_authservid_candidates( context: &Context, - authres: &AuthenticationResults, + authres: &ParsedAuthresHeaders, ) -> Result<()> { let mut new_ids: HashSet<&str> = authres .iter() @@ -205,9 +227,9 @@ async fn update_authservid_candidates( /// we don't accept Autocrypt key changes if they come with negative Authentication-Results. async fn should_allow_keychange( context: &Context, - mut authres: AuthenticationResults, + mut authres: ParsedAuthresHeaders, from_domain: &str, -) -> Result { +) -> Result { let mut dkim_passed = false; let ids_config = context.get_config(Config::AuthservidCandidates).await?; @@ -239,12 +261,17 @@ async fn should_allow_keychange( } } - let dkim_works = dkim_works(context, from_domain).await?; + let mut dkim_works = dkim_works(context, from_domain).await?; if !dkim_works && dkim_passed { set_dkim_works(context, from_domain).await?; + dkim_works = true; } - Ok(dkim_passed || !dkim_works) + Ok(DkimResults { + dkim_passed, + dkim_works, + allow_keychange: dkim_passed || !dkim_works, + }) } async fn dkim_works(context: &Context, from_domain: &str) -> Result { @@ -492,8 +519,8 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ let mail = mailparse::parse_mail(&bytes)?; let from = &mimeparser::get_from(&mail.headers)[0].addr; - let allow_keychange = handle_authres(&t, &mail, from).await?; - assert!(allow_keychange); + let res = handle_authres(&t, &mail, from).await?; + assert!(res.allow_keychange); } for entry in &dir { @@ -504,8 +531,8 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ let mail = mailparse::parse_mail(&bytes)?; let from = &mimeparser::get_from(&mail.headers)[0].addr; - let allow_keychange = handle_authres(&t, &mail, from).await?; - if !allow_keychange { + let res = handle_authres(&t, &mail, from).await?; + if !res.allow_keychange { println!( "!!!!!! FAILURE Receiving {:?}, keychange is not allowed !!!!!!", entry.path() @@ -514,7 +541,8 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ } let from_domain = EmailAddress::new(from).unwrap().domain; - let dkim_result = dkim_works(&t, &from_domain).await.unwrap(); + assert_eq!(res.dkim_works, dkim_works(&t, &from_domain).await.unwrap()); + assert_eq!(res.dkim_passed, res.dkim_works); // delta.blinzeln.de and gmx.de have invalid DKIM, so the DKIM check should fail let expected_result = (from_domain != "delta.blinzeln.de") && (from_domain != "gmx.de") @@ -524,7 +552,7 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ // Other forged emails && !from.starts_with("forged"); - if dkim_result != expected_result { + if res.dkim_passed != expected_result { if authres_parsing_works { println!( "!!!!!! FAILURE Receiving {:?}, order {:#?} wrong result: !!!!!!", @@ -533,7 +561,7 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ ); test_failed = true; } - println!("From {}: {}", from_domain, dkim_result); + println!("From {}: {}", from_domain, res.dkim_passed); } } } diff --git a/src/decrypt.rs b/src/decrypt.rs index 4984846bf..53a2dd88d 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -7,6 +7,7 @@ use mailparse::ParsedMail; use mailparse::SingleInfo; use crate::aheader::Aheader; +use crate::authres_handling; use crate::authres_handling::handle_authres; use crate::contact::addr_cmp; use crate::context::Context; @@ -64,26 +65,21 @@ pub async fn prepare_decryption( let from = if let Some(f) = from.first() { &f.addr } else { - return Ok(DecryptionInfo { - from: "".to_string(), - autocrypt_header: None, - peerstate: None, - message_time, - }); + return Ok(DecryptionInfo::default()); }; let autocrypt_header = Aheader::from_headers(from, &mail.headers) .ok_or_log_msg(context, "Failed to parse Autocrypt header") .flatten(); - let allow_keychange = handle_authres(context, mail, from).await?; + let dkim_results = handle_authres(context, mail, from).await?; let peerstate = get_autocrypt_peerstate( context, from, autocrypt_header.as_ref(), message_time, - allow_keychange, + dkim_results.allow_keychange, ) .await?; @@ -92,10 +88,11 @@ pub async fn prepare_decryption( autocrypt_header, peerstate, message_time, + dkim_results, }) } -#[derive(Debug)] +#[derive(Default, Debug)] pub struct DecryptionInfo { /// The From address. This is the address from the unnencrypted, outer /// From header. @@ -108,6 +105,7 @@ pub struct DecryptionInfo { /// means out-of-order message arrival, We don't modify the /// peerstate in this case. pub message_time: i64, + pub(crate) dkim_results: authres_handling::DkimResults, } /// Returns a reference to the encrypted payload of a ["Mixed diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 90571058d..e584a8d1f 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -178,7 +178,7 @@ impl MimeMessage { .get_header_value(HeaderDef::Date) .and_then(|v| mailparse::dateparse(&v).ok()) .unwrap_or_default(); - let hop_info = parse_receive_headers(&mail.get_headers()); + let mut hop_info = parse_receive_headers(&mail.get_headers()); let mut headers = Default::default(); let mut recipients = Default::default(); @@ -221,6 +221,7 @@ impl MimeMessage { 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 += &decryption_info.dkim_results.to_string(); // `signatures` is non-empty exactly if the message was encrypted and correctly signed. let (mail, signatures, warn_empty_signature) = @@ -374,6 +375,11 @@ impl MimeMessage { part.error = Some("No valid signature".to_string()); } } + if !decryption_info.dkim_results.allow_keychange { + for part in parser.parts.iter_mut() { + part.error = Some("No keychange allowed, this either is an attack or (more likely) a bug in authres-checking".to_string()); + } + } if parser.is_mime_modified { parser.decoded_data = mail_raw;