Mirrors/chatmail-core
1
0
Fork 0
mirror of https://github.com/chatmail/core.git synced 2026-05-03 21:36:29 +03:00
Code Issues Packages Projects Releases Wiki Activity

Fix some stuff

Browse Source
This commit is contained in:
Hocuri
2022-10-10 12:07:51 +02:00
parent 47500bb3ea
commit 65040fe6f2
3 changed files with 154 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

180
src/authentication_results_handling.rs
View File

@@ -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<AuthservId, Vec<String>> = 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<bo
// We have found a `dkim=pass` header!
return Ok(true);
}
} else {
// dkim=fail, dkim=none or whatever
return Ok(false);
}
}
}
@@ -91,6 +103,13 @@ fn authresults_dkim_passed(headers: &[String], sender_domain: &str) -> Result<bo
Ok(false)
}
fn parse_authservid_candidates_config(config: &Option<String>) -> 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::<Vec<_>>().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::<Vec<_>>().join(" ");
context
.set_config(Config::AuthservIdCandidates, Some(&new_config))
.await?;
}
Ok(())
}
@@ -138,16 +149,19 @@ pub(crate) async fn should_allow_keychange(
) -> Result<bool> {
// 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);

34
src/decrypt.rs
View File

@@ -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<DecryptionInfo> {
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,

6
src/mimeparser.rs
View File

@@ -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 {