From 78ea9cb7dabb9d360aa1ad2d8e91f64c1e7d62eb Mon Sep 17 00:00:00 2001
From: Tony Arcieri <bascule@gmail.com>
Date: Wed, 26 Apr 2023 10:39:18 -0600
Subject: [PATCH] Impl `ZeroizeOnDrop` for `RsaPrivateKey`+newtypes (#311)

`RsaPrivateKey` self-zeroizes on drop, so add the `ZeroizeOnDrop` marker
trait to `RsaPrivateKey` and all newtypes thereof, i.e. `DecryptingKey`
and `SigningKey` for the various padding modes.

This also removes the `Zeroize` impl on `RsaPrivateKey`, since it
self-zeroizes on `Drop`, and allowing `Zeroize` might accidentally
permit use-after-zeroize vulnerabilities.
---
 Cargo.toml      |  2 +-
 src/key.rs      | 21 ++++++---------------
 src/oaep.rs     | 12 ++++++++++--
 src/pkcs1v15.rs |  6 +++++-
 src/pss.rs      |  5 +++++
 5 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 12618e0f..ffe5a5a5 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -25,7 +25,7 @@ digest = { version = "0.10.5", default-features = false, features = ["alloc", "o
 pkcs1 = { version = "0.7.5", default-features = false, features = ["alloc", "pkcs8"] }
 pkcs8 = { version = "0.10.2", default-features = false, features = ["alloc"] }
 signature = { version = "2", default-features = false , features = ["digest", "rand_core"] }
-zeroize = { version = "1", features = ["alloc"] }
+zeroize = { version = "1.5", features = ["alloc"] }
 
 # optional dependencies
 serde = { version = "1.0.103", optional = true, default-features = false, features = ["derive"] }
diff --git a/src/key.rs b/src/key.rs
index 09232282..9cfda579 100644
--- a/src/key.rs
+++ b/src/key.rs
@@ -11,7 +11,7 @@ use num_traits::{FromPrimitive, One, ToPrimitive};
 use rand_core::CryptoRngCore;
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
-use zeroize::Zeroize;
+use zeroize::{Zeroize, ZeroizeOnDrop};
 
 use crate::algorithms::generate::generate_multi_prime_key_with_exp;
 use crate::dummy_rng::DummyRng;
@@ -61,22 +61,11 @@ impl Hash for RsaPrivateKey {
     }
 }
 
-impl Zeroize for RsaPrivateKey {
-    fn zeroize(&mut self) {
-        self.d.zeroize();
-        for prime in self.primes.iter_mut() {
-            prime.zeroize();
-        }
-        self.primes.clear();
-        if self.precomputed.is_some() {
-            self.precomputed.take().unwrap().zeroize();
-        }
-    }
-}
-
 impl Drop for RsaPrivateKey {
     fn drop(&mut self) {
-        self.zeroize();
+        self.d.zeroize();
+        self.primes.zeroize();
+        self.precomputed.zeroize();
     }
 }
 
@@ -87,6 +76,8 @@ impl Deref for RsaPrivateKey {
     }
 }
 
+impl ZeroizeOnDrop for RsaPrivateKey {}
+
 #[derive(Debug, Clone)]
 pub(crate) struct PrecomputedValues {
     /// D mod (P-1)
diff --git a/src/oaep.rs b/src/oaep.rs
index 685807ca..3941e709 100644
--- a/src/oaep.rs
+++ b/src/oaep.rs
@@ -3,16 +3,17 @@
 //! # Usage
 //!
 //! See [code example in the toplevel rustdoc](../index.html#oaep-encryption).
+
 use alloc::boxed::Box;
 use alloc::string::{String, ToString};
 use alloc::vec::Vec;
 use core::fmt;
 use core::marker::PhantomData;
-use rand_core::CryptoRngCore;
 
 use digest::{Digest, DynDigest, FixedOutputReset};
 use num_bigint::BigUint;
-use zeroize::Zeroizing;
+use rand_core::CryptoRngCore;
+use zeroize::{ZeroizeOnDrop, Zeroizing};
 
 use crate::algorithms::oaep::*;
 use crate::algorithms::pad::{uint_to_be_pad, uint_to_zeroizing_be_pad};
@@ -411,6 +412,13 @@ where
     }
 }
 
+impl<D, MGD> ZeroizeOnDrop for DecryptingKey<D, MGD>
+where
+    D: Digest,
+    MGD: Digest + FixedOutputReset,
+{
+}
+
 #[cfg(test)]
 mod tests {
     use crate::key::{RsaPrivateKey, RsaPublicKey};
diff --git a/src/pkcs1v15.rs b/src/pkcs1v15.rs
index ce7e769b..0810c86f 100644
--- a/src/pkcs1v15.rs
+++ b/src/pkcs1v15.rs
@@ -24,7 +24,7 @@ use signature::{
     DigestSigner, DigestVerifier, Keypair, RandomizedDigestSigner, RandomizedSigner,
     SignatureEncoding, Signer, Verifier,
 };
-use zeroize::Zeroizing;
+use zeroize::{ZeroizeOnDrop, Zeroizing};
 
 use crate::algorithms::pad::{uint_to_be_pad, uint_to_zeroizing_be_pad};
 use crate::algorithms::pkcs1v15::*;
@@ -418,6 +418,8 @@ where
     }
 }
 
+impl<D> ZeroizeOnDrop for SigningKey<D> where D: Digest {}
+
 impl<D> Signer<Signature> for SigningKey<D>
 where
     D: Digest,
@@ -731,6 +733,8 @@ impl EncryptingKeypair for DecryptingKey {
     }
 }
 
+impl ZeroizeOnDrop for DecryptingKey {}
+
 mod oid {
     use const_oid::ObjectIdentifier;
 
diff --git a/src/pss.rs b/src/pss.rs
index d6e936d9..56d2a2b4 100644
--- a/src/pss.rs
+++ b/src/pss.rs
@@ -30,6 +30,7 @@ use signature::{
     hazmat::{PrehashVerifier, RandomizedPrehashSigner},
     DigestVerifier, Keypair, RandomizedDigestSigner, RandomizedSigner, SignatureEncoding, Verifier,
 };
+use zeroize::ZeroizeOnDrop;
 
 use crate::algorithms::pad::{uint_to_be_pad, uint_to_zeroizing_be_pad};
 use crate::algorithms::pss::*;
@@ -483,6 +484,8 @@ where
     }
 }
 
+impl<D> ZeroizeOnDrop for SigningKey<D> where D: Digest {}
+
 /// Signing key for producing "blinded" RSASSA-PSS signatures as described in
 /// [draft-irtf-cfrg-rsa-blind-signatures](https://datatracker.ietf.org/doc/draft-irtf-cfrg-rsa-blind-signatures/).
 #[derive(Debug, Clone)]
@@ -656,6 +659,8 @@ where
     }
 }
 
+impl<D> ZeroizeOnDrop for BlindedSigningKey<D> where D: Digest {}
+
 /// Verifying key for checking the validity of RSASSA-PSS signatures as
 /// described in [RFC8017 § 8.1].
 ///