From 5142fe9bf055631ad1ba234bf7de24fb54a60f5f Mon Sep 17 00:00:00 2001 From: Alisher Date: Tue, 11 Aug 2026 17:29:54 +0200 Subject: [PATCH] security: bind a fetched KeyPackage to the requested signer on the group add path (#202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On the group add path, a KeyPackage fetched from the registry for a requested signer id is validated but never checked to belong to that signer: the member id is read from the package's own credential. A registry that returns an attacker's (validly self-signed) package for a victim's id therefore inserts the attacker's leaf under the victim's identity — a confidentiality break and sender-attribution spoof. validate() is not meant to enforce this; the application (AS) layer is. Bind the validated leaf's signature_key (hex) to the requested signer id in both GroupV1::key_package_for_signer and GroupV2::add_member, rejecting a mismatch. Bind to the key, not the credential bytes: an impostor can copy an id into a credential but cannot sign a leaf with the victim's key. Courtesy patch from the downstream Peers fork; coordinated via the libchat security advisory. Reported by @x0net. Co-authored-by: Claude Opus 4.8 --- .../src/conversation/group_v1.rs | 13 ++++++++++++ .../src/conversation/group_v2.rs | 20 +++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/core/conversations/src/conversation/group_v1.rs b/core/conversations/src/conversation/group_v1.rs index c7b4cad..665a9f4 100644 --- a/core/conversations/src/conversation/group_v1.rs +++ b/core/conversations/src/conversation/group_v1.rs @@ -157,6 +157,19 @@ impl GroupV1Convo { let key_package_in = KeyPackageIn::tls_deserialize(&mut keypkg_bytes.as_slice())?; let keypkg = key_package_in.validate(provider.crypto(), ProtocolVersion::Mls10)?; //TODO: P3 - Hardcoded Protocol Version + // SECURITY: validate() only proves the package is well-formed and self-signed + // — NOT that it belongs to the signer we asked the registry for. Bind the + // fetched leaf's signature_key to the requested id (a signer id is + // hex(Ed25519 verifying key)); reject a mismatch so a malicious/compromised + // registry cannot insert an attacker's leaf under a victim's identity + // (confidentiality break + sender-attribution spoof). Bind to the key, not the + // spoofable credential bytes. + let leaf_key = hex::encode(keypkg.leaf_node().signature_key().as_slice()); + if leaf_key != signer.as_str() { + return Err(ChatError::Protocol(format!( + "keypackage for signer {signer} is bound to a different signing key ({leaf_key})" + ))); + } Ok(keypkg) } diff --git a/core/conversations/src/conversation/group_v2.rs b/core/conversations/src/conversation/group_v2.rs index f4b5d87..5a4172e 100644 --- a/core/conversations/src/conversation/group_v2.rs +++ b/core/conversations/src/conversation/group_v2.rs @@ -152,8 +152,24 @@ fn fetch_key_packages( .retrieve(member.as_str()) .map_err(ChatError::generic)? .ok_or_else(|| ChatError::generic("No key package"))?; - let member_id = KeyPackageIn::tls_deserialize(&mut key_package.as_slice())? - .validate(service_ctx.mls_provider.crypto(), ProtocolVersion::Mls10)? + let validated = KeyPackageIn::tls_deserialize(&mut key_package.as_slice())? + .validate(service_ctx.mls_provider.crypto(), ProtocolVersion::Mls10)?; + // SECURITY: a validated KeyPackage only proves it is well-formed and + // self-signed — NOT that it belongs to the signer we asked the registry + // for. `member_id` below is read from the package's OWN credential and was + // never checked equal to `member`, so a malicious/compromised registry (or + // a cache poisoned by an untrusted transport) can return an attacker's + // package for a victim's id, inserting the attacker's leaf under the + // victim's identity: confidentiality break + sender-attribution spoof. + // A signer id is hex(Ed25519 verifying key), so bind the leaf's + // signature_key (not the spoofable credential bytes) to the requested id. + let leaf_key = hex::encode(validated.leaf_node().signature_key().as_slice()); + if leaf_key != member.as_str() { + return Err(ChatError::generic(format!( + "key package for {member} is bound to a different signing key ({leaf_key})" + ))); + } + let member_id = validated .leaf_node() .credential() .serialized_content()