From 0f7cc111f0386290c7fa071f475f742e300c1985 Mon Sep 17 00:00:00 2001 From: mdecimus Date: Sat, 18 May 2024 10:28:34 +0200 Subject: [PATCH] v0.4.0 --- CHANGELOG.md | 5 ++ Cargo.toml | 4 +- src/common/auth_results.rs | 1 + src/common/message.rs | 159 ++++++++++++++++++++----------------- src/dkim/sign.rs | 41 +++++++++- src/dkim/verify.rs | 1 + src/lib.rs | 2 + 7 files changed, 136 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a9e8c0..c87aa76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +mail-auth 0.4.0 +================================ +- DKIM verification defaults to `strict` mode and ignores signatures with a `l=` tag to avoid exploits (see https://stalw.art/blog/dkim-exploit). Use `AuthenticatedMessage::parse_with_opts(&message, false)` to enable `relaxed` mode. +- Parsed fields are now public. + mail-auth 0.3.11 ================================ - Added: DKIM keypair generation for both RSA and Ed25519. diff --git a/Cargo.toml b/Cargo.toml index 1899fe4..2ddd4af 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "mail-auth" description = "DKIM, ARC, SPF and DMARC library for Rust" -version = "0.3.11" +version = "0.4.0" edition = "2021" authors = [ "Stalwart Labs "] license = "Apache-2.0 OR MIT" @@ -38,7 +38,7 @@ serde_json = "1.0" sha1 = { version = "0.10", features = ["oid"], optional = true } sha2 = { version = "0.10.6", features = ["oid"], optional = true } hickory-resolver = { version = "0.24", features = ["dns-over-rustls", "dnssec-ring"] } -zip = "0.6.3" +zip = "1.3.0" rand = { version = "0.8.5", optional = true } [dev-dependencies] diff --git a/src/common/auth_results.rs b/src/common/auth_results.rs index cb79d54..d8c384a 100644 --- a/src/common/auth_results.rs +++ b/src/common/auth_results.rs @@ -344,6 +344,7 @@ impl AsAuthResult for Error { Error::ArcBrokenChain => "broken ARC chain", Error::NotAligned => "policy not aligned", Error::InvalidRecordType => "invalid dns record type", + Error::SignatureLength => "signature length ignored due to security risk", }); header.push(')'); } diff --git a/src/common/message.rs b/src/common/message.rs index e570a29..ec413db 100644 --- a/src/common/message.rs +++ b/src/common/message.rs @@ -16,6 +16,10 @@ use super::headers::{AuthenticatedHeader, Header, HeaderParser}; impl<'x> AuthenticatedMessage<'x> { pub fn parse(raw_message: &'x [u8]) -> Option { + Self::parse_with_opts(raw_message, true) + } + + pub fn parse_with_opts(raw_message: &'x [u8], strict: bool) -> Option { let mut message = AuthenticatedMessage { headers: Vec::new(), from: Vec::new(), @@ -35,90 +39,103 @@ impl<'x> AuthenticatedMessage<'x> { let mut has_arc_errors = false; for (header, value) in &mut headers { - let name = match header { - AuthenticatedHeader::Ds(name) => { - let signature = dkim::Signature::parse(value); - if let Ok(signature) = &signature { - let ha = HashAlgorithm::from(signature.a); - if !message - .body_hashes - .iter() - .any(|(c, h, l, _)| c == &signature.cb && h == &ha && l == &signature.l) - { - message - .body_hashes - .push((signature.cb, ha, signature.l, Vec::new())); - } - } - message - .dkim_headers - .push(Header::new(name, value, signature)); - name - } - AuthenticatedHeader::Aar(name) => { - let results = arc::Results::parse(value); - if !has_arc_errors { - has_arc_errors = results.is_err(); + let name = + match header { + AuthenticatedHeader::Ds(name) => { + let signature = match dkim::Signature::parse(value) { + Ok(signature) if signature.l == 0 || !strict => { + let ha = HashAlgorithm::from(signature.a); + if !message.body_hashes.iter().any(|(c, h, l, _)| { + c == &signature.cb && h == &ha && l == &signature.l + }) { + message.body_hashes.push(( + signature.cb, + ha, + signature.l, + Vec::new(), + )); + } + Ok(signature) + } + Ok(_) => Err(crate::Error::SignatureLength), + Err(err) => Err(err), + }; + + message + .dkim_headers + .push(Header::new(name, value, signature)); + name } - message.aar_headers.push(Header::new(name, value, results)); - name - } - AuthenticatedHeader::Ams(name) => { - let signature = arc::Signature::parse(value); - - if let Ok(signature) = &signature { - let ha = HashAlgorithm::from(signature.a); - if !message - .body_hashes - .iter() - .any(|(c, h, l, _)| c == &signature.cb && h == &ha && l == &signature.l) - { - message - .body_hashes - .push((signature.cb, ha, signature.l, Vec::new())); + AuthenticatedHeader::Aar(name) => { + let results = arc::Results::parse(value); + if !has_arc_errors { + has_arc_errors = results.is_err(); } - } else { - has_arc_errors = true; + message.aar_headers.push(Header::new(name, value, results)); + name } - - message - .ams_headers - .push(Header::new(name, value, signature)); - name - } - AuthenticatedHeader::As(name) => { - let seal = arc::Seal::parse(value); - if !has_arc_errors { - has_arc_errors = seal.is_err(); + AuthenticatedHeader::Ams(name) => { + let signature = match arc::Signature::parse(value) { + Ok(signature) if signature.l == 0 || !strict => { + let ha = HashAlgorithm::from(signature.a); + if !message.body_hashes.iter().any(|(c, h, l, _)| { + c == &signature.cb && h == &ha && l == &signature.l + }) { + message.body_hashes.push(( + signature.cb, + ha, + signature.l, + Vec::new(), + )); + } + Ok(signature) + } + Ok(_) => { + has_arc_errors = true; + Err(crate::Error::SignatureLength) + } + Err(err) => { + has_arc_errors = true; + Err(err) + } + }; + + message + .ams_headers + .push(Header::new(name, value, signature)); + name } - message.as_headers.push(Header::new(name, value, seal)); - name - } - AuthenticatedHeader::From(name) => { - match MessageStream::new(value).parse_address() { - HeaderValue::Address(Address::List(list)) => { - message.from.extend( - list.into_iter() - .filter_map(|a| a.address.map(|a| a.to_lowercase())), - ); + AuthenticatedHeader::As(name) => { + let seal = arc::Seal::parse(value); + if !has_arc_errors { + has_arc_errors = seal.is_err(); } - HeaderValue::Address(Address::Group(group_list)) => { - message + message.as_headers.push(Header::new(name, value, seal)); + name + } + AuthenticatedHeader::From(name) => { + match MessageStream::new(value).parse_address() { + HeaderValue::Address(Address::List(list)) => { + message.from.extend( + list.into_iter() + .filter_map(|a| a.address.map(|a| a.to_lowercase())), + ); + } + HeaderValue::Address(Address::Group(group_list)) => message .from .extend(group_list.into_iter().flat_map(|group| { group .addresses .into_iter() .filter_map(|a| a.address.map(|a| a.to_lowercase())) - })) + })), + _ => (), } - _ => (), - } - name - } - AuthenticatedHeader::Other(name) => name, - }; + name + } + AuthenticatedHeader::Other(name) => name, + }; message.headers.push((name, value)); } diff --git a/src/dkim/sign.rs b/src/dkim/sign.rs index 7e1c9ec..e3ef82c 100644 --- a/src/dkim/sign.rs +++ b/src/dkim/sign.rs @@ -106,6 +106,7 @@ impl<'a> Writable for SignableMessage<'a> { #[cfg(test)] #[allow(unused)] pub mod test { + use core::str; use std::time::{Duration, Instant}; use hickory_resolver::proto::op::ResponseCode; @@ -351,12 +352,12 @@ pub mod test { ) .await; - dbg!("Test RSA-SHA256 simple/relaxed with fixed body length"); + dbg!("Test RSA-SHA256 simple/relaxed with fixed body length (relaxed)"); #[cfg(feature = "rust-crypto")] let pk_rsa = RsaKey::::from_pkcs1_pem(RSA_PRIVATE_KEY).unwrap(); #[cfg(all(feature = "ring", not(feature = "rust-crypto")))] let pk_rsa = RsaKey::::from_rsa_pem(RSA_PRIVATE_KEY).unwrap(); - verify( + verify_with_opts( &resolver, DkimSigner::from_key(pk_rsa) .domain("example.com") @@ -368,6 +369,28 @@ pub mod test { .unwrap(), &(message.to_string() + "\r\n----- Mailing list"), Ok(()), + false, + ) + .await; + + dbg!("Test RSA-SHA256 simple/relaxed with fixed body length (strict)"); + #[cfg(feature = "rust-crypto")] + let pk_rsa = RsaKey::::from_pkcs1_pem(RSA_PRIVATE_KEY).unwrap(); + #[cfg(all(feature = "ring", not(feature = "rust-crypto")))] + let pk_rsa = RsaKey::::from_rsa_pem(RSA_PRIVATE_KEY).unwrap(); + verify_with_opts( + &resolver, + DkimSigner::from_key(pk_rsa) + .domain("example.com") + .selector("default") + .headers(["From", "To", "Subject"]) + .header_canonicalization(Canonicalization::Simple) + .body_length(true) + .sign(message.as_bytes()) + .unwrap(), + &(message.to_string() + "\r\n----- Mailing list"), + Err(super::Error::SignatureLength), + true, ) .await; @@ -486,17 +509,18 @@ pub mod test { .await; } - pub async fn verify<'x>( + pub async fn verify_with_opts<'x>( resolver: &Resolver, signature: Signature, message_: &'x str, expect: Result<(), super::Error>, + strict: bool, ) -> Vec> { let mut message = Vec::with_capacity(message_.len() + 100); signature.write(&mut message, true); message.extend_from_slice(message_.as_bytes()); - let message = AuthenticatedMessage::parse(&message).unwrap(); + let message = AuthenticatedMessage::parse_with_opts(&message, strict).unwrap(); let dkim = resolver.verify_dkim(&message).await; match (dkim.last().unwrap().result(), &expect) { @@ -517,4 +541,13 @@ pub mod test { }) .collect() } + + pub async fn verify<'x>( + resolver: &Resolver, + signature: Signature, + message_: &'x str, + expect: Result<(), super::Error>, + ) -> Vec> { + verify_with_opts(resolver, signature, message_, expect, true).await + } } diff --git a/src/dkim/verify.rs b/src/dkim/verify.rs index 31690f9..1fcf246 100644 --- a/src/dkim/verify.rs +++ b/src/dkim/verify.rs @@ -221,6 +221,7 @@ impl Resolver { | Error::ArcInvalidCV | Error::ArcHasHeaderTag | Error::ArcBrokenChain + | Error::SignatureLength | Error::NotAligned => (record.rr & RR_OTHER) != 0, }; diff --git a/src/lib.rs b/src/lib.rs index f49feef..7bee922 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -461,6 +461,7 @@ pub enum Error { RevokedPublicKey, IncompatibleAlgorithms, SignatureExpired, + SignatureLength, DnsError(String), DnsRecordNotFound(ResponseCode), ArcChainTooLong, @@ -501,6 +502,7 @@ impl Display for Error { ), Error::FailedVerification => write!(f, "Signature verification failed"), Error::SignatureExpired => write!(f, "Signature expired"), + Error::SignatureLength => write!(f, "Insecure 'l=' tag found in Signature"), Error::FailedAuidMatch => write!(f, "AUID does not match domain name"), Error::ArcInvalidInstance(i) => { write!(f, "Invalid 'i={i}' value found in ARC header")