diff --git a/src/authres.rs b/src/authres.rs index 5690048e5..1b21ce247 100644 --- a/src/authres.rs +++ b/src/authres.rs @@ -317,12 +317,21 @@ 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::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; #[test] @@ -583,4 +592,103 @@ Authentication-Results: box.hispanilandia.net; spf=pass smtp.mailfrom=adbenitez@ let mail = mailparse::parse_mail(bytes).unwrap(); handle_authres(&t, &mail, "invalidfrom.com").await.unwrap(); } + + #[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 in the past with + // correct DKIM -> `dkim_works()` was called + dkim_works(&alice, "example.net").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!"); + + 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"); + + 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(); + + // 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.as_ref().unwrap().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"); + 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(()) + } } diff --git a/src/chat.rs b/src/chat.rs index 00c187974..b281d757d 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -3702,11 +3702,11 @@ mod tests { // create group and sync it to the second device let a1_chat_id = create_group_chat(&a1, ProtectionStatus::Unprotected, "foo").await?; - a1.send_text(a1_chat_id, "ho!").await; + let sent = a1.send_text(a1_chat_id, "ho!").await; let a1_msg = a1.get_last_msg().await; let a1_chat = Chat::load_from_db(&a1, a1_chat_id).await?; - let a2_msg = a2.recv_msg(&a1.pop_sent_msg().await).await; + let a2_msg = a2.recv_msg(&sent).await; let a2_chat_id = a2_msg.chat_id; let a2_chat = Chat::load_from_db(&a2, a2_chat_id).await?; diff --git a/src/mimeparser.rs b/src/mimeparser.rs index e584a8d1f..0a32f77da 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -370,16 +370,16 @@ impl MimeMessage { parser.heuristically_parse_ndn(context).await; parser.parse_headers(context).await?; + 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.\n\nScan the sender's QR code to make this work again.".to_string()); + } + } if warn_empty_signature && parser.signatures.is_empty() { for part in parser.parts.iter_mut() { 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; diff --git a/src/test_utils.rs b/src/test_utils.rs index 9f9e456a0..b3699be95 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -74,6 +74,13 @@ impl TestContextManager { .await } + pub async fn unconfigured(&mut self) -> TestContext { + TestContext::builder() + .with_log_sink(self.log_tx.clone()) + .build() + .await + } + /// Writes info events to the log that mark a section, e.g.: /// /// ========== `msg` goes here ========== @@ -89,19 +96,24 @@ impl TestContextManager { /// - Let the other TestContext receive it and accept the chat /// - Assert that the message arrived pub async fn send_recv_accept(&self, from: &TestContext, to: &TestContext, msg: &str) { + let received_msg = self.try_send_recv(from, to, msg).await; + assert_eq!(received_msg.text.as_ref().unwrap(), msg); + received_msg.chat_id.accept(to).await.unwrap(); + } + + /// - Let one TestContext send a message + /// - Let the other TestContext receive it + pub async fn try_send_recv(&self, from: &TestContext, to: &TestContext, msg: &str) -> Message { self.section(&format!( "{} sends a message '{}' to {}", from.name(), msg, to.name() )); - let chat = from.create_chat(to).await; let sent = from.send_text(chat.id, msg).await; - let received_msg = to.recv_msg(&sent).await; - received_msg.chat_id.accept(to).await.unwrap(); - assert_eq!(received_msg.text.unwrap(), msg); + received_msg } pub async fn change_addr(&self, test_context: &TestContext, new_addr: &str) { @@ -369,6 +381,12 @@ impl TestContext { /// /// Panics if there is no message or on any error. pub async fn pop_sent_msg(&self) -> SentMessage { + self.pop_sent_msg_opt(Duration::from_secs(3)) + .await + .expect("no sent message found in jobs table") + } + + pub async fn pop_sent_msg_opt(&self, timeout: Duration) -> Option { let start = Instant::now(); let (rowid, msg_id, payload, recipients) = loop { let row = self @@ -393,25 +411,25 @@ impl TestContext { if let Some(row) = row { break row; } - if start.elapsed() < Duration::from_secs(3) { + if start.elapsed() < timeout { tokio::time::sleep(Duration::from_millis(100)).await; } else { - panic!("no sent message found in jobs table"); + return None; } }; self.ctx .sql - .execute("DELETE FROM jobs WHERE id=?;", paramsv![rowid]) + .execute("DELETE FROM smtp WHERE id=?;", paramsv![rowid]) .await .expect("failed to remove job"); update_msg_state(&self.ctx, msg_id, MessageState::OutDelivered) .await .expect("failed to update message state"); - SentMessage { + Some(SentMessage { payload, sender_msg_id: msg_id, recipients, - } + }) } /// Parses a message. @@ -725,7 +743,7 @@ impl Drop for LogSink { /// passed through a SMTP-IMAP pipeline. #[derive(Debug, Clone)] pub struct SentMessage { - payload: String, + pub payload: String, recipients: String, pub sender_msg_id: MsgId, }