`mls_rs_core::group::GroupState` and `EpochRecord` `data` fields are not zeroized on drop

[SimonThormeyer]

Problem:

There is an inconsistency how data is zeroized; In

mls-rs/mls-rs/src/group/state_repo.rs

Line 195 in d9e5f8b

pub async fn write_to_storage(&mut self, group_snapshot: Snapshot) -> Result<(), MlsError> {

the argument is a

mls-rs/mls-rs/src/group/snapshot.rs

Lines 40 to 50 in d9e5f8b

	pub(crate) struct Snapshot {
	version: u16,
	pub(crate) state: RawGroupState,
	private_tree: TreeKemPrivate,
	epoch_secrets: EpochSecrets,
	key_schedule: KeySchedule,
	#[cfg(feature = "by_ref_proposal")]
	pending_updates: SmallMap<HpkePublicKey, (HpkeSecretKey, Option<SignatureSecretKey>)>,
	pending_commit_snapshot: PendingCommitSnapshot,
	signer: SignatureSecretKey,
	}

which contains secret data, on which you derive ZeroizeOnDrop.

From that Snapshot, GroupState is constructed:

mls-rs/mls-rs/src/group/state_repo.rs

Lines 210 to 213 in d9e5f8b

	let group_state = GroupState {
	data: group_snapshot.mls_encode_to_vec()?,
	id: group_snapshot.state.context.group_id,
	};

which contains a transformation of that Snapshot in its data field.
Consequently, I would expect that data field to be zeroized on drop also.

Solution:

A)

Keep the Snapshot type for the data field on GroupState, and let the storage implementation do the transformation to bytes and handle its zeroization: similar to how it is done for

mls-rs/mls-rs-core/src/key_package.rs

Lines 17 to 24 in d9e5f8b

	pub struct KeyPackageData {
	#[cfg_attr(feature = "serde", serde(with = "crate::vec_serde"))]
	pub key_package_bytes: Vec<u8>,
	pub init_key: HpkeSecretKey,
	pub leaf_node_key: HpkeSecretKey,
	/// Seconds since the Unix epoch starting Jan 1st 1970.
	pub expiration: u64,
	}

B)

Alternatively, wrap the data field of

mls-rs/mls-rs-core/src/group/group_state.rs

Lines 14 to 18 in d9e5f8b

	pub struct GroupState {
	/// A unique group identifier.
	pub id: Vec<u8>,
	pub data: Vec<u8>,
	}

in zeroize::Zeroizing<_>>, so that it becomes

pub struct GroupState {
    /// A unique group identifier.
    pub id: Vec<u8>,
    pub data: Zeroizing<Vec<u8>>,
}

---

This applies analogously to

mls-rs/mls-rs-core/src/group/group_state.rs

Lines 31 to 35 in d9e5f8b

	pub struct EpochRecord {
	/// A unique epoch identifier within a particular group.
	pub id: u64,
	pub data: Vec<u8>,
	}

that is constructed in write_to_storage() from

mls-rs/mls-rs/src/group/epoch.rs

Lines 28 to 35 in d9e5f8b

	pub(crate) struct PriorEpoch {
	pub(crate) context: GroupContext,
	pub(crate) self_index: LeafIndex,
	pub(crate) secrets: EpochSecrets,
	pub(crate) signature_public_keys: Vec<Option<SignaturePublicKey>>,
	#[cfg(feature = "prior_epoch_membership_key")]
	pub(crate) membership_key: Vec<u8>,
	}

[tomleavy]

Thanks for reporting @SimonThormeyer . Indeed this should be corrected. Will take a look