Use zeroize for secure secret cleanup#3411
Conversation
Add the zeroize crate to securely clear cryptographic keys and secrets from memory when they are dropped, preventing sensitive data from persisting after Rust objects are deallocated. Changes: - Add zeroize 1.8 with derive feature as a workspace dependency - Derive Zeroize/ZeroizeOnDrop on HardwareDerivedKeys (AES + HMAC keys) - Derive Zeroize/ZeroizeOnDrop on Keys (VMGS ingress/egress keys) - Derive Zeroize/ZeroizeOnDrop on DekKp, GspKp, KeyProtector, HardwareKeyProtector, and GuestSecretKey structs - Replace .fill(0) with .zeroize() in vmgs_impl.rs and lib.rs to prevent dead-store elimination by the compiler
There was a problem hiding this comment.
Pull request overview
This PR improves secret-handling hygiene across VMGS and Underhill attestation by adopting the zeroize crate for explicit, compiler-resistant memory clearing of cryptographic material (both via ZeroizeOnDrop and via replacing manual .fill(0) calls).
Changes:
- Add
zeroize(withderive) as a workspace dependency and wire it into affected crates. - Apply
Zeroize/ZeroizeOnDropto key/secret-bearing structs (protocol VMGS structs and Underhill-derived key containers). - Replace manual zeroing (
.fill(0)) with.zeroize()in key cleanup paths.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vm/vmgs/vmgs/src/vmgs_impl.rs | Uses .zeroize() for datastore key removal when encryption is enabled. |
| vm/vmgs/vmgs/Cargo.toml | Adds zeroize dependency to the vmgs crate. |
| openhcl/underhill_attestation/src/lib.rs | Derives zeroize traits for Keys and replaces a secret buffer .fill(0) with .zeroize(). |
| openhcl/underhill_attestation/src/hardware_key_sealing.rs | Derives zeroize traits for HardwareDerivedKeys. |
| openhcl/underhill_attestation/Cargo.toml | Adds zeroize dependency for Underhill attestation. |
| openhcl/openhcl_attestation_protocol/src/vmgs.rs | Derives zeroize traits for VMGS protocol structs and skips zeroizing non-secret header fields. |
| openhcl/openhcl_attestation_protocol/Cargo.toml | Adds zeroize dependency for the protocol crate. |
| Cargo.toml | Adds zeroize as a workspace dependency (with derive). |
| Cargo.lock | Locks zeroize / zeroize_derive and updates dependents. |
Address review feedback: zeroize is only used behind cfg(feature = encryption) in vmgs_impl.rs, so make it an optional dependency gated on the encryption feature rather than an unconditional dependency.
|
|
||
| // Remove the corresponding datastore_key | ||
| temp_state.datastore_keys[key_index].fill(0); | ||
| temp_state.datastore_keys[key_index].zeroize(); |
There was a problem hiding this comment.
Do we not want to add ZeroizeOnDrop to all VmgsDatastoreKeys?
There was a problem hiding this comment.
VmgsDatastoreKey is a type alias (type VmgsDatastoreKey = [u8; 32]) in vmgs_format, so we can't derive ZeroizeOnDrop on it directly. The keys are stored as datastore_keys: [VmgsDatastoreKey; 2] inside VmgsState, a private runtime struct in vmgs_impl.rs.
Options for broader coverage would be either converting it to a newtype struct (touching vmgs_format which is no_std and all consumers), or adding a Drop impl to VmgsState that zeroizes datastore_keys. I'll file a follow-up for that — this PR at least fixes the dead-store-elimination bug at this specific call site by switching from .fill(0) to .zeroize().
| key_protector.dek[key_protector.active_kp as usize % NUMBER_KP] | ||
| .dek_buffer | ||
| .fill(0); | ||
| .zeroize(); |
There was a problem hiding this comment.
Do we not want to add ZeroizeOnDrop to all DekKps? #Resolved
There was a problem hiding this comment.
DekKp already has #[derive(Zeroize, ZeroizeOnDrop)] in this PR (see the changes in openhcl/openhcl_attestation_protocol/src/vmgs.rs), so all DekKp instances — including the ones inside KeyProtector.dek[] — will have their dek_buffer securely zeroized when dropped.
This explicit .zeroize() call is technically redundant now (the ZeroizeOnDrop on the parent KeyProtector will cascade to its DekKp fields), but I kept it as a 1:1 replacement of the original .fill(0) for defense-in-depth — it ensures the buffer is cleared before the struct is written back to VMGS on line 1345, not just when the struct is eventually dropped. Happy to remove it if you'd prefer to rely solely on the drop behavior.
| .dek_buffer | ||
| .fill(0); | ||
| .zeroize(); | ||
| key_protector.gsp[key_protector.active_kp as usize % NUMBER_KP].gsp_length = 0; |
There was a problem hiding this comment.
Do we need to do something more to clear the gsp than just setting length to 0? #Resolved
There was a problem hiding this comment.
Good catch — setting gsp_length = 0 only clears the length field, leaving the gsp_buffer: [u8; 512] contents intact in the struct that gets written back to VMGS on line 1345. While GspKp now has ZeroizeOnDrop (so the buffer will be zeroized when the KeyProtector is eventually dropped), we should clear it before persisting.
Added gsp_buffer.zeroize() alongside the length clear for consistency with the dek_buffer treatment.
There was a problem hiding this comment.
I think I'm fine with the code that currently exists here, however I do think we need to make sure to take a careful pass over the code to ensure that what we have here is sufficient. Namely, that we're calling zeroize in every location that really needs it and haven't missed any, that keys aren't escaping zeroization through accidental copying, and that we're not holding any structs around forever such that a ZeroizeOnDrop never triggers (because they never drop).
We may want to consider a crate with stronger guarantees here, like https://docs.rs/secrecy/0.10.3/secrecy/ (recommended in the zeroize documentation).
Address review feedback: setting gsp_length = 0 only clears the length field, leaving the gsp_buffer contents intact in the struct written back to VMGS. Add gsp_buffer.zeroize() for consistency with dek_buffer.
Add the zeroize crate to securely clear cryptographic keys and secrets from memory when they are dropped, preventing sensitive data from persisting after Rust objects are deallocated.
Changes: