Replace Secret's Drop supertrait with ZeroizeOnDrop

[tad-fr]

Description

First pass integrating the zeroize crate.
Currently only the structs that are already implementing Secret are refactored. The intention is that it allows the PR to be relatively small and also settle on the pattern to be followed before full propagation.

Issue

Relates to: #32

[ounsworth]

This shouldn't need to be Zeroized because Polynomial is where the secret data is, and it is where we should impl ZeroizeOnDrop.

[tad-fr]

Deriving ZeroizeOnDrop requires all fields to implement it as well. I chose to derive it on these types with the understanding that rust zero-cost abstractions would result in no additional overhead. I could be wrong of course in which case we could remove the derivations on MLDSAPrivateKey,Matrix and Vector types and implement a Drop() for MLDSAPrivateKey.

[ounsworth]

I think I maybe see your point: you've tagged struct MLDSAPrivateKey with #[derive(ZeroizeOnDrop)], which I think makes sense for optics, but then in requires that all of its members (Vector, in this case) also impl ZeroizeOnDrop.

But I notice that you have not impl'd ZeroizeOnDrop for struct MLKEMPRivateKey, which I assume you can't because that contains an MLKEMPublicKey member, which has no reason to be zeroized.

Which brings up another point:
I'm not yet sure that this is good or bad; I'm just thinking out loud here.
The full struct is:

pub struct MLDSAPrivateKey<
    const k: usize,
    const l: usize,
    const eta: usize,
    const SK_LEN: usize,
    const PK_LEN: usize,
> {
    rho: [u8; 32],
    K: [u8; 32],
    tr: [u8; 64],
    s1_hat: Vector<l>,
    s2_hat: Vector<k>,
    t0_hat: Vector<k>,
    seed: Option<KeyMaterial<32>>,
}

Of those, K, s1, s2, t0, seed are secret data, but rho and tr are the public key and don't strictly need to be zeroized. In this particular case, zeroizing an extra 64 bytes is not a big deal, but it does mean that this pattern where all members also need to impl ZeroizeOnDrop might not give us fine-enough contral if a secret object contains a large non-secret object.

And you actually have an example of that: crypto/mlkem/src/mlkem_keys.rs in your PR has:

#[derive(Clone)]
pub struct MLKEMPrivateKey<
    const k: usize,
    PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>,
    const SK_LEN: usize,
    const PK_LEN: usize,
> {
    s_hat: Vector<k>,
    ek: PK,
    pk_hash: [u8; 32],
    z: [u8; 32],
    seed_d: Option<[u8; 32]>,
}

It does not impl ZeroizeOnDrop, probably because MLKEMPublicKeyInternalTrait does not, so this won't compile, but absolutely z and seed_d are secret and need to be zeroized. Is there another way to do this more surgically? For example, instead of using the derive macro, can we impl ZeroizeOnDrop manually and delete only the things that need to be deleted?

[npajkovsky]

Is there any reason to have pub key in priv key struct? Those can be separate structs and passed around as tuples.

@ounsworth yes, you're right. The Derive is good only when you know that you want to securely clean, for those who need a partial cleaning, impl ZeroizeOnDrop must be manually implemented.

[tad-fr]

Yes indeed the necessity is to have an impl ZeroizeOnDrop and an impl Drop. This can be done via a marco or manually.

I feel the detailed explanation for the mlkem below covers this as well. Please let me know if i can provide further clarification.

[ounsworth]

Is there any reason to have pub key in priv key struct? Those can be separate structs and passed around as tuples.

The simple reason is that FIPS 203 defines the private key as containing the public key and exporting the private key into the NIST encoding requires embedding the public key inside the private key.

The more complex reason is that you cannot use an ML-KEM private key by itself without the public key because Decapsulation (aka decryption) requires both the private key and the public key, which is why FIPS 203 includes the pub inside the priv.

[ounsworth]

Could you explain for my understanding why here you provide an empty impl, but in other places you use the #derive macro?

[tad-fr]

I noticed the existing Drop() implementation had some additional logic in the form of an if statement. I tried to take the prudent approach as a derived Drop() would not have the same check.

[ounsworth]

I don't understand. How does an empty impl help with that?

[tad-fr]

Since MLKEMPrivateKey implements Secret, It requires a ZeroizeOnDrop implementation as well; In much the same fashion as we have an empty Secret Impl.

When we derive ZeroizeOnDrop, this is exactly what the macro is producing.
If we were to derive it on MLKEMPrivateKey, which is defined as:

pub struct MLKEMPrivateKey<
    const k: usize,
    PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>,
    const SK_LEN: usize,
    const PK_LEN: usize,
> {
    s_hat: Vector<k>,
    ek: PK,
    pk_hash: [u8; 32],
    z: [u8; 32],
    seed_d: Option<[u8; 32]>,
}

the macro would produce:

impl<
    const k: usize,
    PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>,
    const SK_LEN: usize,
    const PK_LEN: usize,
> ZeroizeOnDrop for MLKEMPrivateKey<k, PK, SK_LEN, PK_LEN> {}

impl<
    const k: usize,
    PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>,
    const SK_LEN: usize,
    const PK_LEN: usize,
> Drop for MLKEMPrivateKey<k, PK, SK_LEN, PK_LEN>
{
    fn drop(&mut self) {
        self.s_hat.zeroize();
        self.ek.zeroize();
        self.pk_hash.zeroize();
        self.z.zeroize();
        self.seed_d.zeroize();
    }
}

Since the preexisting Drop() definition did not match the macro output, I chose to not derive it and rather just added the empty impl block.

Although you could argue I should've updated the fill calls with zeroize() => Which I have just thought of now.
This would alter the Drop as such:

impl<
    const k: usize,
    PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>,
    const SK_LEN: usize,
    const PK_LEN: usize,
> Drop for MLKEMPrivateKey<k, PK, SK_LEN, PK_LEN>
{
    fn drop(&mut self) {
        // s_hat, has its own zeroizing drop
        self.pk_hash.zeroize();
        self.z.zeroize();
        if self.seed_d.is_some() {
               self.seed_d.zeroize();
        }
    }
}

---

All of this has made me realize another point: Since our Secret now has ZeroizeOnDrop as a supertrait, ZeroizeOnDrop itself does not have Drop as a supertrait. ZeroizeOnDrop is just a marker trait. This means the compiler no longer enforces the implementation of a Drop(). The developer must now be aware of the above and remember to implement a Drop with the empty ZeroizeOnDrop impl. This does not seem very developer friendly.

I see 2 resolutions as of now:

• Add Drop back to Secret: I cannot see any obvious downside with this?
• Add test coverage for Drop

Curious to hear your opinions.

[ounsworth]

Hmm. Now I'm a bit confused. I thought ZeroizeOnDrop replaced Drop. Why would you need to implement both?

Also, can we not manually impl ZeroizeOnDrop here as:

impl<
    const k: usize,
    PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>,
    const SK_LEN: usize,
    const PK_LEN: usize,
> ZeroizeOnDrop for MLKEMPrivateKey<k, PK, SK_LEN, PK_LEN>
{
    fn drop(&mut self) {
        // s_hat, has its own zeroizing drop
        self.z.zeroize();
        if self.seed_d.is_some() {
               self.seed_d.zeroize();
        }
    }
}

[tad-fr]

To the first part of your query:

I believe ZeroizeOnDrop is not a subtrait of Drop to allow flexibility.

Simplified scenario:
Person A creates a crate with a type that has some secret data inside, say, SecretKey { inner: SecretDataType }. They implement Zeroize and Drop for SecretDataType, and stop at that - the secret data is now safe (to the extend Rust can guarantee it), the drop() of SecretKey will call the zeroizing drop() of SecretDataType, so there's no need to define anything for SecretKey itself.

Person B uses A's crate and wants to use Secret<SecretKey>. But they cannot, because SecretKey does not implement Zeroize - so they have to wrap it in a newtype and implement an empty Zeroize for it. And they still cannot assert in code that SecretKey keeps its secret - they have to rely on the docs.

So what are the responsibilities of A here? Should they define an (empty) Zeroize for SecretKey? Should they export a Secret<Box<SecretKey>>?

ZeroizeOnDrop aims to solve this. A could implement it for SecretKey, and Secret can have it as a supertrait. It does not provide any methods, but marks crates as safe, allowing for a scalable API. This is the setup we've gone with so far. Further comparison in next section.

---

Review bouncycastles' Secret's supertraits:

Adding Drop as a supertrait would be counterproductive. Since ZeroizeOnDrop is a marker trait, its the best approach for us in my opinion.

Using hypothetical MLKEMPrivateKey as an example to illustrate. The following might not match the code.

pub struct MLKEMPrivateKey {
    s_hat: Vector<k>,
}

pub struct Vector {
    vec: [Polynomial; k],
}

pub struct Polynomial {  
    coeffs: [i16; N],
}

Scenario 1 : Secret is a subtrait of Drop only (Setup before this PR)

Secret: Drop + Debug + Display {}
To implement Secret on MLKEMPrivateKey we need to trust that the composed types Zeroize properly. This works but the behavior needs to be document instead of the compiler enforcing it. But this allows us to skip Vector boilerplate.

impl Drop for MLKEMPrivateKey {
    fn drop(&mut self) {
        s_hat.zeroize();
        }
    }
}

impl Drop for Polynomial {
    fn drop(&mut self) {
        coeffs.zeroize();
        }
    }
}

Scenario 2 : Secret is a subtrait of Drop AND ZeroizeOnDrop

Secret: ZeroizeOnDrop + Drop + Debug + Display {}
To implement Secret on MLKEMPrivateKey we need to implement both for all types. The compiler is now aware of Zeroize safety but we have empty Drop impl for intermediate type Vector.

impl ZeroizeOnDrop for MLKEMPrivateKey {}

impl Drop for MLKEMPrivateKey {
    fn drop(&mut self) {
        s_hat.zeroize();
        }
    }
}

impl ZeroizeOnDrop for Vector {}

impl Drop for Vector {}

impl ZeroizeOnDrop for Polynomial {}

impl Drop for Polynomial {
    fn drop(&mut self) {
        coeffs.zeroize();
        }
    }
}

Scenario 3 : Secret is a subtrait of ZeroizeOnDrop only (Current Setup)

Secret: ZeroizeOnDrop + Debug + Display {}

To implement Secret on MLKEMPrivateKey we need to implement ZeroizeOnDrop for all types. The compiler is now aware of Zeroize safety. No empty drops are necessary on types that don't need it.

impl ZeroizeOnDrop for MLKEMPrivateKey {}

impl Drop for MLKEMPrivateKey {
    fn drop(&mut self) {
        s_hat.zeroize();
        }
    }
}

impl ZeroizeOnDrop for Vector {}

impl ZeroizeOnDrop for Polynomial {}

impl Drop for Polynomial {
    fn drop(&mut self) {
        coeffs.zeroize();
        }
    }
}

---

To the second part of your query:

The Drop trait is not implemented on types by default, its an additional layer before the field destructors are run.

Even if that was the case (drop was implemented for all types by default), we could not use the syntax you have specified. You cannot "override" an existing lifecycle hook or an implicit structural behavior simply by changing a local method declaration; behavior must always be mapped through a discrete trait definition by declaring impl Drop {}. The <T as core::ops::Drop>::drop cannot be overriden from a impl ZeroizeOnDrop. Rust does not inherit. So in order to zeroize a value, a Drop impl must exist (with or without the zeroize crate) as it does stuff that the default "drop glue" does not.

[ounsworth]

I see. I guess I should have just done the research myself from the beginning and read the docs for the Zeroize crate.

https://docs.rs/zeroize/latest/zeroize/trait.ZeroizeOnDrop.html

I didn't realize that ZeroizeOnDrop was itself just an empty marker trait.

So now I don't really understand the value that we're getting from the intrinsics if it's just an empty marker trait and we have to impl Drop and Zeroize ourselves. Maybe I need to find time to actually play with this PR in my own IDE.

[npajkovsky]

Maybe we're looking at the problem from the wrong angle. Instead of implementing Zeroize on structs that mix secret and non-secret data — which forces a massive chain of Zeroize impls onto types that don't need it — we can do something better.
Let's define a Secret that requires T: Zeroize:

Let's define struct Secret that requires T: Zerosize

pub struct Secret<T: Zeroize> {
    inner: Box<T>, // heap-pinned, moves don't copy the bytes
}
// Explicit wrapper returned by expose_secret()
// Makes secret access grep-able and visible in code review
pub struct ExposeSecret<'a, T: Zeroize>(pub &'a T);

impl<T: Zeroize> Secret<T> {
    pub fn new(value: T) -> Self {
        Self {
            inner: Box::new(value),
        }
    }

    // Allocate the destination first, hand the keygen a &mut MaybeUninit<T>, and have it write straight into the heap (or, later, the mlock'd arena). The full T never materializes on the stack.
    pub fn new_with(init: impl FnOnce(&mut MaybeUninit<T>)) -> Self {
        let mut uninit = Box::<T>::new_uninit();
        init(&mut *uninit);
        // SAFETY: `init` fully initialized the value.
        Self { inner: unsafe { uninit.assume_init() } }
    }

    // Only way to access the value — name is intentionally loud
    pub fn expose_secret(&self) -> ExposeSecret<'_, T> {
        ExposeSecret(&self.inner)
    }
}

impl<T: Zeroize> Drop for Secret<T> {
    fn drop(&mut self) {
        self.inner.zeroize();
    }
}

// Redact in logs/debug output
impl<T: Zeroize> fmt::Debug for Secret<T> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "Secret([REDACTED])")
    }
}

impl<T: Zeroize> fmt::Display for Secret<T> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "Secret([REDACTED])")
    }
}

Then, the Vector we can either have

pub(crate) struct Vector<const k: usize> {
    pub(crate) vec: [Secret<Polynomial>; k],
}

or vector can also have a more distinguishing type.

pub(crate) struct SecretVector<const k: usize> {
    pub(crate) vec: [Secret<Polynomial>; k],
}

With that said, we can then have MLKEMPrivateKey defined with secrets

pub struct MLKEMPrivateKey<
    const k: usize,
    PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>,
    const SK_LEN: usize,
    const PK_LEN: usize,
> {
    s_hat: Vector<k>, // or SecretVector
    ek: PK,
    pk_hash: Secret<[u8; 32]>,
    z: Secret<[u8; 32]>,
    seed_d: Option<Secret<[u8; 32]>>,
}

That would eliminate Zeroize for MLKEMPrivateKey, and skip pub key entirely.

The story about handling secrets does not end with clearing them. There are two other requirements that have to be done.

1. Swap to disk
2. Timing side-channels in comparison

It will be much easier to handle those points if the secret is wrapped into own type.

Note, it requires a heap, which we don't have now.

1. https://benma.github.io/2020/10/16/rust-zeroize-move.html

[npajkovsky]

Uff, we reall need allocator

#![no_std]
#![no_main]

use core::fmt::{self, Write};

use zeroize::{Zeroize, ZeroizeOnDrop};

// --- no_std plumbing --------------------------------------------------------

#[link(name = "c")]
unsafe extern "C" {
    fn write(fd: i32, buf: *const u8, count: usize) -> isize;
    fn exit(status: i32) -> !;
}

#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
    unsafe { exit(1) };
}

// libcore's eh_frame references this even with panic = "abort"; it is never called
#[unsafe(no_mangle)]
pub extern "C" fn rust_eh_personality() {}

struct Stdout;

impl fmt::Write for Stdout {
    fn write_str(&mut self, s: &str) -> fmt::Result {
        let buf = s.as_bytes();
        let mut written = 0;
        while written < buf.len() {
            let n = unsafe { write(1, buf[written..].as_ptr(), buf.len() - written) };
            if n < 0 {
                return Err(fmt::Error);
            }
            written += n as usize;
        }
        Ok(())
    }
}

macro_rules! println {
    () => { let _ = <$crate::Stdout as core::fmt::Write>::write_str(&mut $crate::Stdout, "\n"); };
    ($($arg:tt)*) => {{ core::writeln!($crate::Stdout, $($arg)*).ok(); }};
}

#[derive(Debug)]
struct EncryptionKey([u8; 4]);

impl Drop for EncryptionKey {
    fn drop(&mut self) {
        println!("Pointer when zeroing: {:p}", self.0.as_ptr());
        self.0.zeroize();
        println!("Zeroed (before Box::drop): {:?}", self.0);
    }
}

fn get_encryption_key() -> EncryptionKey {
    let key = EncryptionKey(*b"AKey");
    println!("Pointer at creation: {:p}", key.0.as_ptr());
    key
}

#[unsafe(no_mangle)]
pub extern "C" fn main(_argc: i32, _argv: *const *const u8) -> i32 {
    let encryption_key = get_encryption_key();
    let ptr = encryption_key.0.as_ptr();

    println!("Pointer when using: {:p}", encryption_key.0.as_ptr());
    println!("Using key to encrypt stuff: {:?}", encryption_key);

    println!("About to drop.");
    drop(encryption_key);
    println!("Dropped.");

    println!("Memory {:p}: {:?}", ptr, unsafe {
        core::slice::from_raw_parts(ptr, 4)
    });

    0
}

Pointer at creation: 0x16f66615c
Pointer when using: 0x16f666214
Using key to encrypt stuff: EncryptionKey([65, 75, 101, 121])
About to drop.
Pointer when zeroing: 0x16f6661a8
Zeroed (before Box::drop): [0, 0, 0, 0]
Dropped.
Memory 0x16f666214: [65, 75, 101, 121]

[npajkovsky]

ditto.

[npajkovsky]

ditto.

[ounsworth]

ditto.

[ounsworth]

@tad-fr I made some cleanup changes to your branch. I don't know how to push those changes back to your origin (there's probably a way in git, and maybe I'll have time to google it later today), so for now it's here:

https://github.com/ounsworth/bc-rust/tree/tad-fr-secret-zeroize

[ounsworth]

@tad-fr , @npajkovsky I have Resolved most of the discussions on this thread -- this is getting close to being ready to merge, but there are a few still open. The big one is how to deal with structs where only part of the content is secret and needs to be zeroized. Can we impl ZeroizeOnDrop manually instead of via a derive macro?