Skip to content

Use zeroize for secure secret cleanup #3411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mebersol wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use zeroize for secure secret cleanup #3411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a6af09
[Copilot] Use zeroize for secure secret cleanup
mebersol Apr 30, 2026
c994b17
vmgs: make zeroize dep conditional on encryption feature
mebersol May 1, 2026
79ad325
underhill_attestation: also zeroize gsp_buffer before persisting
mebersol May 1, 2026
03a8659
Merge branch 'main' into user/mebersol/zeroize-secrets
mebersol May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5060,6 +5060,7 @@ dependencies = [
"serde_json",
"x86defs",
"zerocopy",
"zeroize",
]

[[package]]
Expand Down Expand Up @@ -8285,6 +8286,7 @@ dependencies = [
"vmgs",
"vmgs_format",
"zerocopy",
"zeroize",
]

[[package]]
Expand Down Expand Up @@ -9850,6 +9852,7 @@ dependencies = [
"tracing",
"vmgs_format",
"zerocopy",
"zeroize",
]

[[package]]
Expand Down Expand Up @@ -11048,3 +11051,17 @@ name = "zeroize"
version = "1.8.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ced3678a2879b30306d323f4542626697a464a97c0a07c9aebf7ebca65cd4dde"
dependencies = [
"zeroize_derive",
]

[[package]]
name = "zeroize_derive"
version = "1.4.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "85a5b4158499876c763cb03bc4e49185d3cccbabb15b33c627f7884f43db852e"
dependencies = [
"proc-macro2",
"quote",
"syn 2.0.106",
]
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,7 @@ xshell = "=0.2.2" # pin to 0.2.2 to work around https://github.com/matklad/xshel
xshell-macros = "0.2"
# We add the derive feature here since the vast majority of our crates use it.
zerocopy = { version = "0.8.25", features = ["derive"]}
zeroize = { version = "1.8", features = ["derive"]}

[profile.release]
panic = 'abort'
Expand Down
1 change: 1 addition & 0 deletions openhcl/openhcl_attestation_protocol/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ hex.workspace = true
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true, features = ["std"] }
zerocopy.workspace = true
zeroize.workspace = true

[lints]
workspace = true
13 changes: 8 additions & 5 deletions openhcl/openhcl_attestation_protocol/src/vmgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use zerocopy::FromBytes;
use zerocopy::Immutable;
use zerocopy::IntoBytes;
use zerocopy::KnownLayout;
use zeroize::Zeroize;
use zeroize::ZeroizeOnDrop;

/// Number of the key protector entries.
/// One for ingress, and one for egress
Expand All @@ -23,15 +25,15 @@ pub const KEY_PROTECTOR_SIZE: usize = size_of::<KeyProtector>();

/// DEK key protector entry.
#[repr(C)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes, Zeroize, ZeroizeOnDrop)]
pub struct DekKp {
/// DEK buffer
pub dek_buffer: [u8; DEK_BUFFER_SIZE],
}

/// GSP key protector entry.
#[repr(C)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes, Zeroize, ZeroizeOnDrop)]
pub struct GspKp {
/// GSP data size
pub gsp_length: u32,
Expand All @@ -41,7 +43,7 @@ pub struct GspKp {

/// The data format of the `FileId::KEY_PROTECTOR` entry in the VMGS file.
#[repr(C)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes, Zeroize, ZeroizeOnDrop)]
pub struct KeyProtector {
/// Array of DEK entries
pub dek: [DekKp; NUMBER_KP],
Expand Down Expand Up @@ -123,9 +125,10 @@ impl HardwareKeyProtectorHeader {

/// The data format of the `FileId::HW_KEY_PROTECTOR` entry in the VMGS file.
#[repr(C)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes, Zeroize, ZeroizeOnDrop)]
pub struct HardwareKeyProtector {
/// Header
#[zeroize(skip)]
pub header: HardwareKeyProtectorHeader,
/// Random IV for AES-CBC
pub iv: [u8; AES_CBC_IV_LENGTH],
Expand All @@ -140,7 +143,7 @@ pub const GUEST_SECRET_KEY_MAX_SIZE: usize = 2048;

/// The data format of the `FileId::GUEST_SECRET_KEY` entry in the VMGS file.
#[repr(C)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes)]
#[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes, Zeroize, ZeroizeOnDrop)]
pub struct GuestSecretKey {
/// the guest secret key to be provisioned to vTPM
pub guest_secret_key: [u8; GUEST_SECRET_KEY_MAX_SIZE],
Expand Down
1 change: 1 addition & 0 deletions openhcl/underhill_attestation/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ serde_json = { workspace = true, features = ["std"] }
static_assertions.workspace = true
thiserror.workspace = true
zerocopy.workspace = true
zeroize.workspace = true

[target.'cfg(target_os = "linux")'.dev-dependencies]
disklayer_ram.workspace = true
Expand Down
3 changes: 3 additions & 0 deletions openhcl/underhill_attestation/src/hardware_key_sealing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ use openhcl_attestation_protocol::vmgs;
use openhcl_attestation_protocol::vmgs::HardwareKeyProtector;
use thiserror::Error;
use zerocopy::IntoBytes;
use zeroize::Zeroize;
use zeroize::ZeroizeOnDrop;

#[derive(Debug, Error)]
pub(crate) enum HardwareDerivedKeysError {
Expand Down Expand Up @@ -39,6 +41,7 @@ pub(crate) enum HardwareKeySealingError {
}

/// Hold the hardware-derived keys.
#[derive(Zeroize, ZeroizeOnDrop)]
pub struct HardwareDerivedKeys {
tcb_version: u64,
aes_key: [u8; vmgs::AES_CBC_KEY_LENGTH],
Expand Down
8 changes: 6 additions & 2 deletions openhcl/underhill_attestation/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ use tee_call::TeeCall;
use thiserror::Error;
use zerocopy::FromZeros;
use zerocopy::IntoBytes;
use zeroize::Zeroize;

/// An attestation error.
#[derive(Debug, Error)]
Expand Down Expand Up @@ -180,7 +181,7 @@ fn derive_key(
Ok(output.try_into().unwrap())
}

#[derive(Debug)]
#[derive(Debug, zeroize::Zeroize, zeroize::ZeroizeOnDrop)]
struct Keys {
ingress: [u8; AES_GCM_KEY_LENGTH],
decrypt_egress: Option<[u8; AES_GCM_KEY_LENGTH]>,
Expand Down Expand Up @@ -1337,8 +1338,11 @@ async fn persist_all_key_protectors(
// Remove ingress KP & DEK, no longer applies to data store
key_protector.dek[key_protector.active_kp as usize % NUMBER_KP]
.dek_buffer
.fill(0);
.zeroize();
Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft May 1, 2026 •
edited by mebersol
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to add ZeroizeOnDrop to all DekKps? #Resolved

Copy link
Copy Markdown
Collaborator Author

@mebersol mebersol May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

key_protector.gsp[key_protector.active_kp as usize % NUMBER_KP].gsp_length = 0;
Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft May 1, 2026 •
edited by mebersol
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do something more to clear the gsp than just setting length to 0? #Resolved

Copy link
Copy Markdown
Collaborator Author

@mebersol mebersol May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

key_protector.gsp[key_protector.active_kp as usize % NUMBER_KP]
.gsp_buffer
.zeroize();
key_protector.active_kp += 1;

vmgs::write_key_protector(key_protector, vmgs)
Expand Down
3 changes: 2 additions & 1 deletion vm/vmgs/vmgs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ default = []
inspect = ["vmgs_format/inspect", "dep:inspect", "dep:inspect_counters"]
save_restore = ["dep:mesh_protobuf"]
mesh = ["dep:mesh_protobuf"]
encryption = ["dep:crypto"]
encryption = ["dep:crypto", "dep:zeroize"]
test_helpers = []

# run encryption unit tests in CI
Expand All @@ -36,6 +36,7 @@ thiserror.workspace = true
tracing.workspace = true
cvm_tracing.workspace = true
zerocopy.workspace = true
zeroize = { workspace = true, optional = true }

[dev-dependencies]
pal_async.workspace = true
Expand Down
4 changes: 3 additions & 1 deletion vm/vmgs/vmgs/src/vmgs_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ use vmgs_format::VmgsProvisioningReason;
use zerocopy::FromBytes;
use zerocopy::FromZeros;
use zerocopy::IntoBytes;
#[cfg(feature = "encryption")]
use zeroize::Zeroize;

/// Operation types for provisioning telemetry.
#[derive(Debug)]
Expand Down Expand Up @@ -1235,7 +1237,7 @@ impl Vmgs {
let mut temp_state = self.temp_state();

// Remove the corresponding datastore_key
temp_state.datastore_keys[key_index].fill(0);
temp_state.datastore_keys[key_index].zeroize();
Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to add ZeroizeOnDrop to all VmgsDatastoreKeys?

Copy link
Copy Markdown
Collaborator Author

@mebersol mebersol May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().


// Remove the corresponding metadata_key
temp_state.encrypted_metadata_keys[key_index] = VmgsEncryptionKey::new_zeroed();
Expand Down
Loading