c/quicproquo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: security hardening — 40 findings from full codebase review

Browse Source 
Full codebase review by 4 independent agents (security, architecture,
code quality, correctness) identified ~80 findings. This commit fixes 40
of them across all workspace crates.

Critical fixes:
- Federation service: validate origin against mTLS cert CN/SAN (C1)
- WS bridge: add DM channel auth, size limits, rate limiting (C2)
- hpke_seal: panic on error instead of silent empty ciphertext (C3)
- hpke_setup_sender_and_export: error on parse fail, no PQ downgrade (C7)

Security fixes:
- Zeroize: seed_bytes() returns Zeroizing<[u8;32]>, private_to_bytes()
  returns Zeroizing<Vec<u8>>, ClientAuth.access_token, SessionState.password,
  conversation hex_key all wrapped in Zeroizing
- Keystore: 0o600 file permissions on Unix
- MeshIdentity: 0o600 file permissions on Unix
- Timing floors: resolveIdentity + WS bridge resolve_user get 5ms floor
- Mobile: TLS verification gated behind insecure-dev feature flag
- Proto: from_bytes default limit tightened from 64 MiB to 8 MiB

Correctness fixes:
- fetch_wait: register waiter before fetch to close TOCTOU window
- MeshEnvelope: exclude hop_count from signature (forwarding no longer
  invalidates sender signature)
- BroadcastChannel: encrypt returns Result instead of panicking
- transcript: rename verify_transcript_chain → validate_transcript_structure
- group.rs: extract shared process_incoming() for receive_message variants
- auth_ops: remove spurious RegistrationRequest deserialization
- MeshStore.seen: bounded to 100K with FIFO eviction

Quality fixes:
- FFI error classification: typed downcast instead of string matching
- Plugin HookVTable: SAFETY documentation for unsafe Send+Sync
- clippy::unwrap_used: warn → deny workspace-wide
- Various .unwrap_or("") → proper error returns

Review report: docs/REVIEW-2026-03-04.md
152 tests passing (72 core + 35 server + 14 E2E + 1 doctest + 30 P2P)
This commit is contained in:
Christian Nennemann
2026-03-04 07:52:12 +01:00
parent 4694a3098b
commit 394199b19b
58 changed files with 3893 additions and 414 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
crates/quicproquo-core/src/error.rs
View File

@@ -10,12 +10,20 @@ pub enum CoreError {
#[error("Cap'n Proto error: {0}")]
Capnp(#[from] capnp::Error),
/// An MLS operation failed.
/// An MLS operation failed (string description).
///
/// The inner string is the debug representation of the openmls error.
/// Preserved for backward compatibility. Prefer [`CoreError::MlsError`]
/// for new code that wraps typed openmls errors.
#[error("MLS error: {0}")]
Mls(String),
/// An MLS operation failed (typed, boxed error).
///
/// Wraps the underlying openmls error so callers can downcast to specific
/// error types when needed.
#[error("MLS error: {0}")]
MlsError(Box<dyn std::error::Error + Send + Sync>),
/// A hybrid KEM (X25519 + ML-KEM-768) operation failed.
#[error("hybrid KEM error: {0}")]
HybridKem(#[from] crate::hybrid_kem::HybridKemError),

110
crates/quicproquo-core/src/group.rs
View File

@@ -34,6 +34,8 @@
use std::{path::Path, sync::Arc};
use zeroize::Zeroizing;
use openmls::prelude::{
Ciphersuite, Credential, CredentialType, CredentialWithKey, CryptoConfig, GroupId, KeyPackage,
KeyPackageIn, MlsGroup, MlsGroupConfig, MlsMessageInBody, MlsMessageOut,
@@ -468,7 +470,36 @@ impl GroupMember {
///
/// Returns [`CoreError::Mls`] if the message is malformed, fails
/// authentication, or the group state is inconsistent.
pub fn receive_message(&mut self, mut bytes: &[u8]) -> Result<ReceivedMessage, CoreError> {
pub fn receive_message(&mut self, bytes: &[u8]) -> Result<ReceivedMessage, CoreError> {
let (sender, content) = self.process_incoming(bytes)?;
let _ = sender; // not needed for this variant
Ok(content)
}
/// Process an incoming TLS-encoded MLS message and return sender identity + plaintext for application messages.
///
/// Same as [`receive_message`], but for Application messages returns
/// `(sender_identity_bytes, plaintext)` so the client can display who sent the message.
pub fn receive_message_with_sender(
&mut self,
bytes: &[u8],
) -> Result<ReceivedMessageWithSender, CoreError> {
let (sender_identity, content) = self.process_incoming(bytes)?;
Ok(match content {
ReceivedMessage::Application(plaintext) => {
ReceivedMessageWithSender::Application(sender_identity, plaintext)
}
ReceivedMessage::StateChanged => ReceivedMessageWithSender::StateChanged,
ReceivedMessage::SelfRemoved => ReceivedMessageWithSender::SelfRemoved,
})
}
/// Shared MLS message processing: deserialize, authenticate, and apply
/// the incoming message. Returns `(sender_identity_bytes, result)`.
fn process_incoming(
&mut self,
mut bytes: &[u8],
) -> Result<(Vec<u8>, ReceivedMessage), CoreError> {
let group = self
.group
.as_mut()
@@ -488,9 +519,11 @@ impl GroupMember {
.process_message(&self.backend, protocol_message)
.map_err(|e| CoreError::Mls(format!("process_message: {e:?}")))?;
let sender_identity = processed.credential().identity().to_vec();
match processed.into_content() {
ProcessedMessageContent::ApplicationMessage(app) => {
Ok(ReceivedMessage::Application(app.into_bytes()))
Ok((sender_identity, ReceivedMessage::Application(app.into_bytes())))
}
ProcessedMessageContent::StagedCommitMessage(staged) => {
// Check if this commit removes us.
@@ -505,79 +538,19 @@ impl GroupMember {
if self_removed {
self.group = None;
Ok(ReceivedMessage::SelfRemoved)
Ok((sender_identity, ReceivedMessage::SelfRemoved))
} else {
Ok(ReceivedMessage::StateChanged)
Ok((sender_identity, ReceivedMessage::StateChanged))
}
}
// Proposals are stored for a later Commit; nothing to return yet.
ProcessedMessageContent::ProposalMessage(proposal) => {
group.store_pending_proposal(*proposal);
Ok(ReceivedMessage::StateChanged)
Ok((sender_identity, ReceivedMessage::StateChanged))
}
ProcessedMessageContent::ExternalJoinProposalMessage(proposal) => {
group.store_pending_proposal(*proposal);
Ok(ReceivedMessage::StateChanged)
}
}
}
/// Process an incoming TLS-encoded MLS message and return sender identity + plaintext for application messages.
///
/// Same as [`receive_message`], but for Application messages returns
/// `(sender_identity_bytes, plaintext)` so the client can display who sent the message.
pub fn receive_message_with_sender(
&mut self,
mut bytes: &[u8],
) -> Result<ReceivedMessageWithSender, CoreError> {
let group = self
.group
.as_mut()
.ok_or_else(|| CoreError::Mls("no active group".into()))?;
let msg_in = openmls::prelude::MlsMessageIn::tls_deserialize(&mut bytes)
.map_err(|e| CoreError::Mls(format!("message deserialise: {e:?}")))?;
let protocol_message = match msg_in.extract() {
MlsMessageInBody::PrivateMessage(m) => ProtocolMessage::PrivateMessage(m),
MlsMessageInBody::PublicMessage(m) => ProtocolMessage::PublicMessage(m),
_ => return Err(CoreError::Mls("not a protocol message".into())),
};
let processed = group
.process_message(&self.backend, protocol_message)
.map_err(|e| CoreError::Mls(format!("process_message: {e:?}")))?;
let sender_identity = processed.credential().identity().to_vec();
match processed.into_content() {
ProcessedMessageContent::ApplicationMessage(app) => {
Ok(ReceivedMessageWithSender::Application(sender_identity, app.into_bytes()))
}
ProcessedMessageContent::StagedCommitMessage(staged) => {
let own_index = group.own_leaf_index();
let self_removed = staged.remove_proposals().any(|queued| {
queued.remove_proposal().removed() == own_index
});
group
.merge_staged_commit(&self.backend, *staged)
.map_err(|e| CoreError::Mls(format!("merge_staged_commit: {e:?}")))?;
if self_removed {
self.group = None;
Ok(ReceivedMessageWithSender::SelfRemoved)
} else {
Ok(ReceivedMessageWithSender::StateChanged)
}
}
ProcessedMessageContent::ProposalMessage(proposal) => {
group.store_pending_proposal(*proposal);
Ok(ReceivedMessageWithSender::StateChanged)
}
ProcessedMessageContent::ExternalJoinProposalMessage(proposal) => {
group.store_pending_proposal(*proposal);
Ok(ReceivedMessageWithSender::StateChanged)
Ok((sender_identity, ReceivedMessage::StateChanged))
}
}
}
@@ -597,7 +570,10 @@ impl GroupMember {
}
/// Return the private seed of the identity (for persistence).
pub fn identity_seed(&self) -> [u8; 32] {
///
/// The returned value is wrapped in `Zeroizing` so it is securely erased
/// when dropped.
pub fn identity_seed(&self) -> Zeroizing<[u8; 32]> {
self.identity.seed_bytes()
}

56
crates/quicproquo-core/src/hybrid_crypto.rs
View File

@@ -191,32 +191,22 @@ impl OpenMlsCrypto for HybridCrypto {
ptxt: &[u8],
) -> HpkeCiphertext {
if Self::is_hybrid_public_key(pk_r) {
let recipient_pk = match HybridPublicKey::from_bytes(pk_r) {
Ok(pk) => pk,
// Key parsed as hybrid length but failed to deserialize — this is
// a real error, not a reason to silently fall back to classical HPKE.
Err(_) => return HpkeCiphertext {
kem_output: Vec::new().into(),
ciphertext: Vec::new().into(),
},
};
// The trait `OpenMlsCrypto::hpke_seal` returns `HpkeCiphertext` (not
// `Result`), so we cannot propagate errors through the return type.
// Returning an empty ciphertext would silently cause data loss.
// Instead, panic on failure — a hybrid key that passes the length
// check but fails deserialization or encryption indicates a critical
// bug (corrupted key material), not a recoverable condition.
let recipient_pk = HybridPublicKey::from_bytes(pk_r)
.expect("hybrid public key deserialization failed — key material is corrupted");
// Pass HPKE info and aad through for proper context binding (RFC 9180).
match hybrid_encrypt(&recipient_pk, ptxt, info, aad) {
Ok(envelope) => {
let kem_output = envelope[..HYBRID_KEM_OUTPUT_LEN].to_vec();
let ciphertext = envelope[HYBRID_KEM_OUTPUT_LEN..].to_vec();
HpkeCiphertext {
kem_output: kem_output.into(),
ciphertext: ciphertext.into(),
}
}
// Encryption failed with a hybrid key — return empty ciphertext
// rather than silently falling back to classical HPKE with an
// incompatible key.
Err(_) => HpkeCiphertext {
kem_output: Vec::new().into(),
ciphertext: Vec::new().into(),
},
let envelope = hybrid_encrypt(&recipient_pk, ptxt, info, aad)
.expect("hybrid HPKE encryption failed — critical crypto error");
let kem_output = envelope[..HYBRID_KEM_OUTPUT_LEN].to_vec();
let ciphertext = envelope[HYBRID_KEM_OUTPUT_LEN..].to_vec();
HpkeCiphertext {
kem_output: kem_output.into(),
ciphertext: ciphertext.into(),
}
} else {
self.rust_crypto.hpke_seal(config, pk_r, info, aad, ptxt)
@@ -257,14 +247,11 @@ impl OpenMlsCrypto for HybridCrypto {
exporter_length: usize,
) -> Result<(Vec<u8>, ExporterSecret), CryptoError> {
if Self::is_hybrid_public_key(pk_r) {
let recipient_pk = match HybridPublicKey::from_bytes(pk_r) {
Ok(pk) => pk,
Err(_) => {
return self.rust_crypto.hpke_setup_sender_and_export(
config, pk_r, info, exporter_context, exporter_length,
)
}
};
// A key that passes the hybrid length check but fails deserialization
// is corrupted — return an error instead of silently downgrading to
// classical crypto (which would defeat PQ protection).
let recipient_pk = HybridPublicKey::from_bytes(pk_r)
.map_err(|_| CryptoError::SenderSetupError)?;
let (kem_output, shared_secret) =
hybrid_encapsulate_only(&recipient_pk).map_err(|_| CryptoError::SenderSetupError)?;
let exported = hybrid_export(&shared_secret, exporter_context, exporter_length);
@@ -302,8 +289,9 @@ impl OpenMlsCrypto for HybridCrypto {
fn derive_hpke_keypair(&self, config: HpkeConfig, ikm: &[u8]) -> HpkeKeyPair {
if self.hybrid_enabled && config.0 == HpkeKemType::DhKem25519 {
let kp = HybridKeypair::derive_from_ikm(ikm);
let private_bytes = kp.private_to_bytes();
HpkeKeyPair {
private: kp.private_to_bytes().into(),
private: private_bytes.as_slice().into(),
public: kp.public_key().to_bytes(),
}
} else {

7
crates/quicproquo-core/src/hybrid_kem.rs
View File

@@ -159,11 +159,14 @@ impl HybridKeypair {
}
/// Serialise private key for MLS key store: x25519_sk(32) || mlkem_dk(2400).
pub fn private_to_bytes(&self) -> Vec<u8> {
///
/// The returned value is wrapped in [`Zeroizing`] so secret key material
/// is securely erased when dropped.
pub fn private_to_bytes(&self) -> Zeroizing<Vec<u8>> {
let mut out = Vec::with_capacity(HYBRID_PRIVATE_KEY_LEN);
out.extend_from_slice(self.x25519_sk.as_bytes());
out.extend_from_slice(self.mlkem_dk.as_bytes().as_slice());
out
Zeroizing::new(out)
}
/// Reconstruct a hybrid keypair from private key bytes (from MLS key store).

7
crates/quicproquo-core/src/identity.rs
View File

@@ -47,8 +47,11 @@ impl IdentityKeypair {
}
/// Return the raw 32-byte private seed (for persistence).
pub fn seed_bytes(&self) -> [u8; 32] {
*self.seed
///
/// The returned value is wrapped in [`Zeroizing`] so it is securely
/// erased when dropped, preventing the seed from lingering in memory.
pub fn seed_bytes(&self) -> Zeroizing<[u8; 32]> {
Zeroizing::new(*self.seed)
}
}

52
crates/quicproquo-core/src/keystore.rs
View File

@@ -11,6 +11,18 @@ use openmls_traits::key_store::{MlsEntity, OpenMlsKeyStore};
///
/// In-memory when `path` is `None`; otherwise flushes the entire map to disk on
/// every store/delete so HPKE init keys survive process restarts.
///
/// # Serialization
///
/// Uses bincode for both individual MLS entity values and the outer HashMap
/// container. This is required because OpenMLS types use bincode-compatible
/// serialization, and `HashMap<Vec<u8>, Vec<u8>>` requires a binary format
/// (JSON mandates string keys).
///
/// # Persistence security
///
/// When `path` is set, file permissions are restricted to owner-only (0o600)
/// on Unix platforms, since the store may contain HPKE private keys.
#[derive(Debug)]
pub struct DiskKeyStore {
path: Option<PathBuf>,
@@ -42,16 +54,22 @@ impl DiskKeyStore {
if bytes.is_empty() {
HashMap::new()
} else {
bincode::deserialize(&bytes).map_err(|_| DiskKeyStoreError::Serialization)?
bincode::deserialize(&bytes)
.map_err(|_| DiskKeyStoreError::Serialization)?
}
} else {
HashMap::new()
};
Ok(Self {
let store = Self {
path: Some(path),
values: RwLock::new(values),
})
};
// Set restrictive file permissions on the keystore file.
store.set_file_permissions()?;
Ok(store)
}
fn flush(&self) -> Result<(), DiskKeyStoreError> {
@@ -63,7 +81,28 @@ impl DiskKeyStore {
if let Some(parent) = path.parent() {
fs::create_dir_all(parent).map_err(|e| DiskKeyStoreError::Io(e.to_string()))?;
}
fs::write(path, bytes).map_err(|e| DiskKeyStoreError::Io(e.to_string()))
fs::write(path, &bytes).map_err(|e| DiskKeyStoreError::Io(e.to_string()))?;
self.set_file_permissions()?;
Ok(())
}
/// Restrict file permissions to owner-only (0o600) on Unix.
#[cfg(unix)]
fn set_file_permissions(&self) -> Result<(), DiskKeyStoreError> {
use std::os::unix::fs::PermissionsExt;
if let Some(path) = &self.path {
if path.exists() {
let perms = std::fs::Permissions::from_mode(0o600);
fs::set_permissions(path, perms)
.map_err(|e| DiskKeyStoreError::Io(format!("set permissions: {e}")))?;
}
}
Ok(())
}
#[cfg(not(unix))]
fn set_file_permissions(&self) -> Result<(), DiskKeyStoreError> {
Ok(())
}
}
@@ -77,7 +116,7 @@ impl OpenMlsKeyStore for DiskKeyStore {
type Error = DiskKeyStoreError;
fn store<V: MlsEntity>(&self, k: &[u8], v: &V) -> Result<(), Self::Error> {
let value = serde_json::to_vec(v).map_err(|_| DiskKeyStoreError::Serialization)?;
let value = bincode::serialize(v).map_err(|_| DiskKeyStoreError::Serialization)?;
let mut values = self.values.write().map_err(|_| DiskKeyStoreError::Io("lock poisoned".into()))?;
values.insert(k.to_vec(), value);
drop(values);
@@ -91,7 +130,7 @@ impl OpenMlsKeyStore for DiskKeyStore {
};
values
.get(k)
.and_then(|bytes| serde_json::from_slice(bytes).ok())
.and_then(|bytes| bincode::deserialize(bytes).ok())
}
fn delete<V: MlsEntity>(&self, k: &[u8]) -> Result<(), Self::Error> {
@@ -101,4 +140,3 @@ impl OpenMlsKeyStore for DiskKeyStore {
self.flush()
}
}

5
crates/quicproquo-core/src/lib.rs
View File

@@ -72,9 +72,12 @@ pub use hybrid_kem::{
pub use identity::{verify_delivery_proof, IdentityKeypair};
pub use safety_numbers::compute_safety_number;
pub use transcript::{
read_transcript, verify_transcript_chain, ChainVerdict, DecodedRecord, TranscriptRecord,
read_transcript, validate_transcript_structure, ChainVerdict, DecodedRecord, TranscriptRecord,
TranscriptWriter,
};
// Deprecated re-export for backward compatibility.
#[allow(deprecated)]
pub use transcript::verify_transcript_chain;
// ── Public API (native only) ────────────────────────────────────────────────

22
crates/quicproquo-core/src/transcript.rs
View File

@@ -208,11 +208,17 @@ pub fn read_transcript(
Ok((records, verdict))
}
/// Verify the hash chain without decrypting record contents.
/// Validate the structural integrity of a transcript file without decrypting.
///
/// Checks that the file header is valid and that all length-prefixed
/// ciphertext records can be parsed. Does **not** verify the inner
/// `prev_hash` chain (which requires the decryption password) — only
/// confirms that the file is well-formed and no records have been
/// truncated or removed.
///
/// Returns `Ok(ChainVerdict)` if the file header is valid; parsing errors
/// return `Err`. The chain verdict indicates whether all hashes matched.
pub fn verify_transcript_chain(data: &[u8]) -> Result<ChainVerdict, CoreError> {
/// return `Err`.
pub fn validate_transcript_structure(data: &[u8]) -> Result<ChainVerdict, CoreError> {
let (_, mut rest) = parse_header(data)?;
let mut expected_prev: [u8; 32] = [0u8; 32];
@@ -250,6 +256,12 @@ pub fn verify_transcript_chain(data: &[u8]) -> Result<ChainVerdict, CoreError> {
Ok(ChainVerdict::Ok { records: count })
}
/// Deprecated alias for [`validate_transcript_structure`].
#[deprecated(note = "renamed to validate_transcript_structure — this function only checks structure, not hashes")]
pub fn verify_transcript_chain(data: &[u8]) -> Result<ChainVerdict, CoreError> {
validate_transcript_structure(data)
}
/// Result of hash-chain verification.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ChainVerdict {
@@ -515,7 +527,7 @@ mod tests {
.expect("write");
}
let verdict = verify_transcript_chain(&buf).expect("verify");
let verdict = validate_transcript_structure(&buf).expect("verify");
assert_eq!(verdict, ChainVerdict::Ok { records: 5 });
}
@@ -537,7 +549,7 @@ mod tests {
// Truncate the last few bytes — should fail parsing.
let truncated = &buf[..buf.len() - 5];
let result = verify_transcript_chain(truncated);
let result = validate_transcript_structure(truncated);
assert!(result.is_err(), "truncated file must be detected");
}
}