From 7fd215fb6d3a851a2c4b76c13d6e7f19a0b46518 Mon Sep 17 00:00:00 2001 From: chrysn Date: Wed, 27 Nov 2024 16:03:56 +0100 Subject: [PATCH 1/2] tests/shared: Add examples of non-acceptable credentials The current code accepts them but produces wrong public keys, resulting in highly confusing `assertion `left == right` failed: Public key is not a good point` errors being raised from subtle/p256_ecdh. --- shared/src/cred.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/shared/src/cred.rs b/shared/src/cred.rs index ac0ed777..12329042 100644 --- a/shared/src/cred.rs +++ b/shared/src/cred.rs @@ -409,6 +409,17 @@ mod test { ); assert_eq!(cred.kid.unwrap().as_slice(), KID_VALUE_TV); assert_eq!(cred.cred_type, CredentialType::CCS); + + // A CCS without a subject. It's OK if this starts working in future, but then its + // public key needs to start with F5AEBA08B599754 (it'd be clearly wrong if this produced + // an Ok value with a different public key). + let cred_no_sub = hex!("a108a101a401022001215820f5aeba08b599754ba16f5db80feafdf91e90a5a7ccb2e83178adb51b8c68ea9522582097e7a3fdd70a3a7c0a5f9578c6e4e96d8bc55f6edd0ff64f1caeaac19d37b67d"); + Credential::parse_ccs(&cred_no_sub).unwrap_err(); + // A CCS without a KID. It's OK if this starts working in future, but then its + // public key needs to start with F5AEBA08B599754 (it'd be clearly wrong if this produced + // an Ok value with a different public key). + let cred_no_kid = hex!("a20263666f6f08a101a401022001215820f5aeba08b599754ba16f5db80feafdf91e90a5a7ccb2e83178adb51b8c68ea9522582097e7a3fdd70a3a7c0a5f9578c6e4e96d8bc55f6edd0ff64f1caeaac19d37b67d"); + Credential::parse_ccs(&cred_no_kid).unwrap_err(); } #[rstest] From 2eb54de22960288ca858e3c311cde273da2cf88b Mon Sep 17 00:00:00 2001 From: chrysn Date: Wed, 27 Nov 2024 16:27:17 +0100 Subject: [PATCH 2/2] parse_ccs: Be strict --- shared/src/cred.rs | 119 ++++++++++++++++++++++++++++++--------------- 1 file changed, 81 insertions(+), 38 deletions(-) diff --git a/shared/src/cred.rs b/shared/src/cred.rs index 12329042..098b6908 100644 --- a/shared/src/cred.rs +++ b/shared/src/cred.rs @@ -217,50 +217,93 @@ impl Credential { /// If the given value matches the shape lakers expects of a CCS, i.e. credentials from RFC9529, /// its public key and key ID are extracted into a full credential. pub fn parse_ccs(value: &[u8]) -> Result { - const CCS_PREFIX_LEN: usize = 3; - const CNF_AND_COSE_KEY_PREFIX_LEN: usize = 8; - const COSE_KEY_FIRST_ITEMS_LEN: usize = 6; + let mut decoder = CBORDecoder::new(value); + if decoder.map()? != 2 { + // eg. no subject present + return Err(EDHOCError::ParsingError); + } - if value.len() - < 3 + CCS_PREFIX_LEN - + 1 - + CNF_AND_COSE_KEY_PREFIX_LEN - + COSE_KEY_FIRST_ITEMS_LEN - + P256_ELEM_LEN - { - Err(EDHOCError::ParsingError) - } else { - let subject_len = CBORDecoder::info_of(value[2]) as usize; + if decoder.u8()? != 2 { + // expected 2 (subject) + return Err(EDHOCError::ParsingError); + } - let id_cred_offset: usize = CCS_PREFIX_LEN - .checked_add(subject_len) - .and_then(|x| x.checked_add(CNF_AND_COSE_KEY_PREFIX_LEN)) - .ok_or(EDHOCError::ParsingError)?; + let _subject = decoder.str()?; - let g_a_x_offset: usize = id_cred_offset - .checked_add(COSE_KEY_FIRST_ITEMS_LEN) - .ok_or(EDHOCError::ParsingError)?; + if decoder.u8()? != 8 { + // expected 8 (cnf) + return Err(EDHOCError::ParsingError); + } - if g_a_x_offset - .checked_add(P256_ELEM_LEN) - .map_or(false, |end| end <= value.len()) - { - let public_key: BytesKeyEC2 = value[g_a_x_offset..g_a_x_offset + P256_ELEM_LEN] - .try_into() - .expect("Wrong key length"); - let kid = value[id_cred_offset]; + if decoder.map()? != 1 { + // cnf is always single-item'd + return Err(EDHOCError::ParsingError); + } - Ok(Self { - bytes: BufferCred::new_from_slice(value) - .map_err(|_| EDHOCError::ParsingError)?, - key: CredentialKey::EC2Compact(public_key), - kid: Some(BufferKid::new_from_slice(&[kid]).unwrap()), - cred_type: CredentialType::CCS, - }) - } else { - Err(EDHOCError::ParsingError) - } + if decoder.u8()? != 1 { + // Unexpected cnf + return Err(EDHOCError::ParsingError); + } + + if decoder.map()? != 5 { + // Right now we're *very* strict and expect exactly 1/kty=/ec2, 2/kid, -1/crv, -2/x, -3/y. + return Err(EDHOCError::ParsingError); + } + + // kty: EC2 + if decoder.u8()? != 1 { + return Err(EDHOCError::ParsingError); + } + if decoder.u8()? != 2 { + return Err(EDHOCError::ParsingError); + } + + // kid: bytes. Note that this is always a byte string, even if in other places it's used + // with integer compression. + if decoder.u8()? != 2 { + return Err(EDHOCError::ParsingError); + } + let kid = decoder.bytes()?; + let kid = BufferKid::new_from_slice(kid) + // Could be too long + .map_err(|_| EDHOCError::ParsingError)?; + + // crv: p-256 + if decoder.i8()? != -1 { + return Err(EDHOCError::ParsingError); } + if decoder.u8()? != 1 { + return Err(EDHOCError::ParsingError); + } + + // x + if decoder.i8()? != -2 { + return Err(EDHOCError::ParsingError); + } + let x = decoder.bytes()?; + let x = CredentialKey::EC2Compact( + x + // Wrong length + .try_into() + .map_err(|_| EDHOCError::ParsingError)?, + ); + + // y + if decoder.i8()? != -3 { + return Err(EDHOCError::ParsingError); + } + let y = decoder.bytes()?; + + if !decoder.finished() { + return Err(EDHOCError::ParsingError); + } + + Ok(Self { + bytes: BufferCred::new_from_slice(value).map_err(|_| EDHOCError::ParsingError)?, + key: x, + kid: Some(kid), + cred_type: CredentialType::CCS, + }) } /// Parse a CCS style credential, but the key is a symmetric key.