From 7565588be7851cfd68fd2c17a33ececf48d54fd5 Mon Sep 17 00:00:00 2001 From: gnieto Date: Thu, 2 Mar 2023 10:34:57 +0100 Subject: [PATCH] signatures: Prevent accepting events without at least one valid signature `verify_event` has been changed recently to be more aligned with the spec. In the previous algorithm, it returned an error if the event was not signed by at least one of the required entities. The new algortihm is iterating over all the signtures for the required entities *and* skipping unknown algorithms. If an event is signed only by unkonwn algorithms, the event would be accepted and not verifications will happen. In order to prevent that, we check that, at least, a single key is checked. This covers the 2nd point in: https://spec.matrix.org/v1.6/appendices/#checking-for-a-signature A few more test cases were added: - The event is properly signed, but key map contains a key with an unknown algorithm. Under this circumstances, the event should be allowed. - An event signed by multiple keys for an entity, should verify all those signatures - An event signed by a single key with an unkown algorithm by the required entity, should fail --- crates/ruma-signatures/CHANGELOG.md | 4 + crates/ruma-signatures/src/functions.rs | 192 ++++++++++++++++++++++-- 2 files changed, 184 insertions(+), 12 deletions(-) diff --git a/crates/ruma-signatures/CHANGELOG.md b/crates/ruma-signatures/CHANGELOG.md index 2ff569c9..c0a12a53 100644 --- a/crates/ruma-signatures/CHANGELOG.md +++ b/crates/ruma-signatures/CHANGELOG.md @@ -1,5 +1,9 @@ # [unreleased] +Bug fixes: + +- Ignore keys with unknown algorithms in `verify_events` + # 0.13.1 No changes for this version diff --git a/crates/ruma-signatures/src/functions.rs b/crates/ruma-signatures/src/functions.rs index f1bd805c..96ef8e13 100644 --- a/crates/ruma-signatures/src/functions.rs +++ b/crates/ruma-signatures/src/functions.rs @@ -582,6 +582,7 @@ pub fn verify_event( .get(entity_id.as_str()) .ok_or_else(|| VerificationError::public_key_not_found(entity_id))?; + let mut checked = false; for (key_id, signature) in signature_set { // Since only ed25519 is supported right now, we don't actually need to check what the // algorithm is. If it split successfully, it's ed25519. @@ -608,6 +609,11 @@ pub fn verify_event( signature.as_bytes(), &canonical_json, )?; + checked = true; + } + + if !checked { + return Err(VerificationError::UnknownPublicKeysForSignature.into()); } } @@ -799,8 +805,8 @@ mod tests { #[test] fn verify_event_check_signatures_for_both_sender_and_event_id() { - let key_pair_sender = generate_key_pair(); - let key_pair_event = generate_key_pair(); + let key_pair_sender = generate_key_pair("1"); + let key_pair_event = generate_key_pair("2"); let mut signed_event = serde_json::from_str( r#"{ "event_id": "$event_id:domain-event", @@ -837,8 +843,8 @@ mod tests { #[test] fn verify_event_check_signatures_for_authorized_user() { - let key_pair_sender = generate_key_pair(); - let key_pair_authorized = generate_key_pair(); + let key_pair_sender = generate_key_pair("1"); + let key_pair_authorized = generate_key_pair("2"); let mut signed_event = serde_json::from_str( r#"{ "event_id": "$event_id:domain-event", @@ -875,7 +881,7 @@ mod tests { #[test] fn verification_fails_if_missing_signatures_for_authorized_user() { - let key_pair_sender = generate_key_pair(); + let key_pair_sender = generate_key_pair("1"); let mut signed_event = serde_json::from_str( r#"{ "event_id": "$event_id:domain-event", @@ -913,7 +919,7 @@ mod tests { #[test] fn verification_fails_if_required_keys_are_not_given() { - let key_pair_sender = generate_key_pair(); + let key_pair_sender = generate_key_pair("1"); let mut signed_event = serde_json::from_str( r#"{ @@ -950,7 +956,7 @@ mod tests { #[test] fn verify_event_fails_if_public_key_is_invalid() { - let key_pair_sender = generate_key_pair(); + let key_pair_sender = generate_key_pair("1"); let mut signed_event = serde_json::from_str( r#"{ @@ -976,7 +982,7 @@ mod tests { let mut public_key_map = PublicKeyMap::new(); let mut sender_key_map = PublicKeySet::new(); - let newly_generated_key_pair = generate_key_pair(); + let newly_generated_key_pair = generate_key_pair("2"); let encoded_public_key = Base64::new(newly_generated_key_pair.public_key().to_owned()); let version = ServerSigningKeyId::from_parts( SigningKeyAlgorithm::Ed25519, @@ -996,14 +1002,163 @@ mod tests { assert!(format!("{error:?}").contains("Some(Verification equation was not satisfied)")); } - fn generate_key_pair() -> Ed25519KeyPair { + #[test] + fn verify_event_check_signatures_for_sender_is_allowed_with_unknown_algorithms_in_key_map() { + let key_pair_sender = generate_key_pair("1"); + let mut signed_event = serde_json::from_str( + r#"{ + "auth_events": [], + "content": {}, + "depth": 3, + "hashes": { + "sha256": "5jM4wQpv6lnBo7CLIghJuHdW+s2CMBJPUOGOC89ncos" + }, + "origin": "domain", + "origin_server_ts": 1000000, + "prev_events": [], + "room_id": "!x:domain", + "sender": "@name:domain-sender", + "type": "X", + "unsigned": { + "age_ts": 1000000 + } + }"#, + ) + .unwrap(); + sign_json("domain-sender", &key_pair_sender, &mut signed_event).unwrap(); + + let mut public_key_map = BTreeMap::new(); + add_key_to_map(&mut public_key_map, "domain-sender", &key_pair_sender); + add_invalid_key_to_map(&mut public_key_map, "domain-sender", &generate_key_pair("2")); + + let verification = + verify_event(&public_key_map, &signed_event, &RoomVersionId::V6).unwrap(); + + assert_eq!(verification, Verified::Signatures); + } + + #[test] + fn verify_event_fails_with_missing_key_when_event_is_signed_multiple_times_by_same_entity() { + let key_pair_sender = generate_key_pair("1"); + let secondary_key_pair_sender = generate_key_pair("2"); + let mut signed_event = serde_json::from_str( + r#"{ + "auth_events": [], + "content": {}, + "depth": 3, + "hashes": { + "sha256": "5jM4wQpv6lnBo7CLIghJuHdW+s2CMBJPUOGOC89ncos" + }, + "origin": "domain", + "origin_server_ts": 1000000, + "prev_events": [], + "room_id": "!x:domain", + "sender": "@name:domain-sender", + "type": "X", + "unsigned": { + "age_ts": 1000000 + } + }"#, + ) + .unwrap(); + sign_json("domain-sender", &key_pair_sender, &mut signed_event).unwrap(); + sign_json("domain-sender", &secondary_key_pair_sender, &mut signed_event).unwrap(); + + let mut public_key_map = BTreeMap::new(); + add_key_to_map(&mut public_key_map, "domain-sender", &key_pair_sender); + + let verification_result = verify_event(&public_key_map, &signed_event, &RoomVersionId::V6); + + assert_matches!( + verification_result, + Err(Error::Verification(VerificationError::UnknownPublicKeysForSignature)) + ); + } + + #[test] + fn verify_event_checks_all_signatures_from_sender_entity() { + let key_pair_sender = generate_key_pair("1"); + let secondary_key_pair_sender = generate_key_pair("2"); + let mut signed_event = serde_json::from_str( + r#"{ + "auth_events": [], + "content": {}, + "depth": 3, + "hashes": { + "sha256": "5jM4wQpv6lnBo7CLIghJuHdW+s2CMBJPUOGOC89ncos" + }, + "origin": "domain", + "origin_server_ts": 1000000, + "prev_events": [], + "room_id": "!x:domain", + "sender": "@name:domain-sender", + "type": "X", + "unsigned": { + "age_ts": 1000000 + } + }"#, + ) + .unwrap(); + sign_json("domain-sender", &key_pair_sender, &mut signed_event).unwrap(); + sign_json("domain-sender", &secondary_key_pair_sender, &mut signed_event).unwrap(); + + let mut public_key_map = BTreeMap::new(); + add_key_to_map(&mut public_key_map, "domain-sender", &key_pair_sender); + add_key_to_map(&mut public_key_map, "domain-sender", &secondary_key_pair_sender); + + let verification = + verify_event(&public_key_map, &signed_event, &RoomVersionId::V6).unwrap(); + + assert_eq!(verification, Verified::Signatures); + } + + #[test] + fn verify_event_with_single_key_with_unknown_algorithm_should_not_accept_event() { + let key_pair_sender = generate_key_pair("1"); + let signed_event = serde_json::from_str( + r#"{ + "auth_events": [], + "content": {}, + "depth": 3, + "hashes": { + "sha256": "5jM4wQpv6lnBo7CLIghJuHdW+s2CMBJPUOGOC89ncos" + }, + "origin": "domain", + "origin_server_ts": 1000000, + "prev_events": [], + "room_id": "!x:domain", + "sender": "@name:domain-sender", + "type": "X", + "unsigned": { + "age_ts": 1000000 + }, + "signatures": { + "domain-sender": { + "an-unknown-algorithm:1": "pE5UT/4JiY7YZDtZDOsEaxc0wblurdoYqNQx4bCXORA3vLFOGOK10Q/xXVLPWWgIKo15LNvWwWd/2YjmdPvYCg" + } + } + }"#, + ) + .unwrap(); + + let mut public_key_map = BTreeMap::new(); + add_invalid_key_to_map(&mut public_key_map, "domain-sender", &key_pair_sender); + + let verification_result = verify_event(&public_key_map, &signed_event, &RoomVersionId::V6); + assert_matches!( + verification_result, + Err(Error::Verification(VerificationError::UnknownPublicKeysForSignature)) + ); + } + + fn generate_key_pair(name: &str) -> Ed25519KeyPair { let key_content = Ed25519KeyPair::generate().unwrap(); - Ed25519KeyPair::from_der(&key_content, "1".to_owned()) + Ed25519KeyPair::from_der(&key_content, name.to_owned()) .unwrap_or_else(|_| panic!("{:?}", &key_content)) } fn add_key_to_map(public_key_map: &mut PublicKeyMap, name: &str, pair: &Ed25519KeyPair) { - let mut sender_key_map = PublicKeySet::new(); + let sender_key_map = public_key_map.entry(name.to_owned()).or_default(); let encoded_public_key = Base64::new(pair.public_key().to_owned()); let version = ServerSigningKeyId::from_parts( SigningKeyAlgorithm::Ed25519, @@ -1011,7 +1166,20 @@ mod tests { ); sender_key_map.insert(version.to_string(), encoded_public_key); + } - public_key_map.insert(name.to_owned(), sender_key_map); + fn add_invalid_key_to_map( + public_key_map: &mut PublicKeyMap, + name: &str, + pair: &Ed25519KeyPair, + ) { + let sender_key_map = public_key_map.entry(name.to_owned()).or_default(); + let encoded_public_key = Base64::new(pair.public_key().to_owned()); + let version = ServerSigningKeyId::from_parts( + SigningKeyAlgorithm::from("an-unknown-algorithm"), + pair.version().try_into().unwrap(), + ); + + sender_key_map.insert(version.to_string(), encoded_public_key); } }