diff --git a/src/authres.rs b/src/authres.rs index c515c99a9..46dd10eb1 100644 --- a/src/authres.rs +++ b/src/authres.rs @@ -14,7 +14,6 @@ use once_cell::sync::Lazy; use crate::config::Config; use crate::context::Context; use crate::headerdef::HeaderDef; -use crate::tools::time; /// `authres` is short for the Authentication-Results header, defined in /// , which contains info @@ -29,45 +28,28 @@ pub(crate) async fn handle_authres( context: &Context, mail: &ParsedMail<'_>, from: &str, - message_time: i64, ) -> Result { let from_domain = match EmailAddress::new(from) { Ok(email) => email.domain, Err(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 Err(anyhow::format_err!("invalid email {}: {:#}", from, e)); } }; 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 + compute_dkim_results(context, authres).await } #[derive(Debug)] pub(crate) struct DkimResults { /// Whether DKIM passed for this particular e-mail. pub dkim_passed: bool, - /// Whether DKIM is known to work for e-mails coming from the sender's domain, - /// i.e. whether we expect DKIM to work. - pub dkim_should_work: bool, - /// Whether changing the public Autocrypt key should be allowed. - /// This is false if we expected DKIM to work (dkim_works=true), - /// but it failed now (dkim_passed=false). - 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_should_work, self.allow_keychange - )?; - if !self.allow_keychange { - write!(fmt, " KEYCHANGES NOT ALLOWED!!!!")?; - } + write!(fmt, "DKIM Results: Passed={}", self.dkim_passed)?; Ok(()) } } @@ -218,10 +200,6 @@ async fn update_authservid_candidates( context .set_config_internal(Config::AuthservIdCandidates, Some(&new_config)) .await?; - // Updating the authservid candidates may mean that we now consider - // emails as "failed" which "passed" previously, so we need to - // reset our expectation which DKIMs work. - clear_dkim_works(context).await? } Ok(()) } @@ -238,8 +216,6 @@ async fn update_authservid_candidates( async fn compute_dkim_results( context: &Context, mut authres: ParsedAuthresHeaders, - from_domain: &str, - message_time: i64, ) -> Result { let mut dkim_passed = false; @@ -272,71 +248,7 @@ async fn compute_dkim_results( } } - let last_working_timestamp = dkim_works_timestamp(context, from_domain).await?; - let mut dkim_should_work = dkim_should_work(last_working_timestamp)?; - if message_time > last_working_timestamp && dkim_passed { - set_dkim_works_timestamp(context, from_domain, message_time).await?; - dkim_should_work = true; - } - - Ok(DkimResults { - dkim_passed, - dkim_should_work, - allow_keychange: dkim_passed || !dkim_should_work, - }) -} - -/// Whether DKIM in emails from this domain should be considered to work. -fn dkim_should_work(last_working_timestamp: i64) -> Result { - // When we get an email with valid DKIM-Authentication-Results, - // then we assume that DKIM works for 30 days from this time on. - let should_work_until = last_working_timestamp + 3600 * 24 * 30; - - let dkim_ever_worked = last_working_timestamp > 0; - - // We're using time() here and not the time when the message - // claims to have been sent (passed around as `message_time`) - // because otherwise an attacker could just put a time way - // in the future into the `Date` header and then we would - // assume that DKIM doesn't have to be valid anymore. - let dkim_should_work_now = should_work_until > time(); - Ok(dkim_ever_worked && dkim_should_work_now) -} - -async fn dkim_works_timestamp(context: &Context, from_domain: &str) -> Result { - let last_working_timestamp: i64 = context - .sql - .query_get_value( - "SELECT dkim_works FROM sending_domains WHERE domain=?", - (from_domain,), - ) - .await? - .unwrap_or(0); - Ok(last_working_timestamp) -} - -async fn set_dkim_works_timestamp( - context: &Context, - from_domain: &str, - timestamp: i64, -) -> Result<()> { - context - .sql - .execute( - "INSERT INTO sending_domains (domain, dkim_works) VALUES (?,?) - ON CONFLICT(domain) DO UPDATE SET dkim_works=excluded.dkim_works", - (from_domain, timestamp), - ) - .await?; - Ok(()) -} - -async fn clear_dkim_works(context: &Context) -> Result<()> { - context - .sql - .execute("DELETE FROM sending_domains", ()) - .await?; - Ok(()) + Ok(DkimResults { dkim_passed }) } fn parse_authservid_candidates_config(config: &Option) -> BTreeSet<&str> { @@ -349,19 +261,12 @@ fn parse_authservid_candidates_config(config: &Option) -> BTreeSet<&str> #[cfg(test)] mod tests { #![allow(clippy::indexing_slicing)] - use std::time::Duration; - use tokio::fs; use tokio::io::AsyncReadExt; use super::*; - use crate::aheader::EncryptPreference; - use crate::e2ee; use crate::mimeparser; use crate::peerstate::Peerstate; - use crate::securejoin::get_securejoin_qr; - use crate::securejoin::join_securejoin; - use crate::test_utils; use crate::test_utils::TestContext; use crate::test_utils::TestContextManager; use crate::tools; @@ -574,33 +479,8 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ let mail = mailparse::parse_mail(&bytes)?; let from = &mimeparser::get_from(&mail.headers).unwrap().addr; - let res = handle_authres(&t, &mail, from, time()).await?; - assert!(res.allow_keychange); - } - - for entry in &dir { - let mut file = fs::File::open(entry.path()).await?; - bytes.clear(); - file.read_to_end(&mut bytes).await.unwrap(); - - let mail = mailparse::parse_mail(&bytes)?; - let from = &mimeparser::get_from(&mail.headers).unwrap().addr; - - let res = handle_authres(&t, &mail, from, time()).await?; - if !res.allow_keychange { - println!( - "!!!!!! FAILURE Receiving {:?}, keychange is not allowed !!!!!!", - entry.path() - ); - test_failed = true; - } - + let res = handle_authres(&t, &mail, from).await?; let from_domain = EmailAddress::new(from).unwrap().domain; - assert_eq!( - res.dkim_should_work, - dkim_should_work(dkim_works_timestamp(&t, &from_domain).await?)? - ); - assert_eq!(res.dkim_passed, res.dkim_should_work); // 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") @@ -613,9 +493,8 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ if res.dkim_passed != expected_result { if authres_parsing_works { println!( - "!!!!!! FAILURE Receiving {:?}, order {:#?} wrong result: !!!!!!", + "!!!!!! FAILURE Receiving {:?} wrong result: !!!!!!", entry.path(), - dir.iter().map(|e| e.file_name()).collect::>() ); test_failed = true; } @@ -638,116 +517,7 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ let bytes = b"From: invalid@from.com Authentication-Results: dkim="; let mail = mailparse::parse_mail(bytes).unwrap(); - handle_authres(&t, &mail, "invalid@rom.com", time()) - .await - .unwrap(); - } - - #[ignore = "Disallowing keychanges is disabled for now"] - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn test_handle_authres_fails() -> Result<()> { - let mut tcm = TestContextManager::new(); - let alice = tcm.alice().await; - let bob = tcm.bob().await; - - // Bob sends Alice a message, so she gets his key - tcm.send_recv_accept(&bob, &alice, "Hi").await; - - // We don't need bob anymore, let's make sure it's not accidentally used - drop(bob); - - // Assume Alice receives an email from bob@example.net with - // correct DKIM -> `set_dkim_works()` was called - set_dkim_works_timestamp(&alice, "example.net", time()).await?; - // And Alice knows her server's authserv-id - alice - .set_config(Config::AuthservIdCandidates, Some("example.org")) - .await?; - - tcm.section("An attacker, bob2, sends a from-forged email to Alice!"); - - // Sleep to make sure key reset is ignored because of DKIM failure - // and not because reordering is suspected. - tokio::time::sleep(std::time::Duration::from_millis(1100)).await; - - let bob2 = tcm.unconfigured().await; - bob2.configure_addr("bob@example.net").await; - e2ee::ensure_secret_key_exists(&bob2).await?; - - let chat = bob2.create_chat(&alice).await; - let mut sent = bob2 - .send_text(chat.id, "Please send me lots of money") - .await; - - sent.payload - .insert_str(0, "Authentication-Results: example.org; dkim=fail\n"); - - let received = alice.recv_msg(&sent).await; - - // Assert that the error tells the user about the problem - assert!(received.error.unwrap().contains("DKIM failed")); - - let bob_state = Peerstate::from_addr(&alice, "bob@example.net") - .await? - .unwrap(); - - // Encryption preference is still mutual. - assert_eq!(bob_state.prefer_encrypt, EncryptPreference::Mutual); - - // Also check that the keypair was not changed - assert_eq!( - bob_state.public_key.unwrap(), - test_utils::bob_keypair().public - ); - - // Since Alice didn't change the key, Bob can't read her message - let received = tcm - .try_send_recv(&alice, &bob2, "My credit card number is 1234") - .await; - assert!(!received.text.contains("1234")); - assert!(received.error.is_some()); - - tcm.section("Turns out bob2 wasn't an attacker at all, Bob just has a new phone and DKIM just stopped working."); - tcm.section("To fix the key problems, Bob scans Alice's QR code."); - - let qr = get_securejoin_qr(&alice.ctx, None).await.unwrap(); - join_securejoin(&bob2.ctx, &qr).await.unwrap(); - - loop { - if let Some(mut sent) = bob2.pop_sent_msg_opt(Duration::ZERO).await { - sent.payload - .insert_str(0, "Authentication-Results: example.org; dkim=fail\n"); - alice.recv_msg(&sent).await; - } else if let Some(sent) = alice.pop_sent_msg_opt(Duration::ZERO).await { - bob2.recv_msg(&sent).await; - } else { - break; - } - } - - // Unfortunately, securejoin currently doesn't work with authres-checking, - // so these checks would fail: - - // let contact_bob = alice.add_or_lookup_contact(&bob2).await; - // assert_eq!( - // contact_bob.is_verified(&alice.ctx).await.unwrap(), - // VerifiedStatus::BidirectVerified - // ); - - // let contact_alice = bob2.add_or_lookup_contact(&alice).await; - // assert_eq!( - // contact_alice.is_verified(&bob2.ctx).await.unwrap(), - // VerifiedStatus::BidirectVerified - // ); - - // // Bob can read Alice's messages again - // let received = tcm - // .try_send_recv(&alice, &bob2, "Can you read this again?") - // .await; - // assert_eq!(received.text.as_ref().unwrap(), "Can you read this again?"); - // assert!(received.error.is_none()); - - Ok(()) + handle_authres(&t, &mail, "invalid@rom.com").await.unwrap(); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -796,10 +566,7 @@ Authentication-Results: dkim="; let alice = tcm.alice().await; let bob = tcm.bob().await; - // Assume Bob received an email from something@example.net with - // correct DKIM -> `set_dkim_works()` was called - set_dkim_works_timestamp(&bob, "example.org", time()).await?; - // And Bob knows his server's authserv-id + // Bob knows his server's authserv-id bob.set_config(Config::AuthservIdCandidates, Some("example.net")) .await?; @@ -821,15 +588,13 @@ Authentication-Results: dkim="; .insert_str(0, "Authentication-Results: example.net; dkim=fail\n"); let rcvd = bob.recv_msg(&sent).await; - // Disallowing keychanges is disabled for now: - // assert!(rcvd.error.unwrap().contains("DKIM failed")); // The message info should contain a warning: assert!(rcvd .id .get_info(&bob) .await .unwrap() - .contains("KEYCHANGES NOT ALLOWED")); + .contains("DKIM Results: Passed=false")); Ok(()) } diff --git a/src/decrypt.rs b/src/decrypt.rs index 5faf79182..98bd0b16a 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -57,11 +57,7 @@ pub(crate) async fn prepare_decryption( autocrypt_header: None, peerstate: None, message_time, - dkim_results: DkimResults { - dkim_passed: false, - dkim_should_work: false, - allow_keychange: true, - }, + dkim_results: DkimResults { dkim_passed: false }, }); } @@ -86,15 +82,13 @@ pub(crate) async fn prepare_decryption( None }; - let dkim_results = handle_authres(context, mail, from, message_time).await?; + let dkim_results = handle_authres(context, mail, from).await?; let allow_aeap = get_encrypted_mime(mail).is_some(); let peerstate = get_autocrypt_peerstate( context, from, autocrypt_header.as_ref(), message_time, - // Disallowing keychanges is disabled for now: - true, // dkim_results.allow_keychange, allow_aeap, ) .await?; @@ -287,19 +281,15 @@ pub(crate) fn keyring_from_peerstate(peerstate: Option<&Peerstate>) -> Vec, message_time: i64, - allow_change: bool, allow_aeap: bool, ) -> Result> { - let allow_change = allow_change && !context.is_self_addr(from).await?; + let allow_change = !context.is_self_addr(from).await?; let mut peerstate; // Apply Autocrypt header diff --git a/src/mimeparser.rs b/src/mimeparser.rs index ee88130d5..36df10780 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -415,8 +415,6 @@ impl MimeMessage { if let (Some(peerstate), Ok(mail)) = (&mut decryption_info.peerstate, mail) { if timestamp_sent > peerstate.last_seen_autocrypt && mail.ctype.mimetype != "multipart/report" - // Disallowing keychanges is disabled for now: - // && decryption_info.dkim_results.allow_keychange { peerstate.degrade_encryption(timestamp_sent); } @@ -506,13 +504,6 @@ impl MimeMessage { parser.heuristically_parse_ndn(context).await; parser.parse_headers(context).await?; - // Disallowing keychanges is disabled for now - // if !decryption_info.dkim_results.allow_keychange { - // for part in parser.parts.iter_mut() { - // part.error = Some("Seems like DKIM failed, this either is an attack or (more likely) a bug in Authentication-Results checking. Please tell us about this at https://support.delta.chat.".to_string()); - // } - // } - if parser.is_mime_modified { parser.decoded_data = mail_raw; } diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 247d42c73..2bf3ab57c 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -613,6 +613,7 @@ CREATE INDEX smtp_messageid ON imap(rfc724_mid); ).await?; } if dbversion < 93 { + // `sending_domains` is now unused, but was not removed for backwards compatibility. sql.execute_migration( "CREATE TABLE sending_domains(domain TEXT PRIMARY KEY, dkim_works INTEGER DEFAULT 0);", 93, diff --git a/src/tools.rs b/src/tools.rs index fbc4f455f..abbfb25cd 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -735,7 +735,7 @@ Message-ID: 2dfdbde7@example.org Hop: From: localhost; By: hq5.merlinux.eu; Date: Sat, 14 Sep 2019 17:00:22 +0000 Hop: From: hq5.merlinux.eu; By: hq5.merlinux.eu; Date: Sat, 14 Sep 2019 17:00:25 +0000 -DKIM Results: Passed=true, Works=true, Allow_Keychange=true"; +DKIM Results: Passed=true"; check_parse_receive_headers_integration(raw, expected).await; let raw = include_bytes!("../test-data/message/encrypted_with_received_headers.eml"); @@ -754,7 +754,7 @@ Hop: From: [127.0.0.1]; By: mail.example.org; Date: Mon, 27 Dec 2021 11:21:21 +0 Hop: From: mout.example.org; By: hq5.example.org; Date: Mon, 27 Dec 2021 11:21:22 +0000 Hop: From: hq5.example.org; By: hq5.example.org; Date: Mon, 27 Dec 2021 11:21:22 +0000 -DKIM Results: Passed=true, Works=true, Allow_Keychange=true"; +DKIM Results: Passed=true"; check_parse_receive_headers_integration(raw, expected).await; }