From 2bb62183521858c926ba3e40c8ea05e0ed443a24 Mon Sep 17 00:00:00 2001 From: beltram Date: Fri, 22 Dec 2023 11:36:23 +0100 Subject: [PATCH] fix: verify GroupInfo --- .../array_representation/treemath.rs | 2 +- openmls/src/credentials/mod.rs | 1 + .../core_group/new_from_external_init.rs | 3 +- .../src/group/core_group/new_from_welcome.rs | 3 +- openmls/src/group/public_group/mod.rs | 5 +- openmls/src/group/public_group/tests.rs | 1 + openmls/src/messages/group_info.rs | 6 +- openmls/src/treesync/errors.rs | 8 ++- openmls/src/treesync/mod.rs | 34 ++++++--- openmls/src/treesync/node/leaf_node.rs | 25 ++++++- openmls/src/treesync/node/validate.rs | 71 +++++++++++-------- .../kats/kat_tree_operations.rs | 6 +- .../kats/kat_tree_validation.rs | 13 ++-- .../tests_and_kats/kats/kat_treekem.rs | 12 ++-- .../tests_and_kats/tests/test_diff.rs | 6 +- 15 files changed, 132 insertions(+), 64 deletions(-) diff --git a/openmls/src/binary_tree/array_representation/treemath.rs b/openmls/src/binary_tree/array_representation/treemath.rs index e8cf4837fc..d73e019535 100644 --- a/openmls/src/binary_tree/array_representation/treemath.rs +++ b/openmls/src/binary_tree/array_representation/treemath.rs @@ -58,7 +58,7 @@ impl LeafNodeIndex { } /// Warning: Only use when the node index represents a leaf node - fn from_tree_index(node_index: u32) -> Self { + pub fn from_tree_index(node_index: u32) -> Self { debug_assert!(node_index % 2 == 0); LeafNodeIndex(node_index / 2) } diff --git a/openmls/src/credentials/mod.rs b/openmls/src/credentials/mod.rs index e5abf5f441..c7aa05774a 100644 --- a/openmls/src/credentials/mod.rs +++ b/openmls/src/credentials/mod.rs @@ -37,6 +37,7 @@ mod codec; mod tests; use errors::*; + use openmls_x509_credential::X509Ext; use x509_cert::{der::Decode, PkiPath}; diff --git a/openmls/src/group/core_group/new_from_external_init.rs b/openmls/src/group/core_group/new_from_external_init.rs index 7fec0ca333..069cb70439 100644 --- a/openmls/src/group/core_group/new_from_external_init.rs +++ b/openmls/src/group/core_group/new_from_external_init.rs @@ -52,7 +52,8 @@ impl CoreGroup { verifiable_group_info, // Existing proposals are discarded when joining by external commit. ProposalStore::new(), - )?; + ) + .await?; let group_context = public_group.group_context(); // Obtain external_pub from GroupInfo extensions. diff --git a/openmls/src/group/core_group/new_from_welcome.rs b/openmls/src/group/core_group/new_from_welcome.rs index 7ce8cbc0be..173cc56284 100644 --- a/openmls/src/group/core_group/new_from_welcome.rs +++ b/openmls/src/group/core_group/new_from_welcome.rs @@ -151,7 +151,8 @@ impl CoreGroup { ratchet_tree, verifiable_group_info, ProposalStore::new(), - )?; + ) + .await?; KeyPackageIn::from(key_package.clone()) .validate(backend, ProtocolVersion::Mls10, &public_group) diff --git a/openmls/src/group/public_group/mod.rs b/openmls/src/group/public_group/mod.rs index ad787fcfe3..ca6c437f54 100644 --- a/openmls/src/group/public_group/mod.rs +++ b/openmls/src/group/public_group/mod.rs @@ -102,7 +102,7 @@ impl PublicGroup { /// This function performs basic validation checks and returns an error if /// one of the checks fails. See [`CreationFromExternalError`] for more /// details. - pub fn from_external( + pub async fn from_external( backend: &impl OpenMlsCryptoProvider, ratchet_tree: RatchetTreeIn, verifiable_group_info: VerifiableGroupInfo, @@ -122,7 +122,8 @@ impl PublicGroup { // Create a RatchetTree from the given nodes. We have to do this before // verifying the group info, since we need to find the Credential to verify the // signature against. - let treesync = TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree)?; + let treesync = + TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree, group_id, true).await?; let group_info: GroupInfo = { let signer_signature_key = treesync diff --git a/openmls/src/group/public_group/tests.rs b/openmls/src/group/public_group/tests.rs index cc3e451bb7..bd476638ed 100644 --- a/openmls/src/group/public_group/tests.rs +++ b/openmls/src/group/public_group/tests.rs @@ -61,6 +61,7 @@ async fn public_group(ciphersuite: Ciphersuite, backend: &impl OpenMlsCryptoProv verifiable_group_info, ProposalStore::new(), ) + .await .unwrap(); // === Alice adds Bob === diff --git a/openmls/src/messages/group_info.rs b/openmls/src/messages/group_info.rs index 44c820ddf0..f3e002cb3f 100644 --- a/openmls/src/messages/group_info.rs +++ b/openmls/src/messages/group_info.rs @@ -120,7 +120,7 @@ impl VerifiableGroupInfo { } /// Do whatever it takes not to clone the RatchetTree - pub fn take_ratchet_tree( + pub async fn take_ratchet_tree( mut self, backend: &impl OpenMlsCryptoProvider, ) -> Result { @@ -142,7 +142,9 @@ impl VerifiableGroupInfo { .into_verified(cs, backend.crypto(), self.group_id())?; // although it clones the ratchet tree here... - let treesync = TreeSync::from_ratchet_tree(backend, cs, ratchet_tree.clone())?; + let group_id = self.group_id(); + let treesync = + TreeSync::from_ratchet_tree(backend, cs, ratchet_tree.clone(), group_id, true).await?; let signer_signature_key = treesync .leaf(self.signer()) diff --git a/openmls/src/treesync/errors.rs b/openmls/src/treesync/errors.rs index 8884849726..e8d1140647 100644 --- a/openmls/src/treesync/errors.rs +++ b/openmls/src/treesync/errors.rs @@ -152,6 +152,9 @@ pub enum TreeSyncFromNodesError { /// See [`RatchetTreeError`] for more details. #[error(transparent)] RatchetTreeError(#[from] RatchetTreeError), + /// See [`LeafNodeValidationError`] for more details. + #[error(transparent)] + LeafNodeValidationError(#[from] LeafNodeValidationError), } /// TreeSync parent hash error @@ -262,14 +265,15 @@ pub enum LeafNodeValidationError { /// The credential used by a member is not supported by this leaf node. #[error("The credential used by a member is not supported by this leaf node.")] MemberCredentialNotSupportedByLeafNode, - #[error(transparent)] - InvalidCredential(#[from] CredentialError), /// A library error occurred. #[error(transparent)] LibraryError(#[from] LibraryError), /// See [`SignatureError`] for more details. #[error(transparent)] SignatureError(#[from] SignatureError), + /// See [`CredentialError`] for more details. + #[error(transparent)] + CredentialError(#[from] CredentialError), } /// Errors that can happen during lifetime validation. diff --git a/openmls/src/treesync/mod.rs b/openmls/src/treesync/mod.rs index 285a32e34a..e4229b4330 100644 --- a/openmls/src/treesync/mod.rs +++ b/openmls/src/treesync/mod.rs @@ -83,6 +83,7 @@ pub use node::encryption_keys::test_utils; pub use node::encryption_keys::EncryptionKey; // Public re-exports +use crate::treesync::node::leaf_node::LeafNodeIn; pub use node::{leaf_node::LeafNode, parent_node::ParentNode, Node}; // Tests @@ -172,13 +173,13 @@ impl RatchetTree { // Verify the nodes. let signature_scheme = ciphersuite.signature_algorithm(); let mut verified_nodes = Vec::new(); - for (index, node) in nodes.into_iter().enumerate() { - let verified_node = match (index % 2, node) { + for (node_index, node) in nodes.into_iter().enumerate() { + let verified_node = match (node_index % 2, node) { // Even indices must be leaf nodes. (0, Some(NodeIn::LeafNode(leaf_node))) => { let tree_position = TreePosition::new( group_id.clone(), - LeafNodeIndex::new((index / 2) as u32), + LeafNodeIndex::from_tree_index(node_index as u32), ); let verifiable_leaf_node = leaf_node.try_into_verifiable_leaf_node(Some(tree_position))?; @@ -439,10 +440,12 @@ impl TreeSync { /// A helper function that generates a [`TreeSync`] instance from the given /// slice of nodes. It verifies that the provided encryption key is present /// in the tree and that the invariants documented in [`TreeSync`] hold. - pub(crate) fn from_ratchet_tree( + pub(crate) async fn from_ratchet_tree( backend: &impl OpenMlsCryptoProvider, ciphersuite: Ciphersuite, ratchet_tree: RatchetTree, + group_id: &GroupId, + validate_leaf_node: bool, ) -> Result { // TODO #800: Unmerged leaves should be checked let mut ts_nodes: Vec> = @@ -450,11 +453,24 @@ impl TreeSync { // Set the leaf indices in all the leaves and convert the node types. for (node_index, node_option) in ratchet_tree.0.into_iter().enumerate() { - let ts_node_option: TreeNode = match node_option { - Some(node) => { - let node = node.clone(); - TreeSyncNode::from(node).into() + let ts_node: TreeNode = match node_option { + Some(Node::LeafNode(ln)) => { + let ln = if validate_leaf_node { + let index = node_index + .try_into() + .map_err(|_| LibraryError::custom("failed converting a usize -> u32")); + let index = LeafNodeIndex::from_tree_index(index?); + let tree_position = TreePosition::new(group_id.clone(), index); + let ln = LeafNodeIn::from(ln) + .try_into_verifiable_leaf_node(Some(tree_position))?; + ln.validate(backend, ciphersuite.signature_algorithm(), None) + .await? + } else { + ln + }; + TreeSyncNode::from(Node::LeafNode(ln)).into() } + Some(Node::ParentNode(pn)) => TreeSyncNode::from(Node::ParentNode(pn)).into(), None => { if node_index % 2 == 0 { TreeNode::Leaf(TreeSyncLeafNode::blank()) @@ -463,7 +479,7 @@ impl TreeSync { } } }; - ts_nodes.push(ts_node_option); + ts_nodes.push(ts_node); } let tree = MlsBinaryTree::new(ts_nodes).map_err(|_| PublicTreeError::MalformedTree)?; let mut tree_sync = Self { diff --git a/openmls/src/treesync/node/leaf_node.rs b/openmls/src/treesync/node/leaf_node.rs index c66b0b0fb0..db23a03141 100644 --- a/openmls/src/treesync/node/leaf_node.rs +++ b/openmls/src/treesync/node/leaf_node.rs @@ -29,7 +29,10 @@ use crate::treesync::errors::LeafNodeValidationError; mod capabilities; mod codec; +use crate::prelude::PublicGroup; +use crate::treesync::node::validate::ValidatableLeafNode; pub use capabilities::*; +use openmls_traits::types::SignatureScheme; /// Private module to ensure protection. mod private_mod { @@ -706,7 +709,8 @@ impl LeafNodeIn { }) } LeafNodeSource::Update => { - let tree_position = tree_position.ok_or(LibraryError::custom("Internal error"))?; + let tree_position = tree_position + .ok_or(LibraryError::custom("No tree position for Update LeafNode"))?; VerifiableLeafNode::Update(VerifiableUpdateLeafNode { payload: self.payload, signature: self.signature, @@ -714,7 +718,8 @@ impl LeafNodeIn { }) } LeafNodeSource::Commit(_) => { - let tree_position = tree_position.ok_or(LibraryError::custom("Internal error"))?; + let tree_position = tree_position + .ok_or(LibraryError::custom("No tree position for Commit LeafNode"))?; VerifiableLeafNode::Commit(VerifiableCommitLeafNode { payload: self.payload, signature: self.signature, @@ -774,6 +779,22 @@ impl VerifiableLeafNode { VerifiableLeafNode::Commit(v) => v.signature_key(), } } + + pub(crate) async fn validate( + self, + backend: &impl OpenMlsCryptoProvider, + sc: SignatureScheme, + group: Option<&PublicGroup>, + ) -> Result { + match (self, group) { + (VerifiableLeafNode::KeyPackage(ln), None) => ln.standalone_validate(backend, sc).await, + (VerifiableLeafNode::KeyPackage(ln), Some(group)) => ln.validate(group, backend).await, + (VerifiableLeafNode::Update(ln), None) => ln.standalone_validate(backend, sc).await, + (VerifiableLeafNode::Update(ln), Some(group)) => ln.validate(group, backend).await, + (VerifiableLeafNode::Commit(ln), None) => ln.standalone_validate(backend, sc).await, + (VerifiableLeafNode::Commit(ln), Some(group)) => ln.validate(group, backend).await, + } + } } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/openmls/src/treesync/node/validate.rs b/openmls/src/treesync/node/validate.rs index ac8bcef1cd..76a90a72c8 100644 --- a/openmls/src/treesync/node/validate.rs +++ b/openmls/src/treesync/node/validate.rs @@ -13,8 +13,8 @@ use crate::{ }, }, }; -use itertools::Itertools; use openmls_traits::{crypto::OpenMlsCrypto, types::SignatureScheme, OpenMlsCryptoProvider}; +use std::collections::HashSet; impl ValidatableLeafNode for VerifiableCommitLeafNode { fn signature_key(&self) -> &SignaturePublicKey { @@ -47,7 +47,7 @@ impl ValidatableLeafNode for VerifiableUpdateLeafNode { backend: &impl OpenMlsCryptoProvider, ) -> Result { self.validate_replaced_encryption_key(group)?; - self.generic_validate(backend, group).await + self.validate_default(group, backend).await } fn signature_key(&self) -> &SignaturePublicKey { @@ -91,13 +91,23 @@ impl VerifiableUpdateLeafNode { #[cfg_attr(target_family = "wasm", async_trait::async_trait(?Send))] #[cfg_attr(not(target_family = "wasm"), async_trait::async_trait)] impl ValidatableLeafNode for VerifiableKeyPackageLeafNode { + async fn standalone_validate( + self, + backend: &impl OpenMlsCryptoProvider, + signature_scheme: SignatureScheme, + ) -> Result { + self.validate_lifetime()?; + self.standalone_validate_default(backend, signature_scheme) + .await + } + async fn validate( self, group: &PublicGroup, backend: &impl OpenMlsCryptoProvider, ) -> Result { self.validate_lifetime()?; - self.generic_validate(backend, group).await + self.validate_default(group, backend).await } fn signature_key(&self) -> &SignaturePublicKey { @@ -144,8 +154,18 @@ where backend: &impl OpenMlsCryptoProvider, signature_scheme: SignatureScheme, ) -> Result { - let extension_types = self.extension_types(); + self.standalone_validate_default(backend, signature_scheme) + .await + } + + async fn standalone_validate_default( + self, + backend: &impl OpenMlsCryptoProvider, + signature_scheme: SignatureScheme, + ) -> Result { self.validate_credential(backend).await?; + + let extension_types = self.extension_types(); let leaf_node = self.verify_signature(backend.crypto(), signature_scheme)?; Self::validate_extension_support(&leaf_node, &extension_types[..])?; @@ -158,28 +178,20 @@ where group: &PublicGroup, backend: &impl OpenMlsCryptoProvider, ) -> Result { - self.generic_validate(backend, group).await + self.validate_default(group, backend).await } - /// Validation regardless of [LeafNode]'s source - async fn generic_validate( + async fn validate_default( self, - backend: &impl OpenMlsCryptoProvider, group: &PublicGroup, + backend: &impl OpenMlsCryptoProvider, ) -> Result { self.validate_capabilities(group)?; self.validate_credential_type(group)?; let tree = group.treesync(); - self.validate_signature_key_unique(tree)?; - self.validate_encryption_key_unique(tree)?; - self.validate_credential(backend).await?; - - let extension_types = self.extension_types(); + self.validate_signature_encryption_key_unique(tree)?; let signature_scheme = group.ciphersuite().signature_algorithm(); - let leaf_node = self.verify_signature(backend.crypto(), signature_scheme)?; - Self::validate_extension_support(&leaf_node, &extension_types[..])?; - - Ok(leaf_node) + self.standalone_validate(backend, signature_scheme).await } fn signature_key(&self) -> &SignaturePublicKey; @@ -233,24 +245,21 @@ where } /// Verify that the following fields are unique among the members of the group: signature_key - fn validate_signature_key_unique( + fn validate_signature_encryption_key_unique( &self, tree: &TreeSync, ) -> Result<(), LeafNodeValidationError> { - if !tree.raw_leaves().map(LeafNode::signature_key).all_unique() { - return Err(LeafNodeValidationError::SignatureKeyAlreadyInUse); - } - Ok(()) - } - /// Verify that the following fields are unique among the members of the group: encryption_key - fn validate_encryption_key_unique( - &self, - tree: &TreeSync, - ) -> Result<(), LeafNodeValidationError> { - if !tree.raw_leaves().map(LeafNode::encryption_key).all_unique() { - return Err(LeafNodeValidationError::EncryptionKeyAlreadyInUse); + let size = tree.tree_size().leaf_count() as usize; + let mut used_signature_keys = HashSet::with_capacity(size); + let mut used_encryption_keys = HashSet::with_capacity(size); + for ln in tree.raw_leaves() { + if !used_signature_keys.insert(ln.signature_key()) { + return Err(LeafNodeValidationError::SignatureKeyAlreadyInUse); + } + if !used_encryption_keys.insert(ln.encryption_key()) { + return Err(LeafNodeValidationError::EncryptionKeyAlreadyInUse); + } } - Ok(()) } diff --git a/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs b/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs index d362793725..ceab79e21e 100644 --- a/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs +++ b/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs @@ -63,8 +63,10 @@ async fn run_test_vector( let ratchet_tree = RatchetTree::from(RatchetTreeIn::from_nodes(nodes)); - let tree_before = TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree) - .map_err(|e| format!("Error while creating tree sync: {e:?}"))?; + let tree_before = + TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree, &group_id, false) + .await + .map_err(|e| format!("Error while creating tree sync: {e:?}"))?; let group_context = GroupContext::new( ciphersuite, diff --git a/openmls/src/treesync/tests_and_kats/kats/kat_tree_validation.rs b/openmls/src/treesync/tests_and_kats/kats/kat_tree_validation.rs index 978ab154f4..f33b2b9cbf 100644 --- a/openmls/src/treesync/tests_and_kats/kats/kat_tree_validation.rs +++ b/openmls/src/treesync/tests_and_kats/kats/kat_tree_validation.rs @@ -85,7 +85,10 @@ struct TestElement { tree_hashes: Vec, } -fn run_test_vector(test: TestElement, backend: &impl OpenMlsCryptoProvider) -> Result<(), String> { +async fn run_test_vector( + test: TestElement, + backend: &impl OpenMlsCryptoProvider, +) -> Result<(), String> { let ciphersuite = Ciphersuite::try_from(test.cipher_suite).unwrap(); // Skip unsupported ciphersuites. if !backend @@ -106,8 +109,10 @@ fn run_test_vector(test: TestElement, backend: &impl OpenMlsCryptoProvider) -> R .into_verified(ciphersuite, backend.crypto(), group_id) .unwrap(); - let treesync = TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree.clone()) - .map_err(|e| format!("Error while creating tree sync: {e:?}"))?; + let treesync = + TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree.clone(), group_id, true) + .await + .map_err(|e| format!("Error while creating tree sync: {e:?}"))?; let diff = treesync.empty_diff(); @@ -141,7 +146,7 @@ async fn read_test_vectors_tree_validation(backend: &impl OpenMlsCryptoProvider) let tests: Vec = read("test_vectors/tree-validation.json"); for test_vector in tests { - match run_test_vector(test_vector, backend) { + match run_test_vector(test_vector, backend).await { Ok(_) => {} Err(e) => panic!("Error while checking tree validation test vector.\n{e:?}"), } diff --git a/openmls/src/treesync/tests_and_kats/kats/kat_treekem.rs b/openmls/src/treesync/tests_and_kats/kats/kat_treekem.rs index 0d4bdb0f94..ca68a27ff3 100644 --- a/openmls/src/treesync/tests_and_kats/kats/kat_treekem.rs +++ b/openmls/src/treesync/tests_and_kats/kats/kat_treekem.rs @@ -76,7 +76,7 @@ struct LeafNodeInfoTest { signature_keypair: SignatureKeyPair, } -pub fn run_test_vector(test: TreeKemTest, backend: &impl OpenMlsCryptoProvider) { +pub async fn run_test_vector(test: TreeKemTest, backend: &impl OpenMlsCryptoProvider) { // Skip unsupported cipher suites (for now). let ciphersuite = Ciphersuite::try_from(test.cipher_suite).unwrap(); @@ -98,7 +98,9 @@ pub fn run_test_vector(test: TreeKemTest, backend: &impl OpenMlsCryptoProvider) .into_verified(ciphersuite, backend.crypto(), group_id) .unwrap(); - TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree).unwrap() + TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree, group_id, true) + .await + .unwrap() }; let full_leaf_nodes = { @@ -379,14 +381,14 @@ fn apply_update_path( commit_secret } -#[test] -fn read_test_vectors_treekem() { +#[async_std::test] +async fn read_test_vectors_treekem() { let _ = pretty_env_logger::try_init(); let tests: Vec = read("test_vectors/treekem.json"); let backend = OpenMlsRustCrypto::default(); for test in tests.into_iter() { - run_test_vector(test, &backend); + run_test_vector(test, &backend).await; } } diff --git a/openmls/src/treesync/tests_and_kats/tests/test_diff.rs b/openmls/src/treesync/tests_and_kats/tests/test_diff.rs index c3b2ca80f1..7bb816114b 100644 --- a/openmls/src/treesync/tests_and_kats/tests/test_diff.rs +++ b/openmls/src/treesync/tests_and_kats/tests/test_diff.rs @@ -3,6 +3,7 @@ use openmls_traits::{types::Ciphersuite, OpenMlsCryptoProvider}; use rstest::*; use rstest_reuse::apply; +use crate::prelude::GroupId; use crate::test_utils::*; use crate::{ credentials::test_utils::new_credential, @@ -36,8 +37,9 @@ async fn test_free_leaf_computation( ]); // Get the encryption key pair from the leaf. - let tree = TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree) - .expect("error generating tree"); + let group_id = GroupId::random(backend); + let tree = TreeSync::from_ratchet_tree(backend, ciphersuite, ratchet_tree, &group_id, true) + .await.expect("error generating tree"); // Create and add a new leaf. It should go to leaf index 1