Replace Secret's Drop supertrait with ZeroizeOnDrop by tad-fr · Pull Request #36 · bcgit/bc-rust

tad-fr · 2026-06-21T14:45:04Z

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 · 2026-06-21T20:39:49Z


 /// A matrix over the ML-DSA ring.
-#[derive(Clone)]
+#[derive(Clone, ZeroizeOnDrop)]


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

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.

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?

ounsworth · 2026-06-21T20:42:03Z

-#[derive(Clone)]
+use zeroize::ZeroizeOnDrop;
+
+#[derive(Clone, ZeroizeOnDrop)]


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

Same reason as mldsa/matrix

ounsworth · 2026-06-21T20:42:20Z

-    pub(crate) vec: [Polynomial; k],
-}
+
+#[derive(Clone, ZeroizeOnDrop)]


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

Same reason as mldsa/matrix

ounsworth · 2026-06-21T20:43:14Z

 }

+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> {}


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

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.

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

npajkovsky · 2026-06-22T05:42:04Z

npajkovsky · 2026-06-22T06:05:16Z

+/// Input: integer array 𝐹 ∈ ℤ_M^256, where 𝑚 = 2^𝑑 if 𝑑 < 12, and 𝑚 = 𝑞 if 𝑑 = 12.
+/// Output: byte array 𝐵 ∈ 𝔹32𝑑.
+/// Note: this is exposed publicly only for testing purposes and there is no good reason to use it in production code.
+pub fn byte_encode<const d: usize, const PACK_LEN: usize>(F: &Polynomial) -> [u8; PACK_LEN] {


Is there any driver for making the function public?

Did you read the comment on it? 😉

This was not a change made by Tamish -- it is unrelated to this change set; it just got pulled in when I rebased from main to release/0.1.2alpha.

Basically, the crucible harness needs direct access to this function so that it can do more invasive correctness testing than what you can do via the regular public APIs.
I agree that this really shouldn't be public; but making it public in exchange for better test coverage seems like an ok tradeoff?

npajkovsky · 2026-06-22T06:05:30Z

 /// Output: integer array 𝐹 ∈ ℤ256 , where 𝑚 = 2𝑑 if 𝑑 < 12 and 𝑚 = 𝑞 if 𝑑 = 12.
-pub(crate) fn byte_decode<const d: usize, const PACK_LEN: usize>(B: &[u8; PACK_LEN]) -> Polynomial {
+/// Note: this is exposed publicly only for testing purposes and there is no good reason to use it in production code.
+pub fn byte_decode<const d: usize, const PACK_LEN: usize>(B: &[u8; PACK_LEN]) -> Polynomial {


npajkovsky · 2026-06-22T06:05:43Z

 /// Output: array 𝑎_hat ∈ ℤ256 ▷ the coefficients of the NTT of a polynomial
-pub(crate) fn sample_ntt(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial {
+/// Note: this is exposed publicly only for testing purposes and there is no good reason to use it in production code.
+pub fn sample_ntt(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial {


npajkovsky · 2026-06-22T06:09:44Z

-
-mod aux_functions;
+// Exposed only for testing purposes
+pub mod aux_functions;


no need to be pub

…ment DefaultIsZeroes for SecurityStrength

ounsworth · 2026-06-22T18:30:45Z

@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 · 2026-06-22T18:32:01Z

@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?

ounsworth changed the base branch from main to release/0.1.2alpha June 21, 2026 19:09

ounsworth requested changes Jun 21, 2026

View reviewed changes

npajkovsky reviewed Jun 22, 2026

View reviewed changes

tad-fr added 8 commits June 22, 2026 13:48

add zeroize as a dependency in bouncycastle-core

4191905

Replace Drop supertrait for Secret trait with ZeroizeOnDrop and imple…

e6de01c

…ment DefaultIsZeroes for SecurityStrength

Derive ZeroizeOnDrop to replace zeroize() for KeyMaterial

e867483

propagate zeroize in bouncycastle-mldsa

8ef55b2

propagate zeroize inbouncycastle-mldsa-lowmemory

2e3470d

propagate zeroize in bouncycastle-mlkem

1201d90

propagate zeroize in bouncycastle-mlkem-lowmemory

39c3fc7

fix whitespace overwrites

910a831

tad-fr force-pushed the secret-zeroize branch from 4bcb2c0 to 910a831 Compare June 22, 2026 09:42

tad-fr added 4 commits June 22, 2026 15:59

move zeroize version declaration to top-level cargo.toml

f14041e

Derive enum defaults over manual implementation

fb59a59

derive Zeroize for key_material

ef41b40

fix code style

7a36f8a

Conversation

tad-fr commented Jun 21, 2026

Description

Issue

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ounsworth Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ounsworth commented Jun 22, 2026

Uh oh!

ounsworth commented Jun 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ounsworth Jun 22, 2026 •

edited

Loading