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
2026-09-10 04:22:13 +00:00 · 2024-04-10 13:26:49 -07:00
parent f0fdec1eb0
commit fa1dec61a2
7 changed files with 56 additions and 196 deletions
@@ -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",
 ]

@@ -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"
@@ -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();
@@ -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();
@@ -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<openssl::pkey::Public>),
    Ed25519(ed25519_dalek::VerifyingKey),
 }

 #[derive(Debug)]
 pub enum DkimPrivateKey {
-    Rsa(RsaPrivateKey),
    Ed25519(SigningKey),
-    #[cfg(feature = "openssl")]
-    OpenSSLRsa(openssl::rsa::Rsa<openssl::pkey::Private>),
+    OpenSSLRsa(Rsa<openssl::pkey::Private>),
 }

 impl DkimPrivateKey {
@@ -54,41 +52,14 @@ impl DkimPrivateKey {
    pub fn rsa_key(data: &[u8]) -> Result<Self, DKIMError> {
        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<bool, DKIMError> {
    Ok(match public_key {
-        DkimPublicKey::Rsa(public_key) => public_key
-            .verify(
-                match hash_algo {
-                    hash::HashAlgo::RsaSha1 => Pkcs1v15Sign::new::<Sha1>(),
-                    hash::HashAlgo::RsaSha256 => Pkcs1v15Sign::new::<Sha256>(),
-                    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() {
@@ -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];
@@ -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::<Sha1>(),
-                        hash::HashAlgo::RsaSha256 => Pkcs1v15Sign::new::<Sha256>(),
-                        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