From fa1dec61a26dc8436a9fb04d64ff3890d72f2d86 Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Wed, 10 Apr 2024 13:11:12 -0700 Subject: [PATCH] dkim: remove usage of `rsa` crate We keep getting asked about https://rustsec.org/advisories/RUSTSEC-2023-0071.html and how it impacts kumomta. The answer to that question is: in the default build configuration, we use openssl's RSA signing implementation rather than that of the rsa crate. The reason for this is that OpenSSL's RSA implementation is due to the performance gap between the two implementations (https://github.com/RustCrypto/RSA/issues/339). The result of this is that the problematic code and attack vector described in the security advisory does not apply to KumoMTA, because it is not used to compute any signatures. In the interest of not raising any false alarms as more and more people perform security analyses on kumomta, this commit removes the `rsa` crate from the build graph. In order to do so, we need to port verification over to the openssl RSA implementation which is what this commit does. I look forward to a future version of the `rsa` crate being published that has this issue resolved, and that closes the performance gap! refs: https://github.com/RustCrypto/RSA/issues/390 --- Cargo.lock | 54 ----------------- crates/dkim/Cargo.toml | 11 ++-- crates/dkim/benches/sign.rs | 28 --------- crates/dkim/examples/sign_bench.rs | 27 --------- crates/dkim/src/lib.rs | 95 +++++++++++++----------------- crates/dkim/src/public_key.rs | 18 ++++-- crates/dkim/src/sign.rs | 19 ------ 7 files changed, 56 insertions(+), 196 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5ae6f506..efd4394a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2620,7 +2620,6 @@ dependencies = [ "openssl-sys", "quick-error 2.0.1", "regex", - "rsa", "sha-1", "sha2", "textwrap", @@ -2841,9 +2840,6 @@ name = "lazy_static" version = "1.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" -dependencies = [ - "spin 0.5.2", -] [[package]] name = "lazycell" @@ -3650,23 +3646,6 @@ dependencies = [ "num-traits", ] -[[package]] -name = "num-bigint-dig" -version = "0.8.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dc84195820f291c7697304f3cbdadd1cb7199c0efc917ff5eafd71225c136151" -dependencies = [ - "byteorder", - "lazy_static", - "libm", - "num-integer", - "num-iter", - "num-traits", - "rand", - "smallvec 1.13.2", - "zeroize", -] - [[package]] name = "num-complex" version = "0.2.4" @@ -3743,7 +3722,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da0df0e5185db44f69b44f26786fe401b6c293d1907744beaa7fa62b2e5a517a" dependencies = [ "autocfg", - "libm", ] [[package]] @@ -4096,17 +4074,6 @@ dependencies = [ "futures-io", ] -[[package]] -name = "pkcs1" -version = "0.7.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c8ffb9f10fa047879315e6625af03c164b16962a5368d724ed16323b68ace47f" -dependencies = [ - "der", - "pkcs8", - "spki", -] - [[package]] name = "pkcs8" version = "0.10.2" @@ -4782,26 +4749,6 @@ dependencies = [ "librocksdb-sys", ] -[[package]] -name = "rsa" -version = "0.9.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5d0e5124fcb30e76a7e79bfee683a2746db83784b86289f6251b54b7950a0dfc" -dependencies = [ - "const-oid", - "digest", - "num-bigint-dig", - "num-integer", - "num-traits", - "pkcs1", - "pkcs8", - "rand_core", - "signature", - "spki", - "subtle", - "zeroize", -] - [[package]] name = "rustc-demangle" version = "0.1.23" @@ -5280,7 +5227,6 @@ version = "2.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "77549399552de45a898a580c1b41d445bf730df867cc44e6c0233bbc4b8329de" dependencies = [ - "digest", "rand_core", ] diff --git a/crates/dkim/Cargo.toml b/crates/dkim/Cargo.toml index 0b01acc3..308dda52 100644 --- a/crates/dkim/Cargo.toml +++ b/crates/dkim/Cargo.toml @@ -11,24 +11,21 @@ readme = "README.md" license = "MIT" [features] -openssl = ["dep:openssl", "dep:openssl-sys", "dep:foreign-types"] -default = ["openssl"] [dependencies] chrono = { version = "0.4.26", default-features = false, features = ["clock", "std"] } data-encoding = "2.5" -ed25519-dalek = {workspace=true, features=["pkcs8"]} +ed25519-dalek = {workspace=true, features=["pkcs8", "pem"]} futures = {workspace=true} indexmap = "1.9.3" mailparsing = { path="../mailparsing" } memchr = "2.5" nom = "7.1.0" once_cell = "1.17" -foreign-types = {version="0.3", optional=true} -openssl = { workspace=true, optional=true} -openssl-sys = { workspace=true, optional=true} +foreign-types = "0.3" +openssl = { workspace=true } +openssl-sys = { workspace=true } quick-error = "2.0.1" -rsa = "0.9" sha-1 = { version = "0.10", features = ["oid"] } sha2 = { version = "0.10", features = ["oid"] } textwrap = "0.16" diff --git a/crates/dkim/benches/sign.rs b/crates/dkim/benches/sign.rs index b55193b6..281550ef 100644 --- a/crates/dkim/benches/sign.rs +++ b/crates/dkim/benches/sign.rs @@ -2,7 +2,6 @@ use chrono::TimeZone; use criterion::{black_box, criterion_group, criterion_main, Criterion, SamplingMode, Throughput}; use kumo_dkim::canonicalization::Type; use kumo_dkim::{DkimPrivateKey, ParsedEmail, SignerBuilder}; -use rsa::pkcs1::DecodeRsaPrivateKey; fn email_text() -> String { r#"Subject: subject @@ -55,33 +54,6 @@ pub fn criterion_benchmark(c: &mut Criterion) { let email_text = email_text(); let email = ParsedEmail::parse(email_text.clone()).unwrap(); - for canon in [Type::Simple, Type::Relaxed] { - let private_key = - rsa::RsaPrivateKey::read_pkcs1_pem_file("./test/keys/2022.private").unwrap(); - let time = chrono::Utc.with_ymd_and_hms(2021, 1, 1, 0, 0, 1).unwrap(); - - let signer = SignerBuilder::new() - .with_signed_headers(["From", "Subject"]) - .unwrap() - .with_body_canonicalization(canon) - .with_header_canonicalization(canon) - .with_private_key(DkimPrivateKey::Rsa(private_key)) - .with_selector("s20") - .with_signing_domain("example.com") - .with_time(time) - .build() - .unwrap(); - - let mut group = c.benchmark_group("kumo_dkim signing"); - group.sampling_mode(SamplingMode::Flat); - group.throughput(Throughput::Bytes(email_text.len() as u64)); - group.bench_function(&format!("sign {canon:?}"), |b| { - b.iter(|| signer.sign(black_box(&email)).unwrap()) - }); - group.finish(); - } - - #[cfg(feature = "openssl")] for canon in [Type::Simple, Type::Relaxed] { let data = std::fs::read("./test/keys/2022.private").unwrap(); let pkey = openssl::rsa::Rsa::private_key_from_pem(&data).unwrap(); diff --git a/crates/dkim/examples/sign_bench.rs b/crates/dkim/examples/sign_bench.rs index 0fe4863d..796da083 100644 --- a/crates/dkim/examples/sign_bench.rs +++ b/crates/dkim/examples/sign_bench.rs @@ -1,7 +1,6 @@ use chrono::TimeZone; use kumo_dkim::canonicalization::Type; use kumo_dkim::{DkimPrivateKey, ParsedEmail, SignerBuilder}; -use rsa::pkcs1::DecodeRsaPrivateKey; use std::time::Instant; fn email_text() -> String { @@ -55,32 +54,6 @@ fn main() { let email_text = email_text(); let email = ParsedEmail::parse(email_text).unwrap(); - for canon in [Type::Simple, Type::Relaxed] { - let private_key = - rsa::RsaPrivateKey::read_pkcs1_pem_file("crates/dkim/test/keys/2022.private").unwrap(); - let time = chrono::Utc.with_ymd_and_hms(2021, 1, 1, 0, 0, 1).unwrap(); - - let signer = SignerBuilder::new() - .with_signed_headers(["From", "Subject"]) - .unwrap() - .with_body_canonicalization(canon) - .with_header_canonicalization(canon) - .with_private_key(DkimPrivateKey::Rsa(private_key)) - .with_selector("s20") - .with_signing_domain("example.com") - .with_time(time) - .build() - .unwrap(); - - let start = Instant::now(); - let num_iters = 1_000; - for _ in 0..num_iters { - signer.sign(&email).unwrap(); - } - println!("{canon:?}: Did {num_iters} iters in {:?}", start.elapsed()); - } - - #[cfg(feature = "openssl")] for canon in [Type::Simple, Type::Relaxed] { let data = std::fs::read("./crates/dkim/test/keys/2022.private").unwrap(); let pkey = openssl::rsa::Rsa::private_key_from_pem(&data).unwrap(); diff --git a/crates/dkim/src/lib.rs b/crates/dkim/src/lib.rs index a509fba2..e7f3fa2e 100644 --- a/crates/dkim/src/lib.rs +++ b/crates/dkim/src/lib.rs @@ -2,14 +2,14 @@ use crate::errors::Status; use crate::hash::HeaderList; +use ed25519_dalek::pkcs8::DecodePrivateKey; use ed25519_dalek::SigningKey; use hickory_resolver::TokioAsyncResolver; use mailparsing::AuthenticationResult; -use rsa::pkcs1::DecodeRsaPrivateKey; -use rsa::pkcs8::DecodePrivateKey; -use rsa::{Pkcs1v15Sign, RsaPrivateKey, RsaPublicKey}; -use sha1::Sha1; -use sha2::Sha256; +use openssl::md::Md; +use openssl::pkey::PKey; +use openssl::pkey_ctx::PkeyCtx; +use openssl::rsa::{Padding, Rsa}; use std::collections::BTreeMap; #[macro_use] @@ -37,16 +37,14 @@ const DNS_NAMESPACE: &str = "_domainkey"; #[derive(Debug)] pub(crate) enum DkimPublicKey { - Rsa(RsaPublicKey), + Rsa(PKey), Ed25519(ed25519_dalek::VerifyingKey), } #[derive(Debug)] pub enum DkimPrivateKey { - Rsa(RsaPrivateKey), Ed25519(SigningKey), - #[cfg(feature = "openssl")] - OpenSSLRsa(openssl::rsa::Rsa), + OpenSSLRsa(Rsa), } impl DkimPrivateKey { @@ -54,41 +52,14 @@ impl DkimPrivateKey { pub fn rsa_key(data: &[u8]) -> Result { let mut errors = vec![]; - #[cfg(feature = "openssl")] - { - use openssl::rsa::Rsa; - - match Rsa::private_key_from_pem(data) { - Ok(key) => return Ok(Self::OpenSSLRsa(key)), - Err(err) => errors.push(format!("openssl private_key_from_pem: {err:#}")), - }; - match Rsa::private_key_from_der(data) { - Ok(key) => return Ok(Self::OpenSSLRsa(key)), - Err(err) => errors.push(format!("openssl private_key_from_der: {err:#}")), - }; - } - match RsaPrivateKey::from_pkcs1_der(data) { - Ok(key) => return Ok(Self::Rsa(key)), - Err(err) => errors.push(format!("from_pkcs1_der: {err:#}")), - } - match RsaPrivateKey::from_pkcs8_der(data) { - Ok(key) => return Ok(Self::Rsa(key)), - Err(err) => errors.push(format!("from_pkcs8_der: {err:#}")), - } - - match std::str::from_utf8(data) { - Ok(s) => { - match RsaPrivateKey::from_pkcs1_pem(s) { - Ok(key) => return Ok(Self::Rsa(key)), - Err(err) => errors.push(format!("from_pkcs1_pem: {err:#}")), - } - match RsaPrivateKey::from_pkcs8_pem(s) { - Ok(key) => return Ok(Self::Rsa(key)), - Err(err) => errors.push(format!("from_pkcs8_pem: {err:#}")), - } - } - Err(err) => errors.push(format!("from_pkcs1_pem: data is not UTF-8: {err:#}")), - } + match Rsa::private_key_from_pem(data) { + Ok(key) => return Ok(Self::OpenSSLRsa(key)), + Err(err) => errors.push(format!("openssl private_key_from_pem: {err:#}")), + }; + match Rsa::private_key_from_der(data) { + Ok(key) => return Ok(Self::OpenSSLRsa(key)), + Err(err) => errors.push(format!("openssl private_key_from_der: {err:#}")), + }; Err(DKIMError::PrivateKeyLoadError(errors.join(". "))) } @@ -134,17 +105,31 @@ fn verify_signature( public_key: DkimPublicKey, ) -> Result { Ok(match public_key { - DkimPublicKey::Rsa(public_key) => public_key - .verify( - match hash_algo { - hash::HashAlgo::RsaSha1 => Pkcs1v15Sign::new::(), - hash::HashAlgo::RsaSha256 => Pkcs1v15Sign::new::(), - hash => return Err(DKIMError::UnsupportedHashAlgorithm(format!("{:?}", hash))), - }, - header_hash, - signature, - ) - .is_ok(), + DkimPublicKey::Rsa(public_key) => { + let md = match hash_algo { + hash::HashAlgo::RsaSha1 => Md::sha1(), + hash::HashAlgo::RsaSha256 => Md::sha256(), + hash => return Err(DKIMError::UnsupportedHashAlgorithm(format!("{:?}", hash))), + }; + + let mut ctx = PkeyCtx::new(&public_key).map_err(|err| { + DKIMError::SignatureSyntaxError(format!("Error loading RSA public key: {err}")) + })?; + + ctx.verify_init().map_err(|err| { + DKIMError::UnknownInternalError(format!("ctx.verify_init failed: {err}")) + })?; + ctx.set_rsa_padding(Padding::PKCS1).map_err(|err| { + DKIMError::UnknownInternalError(format!("ctx.set_rsa_padding failed: {err}")) + })?; + ctx.set_signature_md(&md).map_err(|err| { + DKIMError::UnknownInternalError(format!("ctx.set_signature_md failed: {err}")) + })?; + match ctx.verify(header_hash, signature) { + Ok(result) => result, + Err(_) => false, + } + } DkimPublicKey::Ed25519(public_key) => { let mut sig_bytes = [0u8; ed25519_dalek::Signature::BYTE_SIZE]; if signature.len() != sig_bytes.len() { diff --git a/crates/dkim/src/public_key.rs b/crates/dkim/src/public_key.rs index bee62d23..2d2958c3 100644 --- a/crates/dkim/src/public_key.rs +++ b/crates/dkim/src/public_key.rs @@ -1,4 +1,5 @@ -use rsa::{pkcs1, pkcs8}; +use openssl::pkey::PKey; +use openssl::rsa::Rsa; use std::collections::HashMap; use crate::{dns, parser, DKIMError, DkimPublicKey, DNS_NAMESPACE}; @@ -56,11 +57,16 @@ pub(crate) async fn retrieve_public_key( })?; let key = if key_type == RSA_KEY_TYPE { DkimPublicKey::Rsa( - pkcs8::DecodePublicKey::from_public_key_der(&bytes) - .or_else(|_| pkcs1::DecodeRsaPublicKey::from_pkcs1_der(&bytes)) - .map_err(|err| { - DKIMError::KeyUnavailable(format!("failed to parse public key: {}", err)) - })?, + PKey::from_rsa( + Rsa::public_key_from_der(&bytes) + .or_else(|_| Rsa::public_key_from_der_pkcs1(&bytes)) + .map_err(|err| { + DKIMError::KeyUnavailable(format!("failed to parse public key: {}", err)) + })?, + ) + .map_err(|err| { + DKIMError::KeyUnavailable(format!("failed to parse public key: {}", err)) + })?, ) } else { let mut key_bytes = [0u8; ed25519_dalek::PUBLIC_KEY_LENGTH]; diff --git a/crates/dkim/src/sign.rs b/crates/dkim/src/sign.rs index b05194dd..23f01e09 100644 --- a/crates/dkim/src/sign.rs +++ b/crates/dkim/src/sign.rs @@ -2,9 +2,6 @@ use crate::header::DKIMHeaderBuilder; use crate::{canonicalization, hash, DKIMError, DkimPrivateKey, HeaderList, ParsedEmail, HEADER}; use data_encoding::BASE64; use ed25519_dalek::Signer as _; -use rsa::Pkcs1v15Sign; -use sha1::Sha1; -use sha2::Sha256; /// Builder for the Signer pub struct SignerBuilder { @@ -113,8 +110,6 @@ impl SignerBuilder { .private_key .ok_or(BuilderError("missing required private key"))?; let hash_algo = match private_key { - DkimPrivateKey::Rsa(_) => hash::HashAlgo::RsaSha256, - #[cfg(feature = "openssl")] DkimPrivateKey::OpenSSLRsa(_) => hash::HashAlgo::RsaSha256, DkimPrivateKey::Ed25519(_) => hash::HashAlgo::Ed25519Sha256, }; @@ -180,22 +175,9 @@ impl Signer { self.compute_header_hash(email, effective_header_list, dkim_header_builder.clone())?; let signature = match &self.private_key { - DkimPrivateKey::Rsa(private_key) => private_key - .sign( - match &self.hash_algo { - hash::HashAlgo::RsaSha1 => Pkcs1v15Sign::new::(), - hash::HashAlgo::RsaSha256 => Pkcs1v15Sign::new::(), - hash => { - return Err(DKIMError::UnsupportedHashAlgorithm(format!("{:?}", hash))) - } - }, - &header_hash, - ) - .map_err(|err| DKIMError::FailedToSign(err.to_string()))?, DkimPrivateKey::Ed25519(signing_key) => { signing_key.sign(&header_hash).to_bytes().into() } - #[cfg(feature = "openssl")] DkimPrivateKey::OpenSSLRsa(private_key) => { use foreign_types::ForeignType; @@ -394,7 +376,6 @@ DKIM-Signature: v=1; a=rsa-sha256; d=example.com; s=s20; c=simple/simple;\r ); } - #[cfg(feature = "openssl")] #[test] fn test_sign_rsa_openssl() { let raw_email = r#"Subject: subject