From 1f80ec14d3171246568048c69af65ecb028e909e Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Thu, 27 Feb 2020 11:38:51 -0800 Subject: [PATCH 1/2] Deduplicate signatures with the same key_id. We should only verify one signature for one key_id, otherwise an attacker owning one key could simply duplicate one valid signature and thus increasing the number of signatures that are valid. This bypass the limitation of threshold. Tested: Unit tests. --- src/metadata.rs | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/metadata.rs b/src/metadata.rs index bf04a423..372f4af0 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -560,9 +560,15 @@ where let canonical_bytes = D::canonicalize(&self.metadata)?; let mut signatures_needed = threshold; - for sig in &self.signatures { - match authorized_keys.get(sig.key_id()) { - Some(ref pub_key) => match pub_key.verify(&canonical_bytes, &sig) { + // Create a key_id->signature map to deduplicate the key_ids. + let signatures = self + .signatures + .iter() + .map(|sig| (sig.key_id(), sig)) + .collect::>(); + for (key_id, sig) in signatures { + match authorized_keys.get(key_id) { + Some(ref pub_key) => match pub_key.verify(&canonical_bytes, sig) { Ok(()) => { debug!("Good signature from key ID {:?}", pub_key.key_id()); signatures_needed -= 1; @@ -2382,6 +2388,26 @@ mod test { decoded.verify(1, &[root_key.public().clone()]).unwrap(); } + #[test] + fn verify_signed_serialized_root_metadata_with_duplicate_sig() { + let jsn = json!({ + "signatures": [{ + "keyid": "a9f3ebc9b138762563a9c27b6edd439959e559709babd123e8d449ba2c18c61a", + "sig": "c4ba838e0d3f783716393a4d691f568f840733ff488bb79ac68287e97e0b31d63fcef392dbc978e878c2103ba231905af634cc651d6f0e63a35782d051ac6e00" + }, + { + "keyid": "a9f3ebc9b138762563a9c27b6edd439959e559709babd123e8d449ba2c18c61a", + "sig": "c4ba838e0d3f783716393a4d691f568f840733ff488bb79ac68287e97e0b31d63fcef392dbc978e878c2103ba231905af634cc651d6f0e63a35782d051ac6e00" + }], + "signed": jsn_root_metadata_without_keyid_hash_algos() + }); + let root_key = PrivateKey::from_pkcs8(ED25519_1_PK8, SignatureScheme::Ed25519).unwrap(); + let decoded: SignedMetadata = + serde_json::from_value(jsn).unwrap(); + assert_matches!(decoded.verify(2, &[root_key.public().clone()]), Err(Error::VerificationFailure(_))); + decoded.verify(1, &[root_key.public().clone()]).unwrap(); + } + fn verify_signature_with_unknown_fields(mut metadata: serde_json::Value) where M: Metadata, From 5fc3cf303ec9259c3d3982ff08c1d2ee4a31ff86 Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Thu, 27 Feb 2020 14:00:28 -0800 Subject: [PATCH 2/2] Add verification for key ID. TUF spec requires client to check key ID calculation. Due to backward compatibility, instead of giving error when an incorrect key ID is found, we simply ignore that key. Tested: Unit tests. --- src/interchange/cjson/shims.rs | 11 ++++++++++- src/metadata.rs | 21 ++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/interchange/cjson/shims.rs b/src/interchange/cjson/shims.rs index d916318f..32b3429d 100644 --- a/src/interchange/cjson/shims.rs +++ b/src/interchange/cjson/shims.rs @@ -77,11 +77,20 @@ impl RootMetadata { ))); } + // Ignore all keys with incorrect key IDs. We should give an error if the key ID is not + // correct according to TUF spec. However, due to backward compatibility, we may receive + // metadata with key IDs generated by TUF 0.9. We simply ignore those old keys. + let keys_with_correct_key_id = self + .keys + .into_iter() + .filter(|(key_id, pkey)| key_id == pkey.key_id()) + .collect(); + metadata::RootMetadata::new( self.version, parse_datetime(&self.expires)?, self.consistent_snapshot, - self.keys.into_iter().collect(), + keys_with_correct_key_id, self.roles.root, self.roles.snapshot, self.roles.targets, diff --git a/src/metadata.rs b/src/metadata.rs index 372f4af0..ba61dc2c 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -10,6 +10,7 @@ use std::collections::{HashMap, HashSet}; use std::fmt::{self, Debug, Display}; use std::io::Read; use std::marker::PhantomData; +use std::str; use crate::crypto::{self, HashAlgorithm, HashValue, KeyId, PrivateKey, PublicKey, Signature}; use crate::error::Error; @@ -2361,6 +2362,21 @@ mod test { assert_eq!(jsn, encoded); } + #[test] + fn de_ser_root_metadata_wrong_key_id() { + let jsn = jsn_root_metadata_without_keyid_hash_algos(); + let mut jsn_str = str::from_utf8(&Json::canonicalize(&jsn).unwrap()) + .unwrap() + .to_owned(); + // Replace the key id to something else. + jsn_str = jsn_str.replace( + "12435b260b6172bd750aeb102f54a347c56b109e0524ab1f144593c07af66356", + "00435b260b6172bd750aeb102f54a347c56b109e0524ab1f144593c07af66356", + ); + let decoded: RootMetadata = serde_json::from_str(&jsn_str).unwrap(); + assert_eq!(3, decoded.keys.len()); + } + #[test] fn sign_and_verify_root_metadata() { let jsn = jsn_root_metadata_without_keyid_hash_algos(); @@ -2404,7 +2420,10 @@ mod test { let root_key = PrivateKey::from_pkcs8(ED25519_1_PK8, SignatureScheme::Ed25519).unwrap(); let decoded: SignedMetadata = serde_json::from_value(jsn).unwrap(); - assert_matches!(decoded.verify(2, &[root_key.public().clone()]), Err(Error::VerificationFailure(_))); + assert_matches!( + decoded.verify(2, &[root_key.public().clone()]), + Err(Error::VerificationFailure(_)) + ); decoded.verify(1, &[root_key.public().clone()]).unwrap(); }