From c204144ac0b368408b794e684eb3dba2f1472925 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 00:01:57 +0000 Subject: [PATCH 1/8] ConnId: Refactor to prepare for parsing longer identifiers --- shared/src/lib.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 17473a71..95df0ad6 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -144,6 +144,15 @@ impl ConnId { Self(raw) } + /// Read a connection identifier from a given decoder. + /// + /// It is an error for the decoder to read anything but a small integer or a byte string, to + /// exceed the maximum allowed ConnId length, or to contain a byte string that should have been + /// encoded as a small integer. + pub fn from_decoder(decoder: &mut CBORDecoder<'_>) -> Result { + Ok(Self(decoder.int_raw()?)) + } + /// The bytes that form the identifier (an arbitrary byte string) pub fn as_slice(&self) -> &[u8] { core::slice::from_ref(&self.0) @@ -678,7 +687,7 @@ mod edhoc_parser { g_x.copy_from_slice(decoder.bytes_sized(P256_ELEM_LEN)?); // consume c_i encoded as single-byte int (we still do not support bstr encoding) - let c_i = ConnId::from_int_raw(decoder.int_raw()?); + let c_i = ConnId::from_decoder(&mut decoder)?; // if there is still more to parse, the rest will be the EAD_1 if rcvd_message_1.len > decoder.position() { @@ -740,7 +749,7 @@ mod edhoc_parser { let mut decoder = CBORDecoder::new(plaintext_2.as_slice()); - let c_r = ConnId::from_int_raw(decoder.int_raw()?); + let c_r = ConnId::from_decoder(&mut decoder)?; // the id_cred may have been encoded as a single int, a byte string, or a map let id_cred_r = IdCred::from_encoded_value(decoder.any_as_encoded()?)?; From 13886f0f43445324ec0bebebf674a3d8814bc883 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 00:42:33 +0000 Subject: [PATCH 2/8] tests: Make 'invalid C_x' actually invalid The previously used vector was valid but merely unsupported. --- lib/src/edhoc.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/src/edhoc.rs b/lib/src/edhoc.rs index 446f264d..e877fc6e 100644 --- a/lib/src/edhoc.rs +++ b/lib/src/edhoc.rs @@ -1096,8 +1096,9 @@ mod tests { // invalid test vectors, should result in a parsing error const MESSAGE_1_INVALID_ARRAY_TV: &str = "8403025820741a13d7ba048fbb615e94386aa3b61bea5b3d8f65f32620b749bee8d278efa90e"; + // This is invalid because the h'0e' byte string is a text string instead const MESSAGE_1_INVALID_C_I_TV: &str = - "03025820741a13d7ba048fbb615e94386aa3b61bea5b3d8f65f32620b749bee8d278efa9410e"; + "03025820741a13d7ba048fbb615e94386aa3b61bea5b3d8f65f32620b749bee8d278efa9610e"; const MESSAGE_1_INVALID_CIPHERSUITE_TV: &str = "0381025820741a13d7ba048fbb615e94386aa3b61bea5b3d8f65f32620b749bee8d278efa90e"; const MESSAGE_1_INVALID_TEXT_EPHEMERAL_KEY_TV: &str = From 78c400497a0b161045a1ef7813a0d4f927389d99 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 00:46:55 +0000 Subject: [PATCH 3/8] ConnId: Grow to store more legal values (and process them) --- shared/src/lib.rs | 93 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 95df0ad6..99db64bb 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -89,6 +89,15 @@ pub const ENC_STRUCTURE_LEN: usize = 8 + 5 + SHA256_DIGEST_LEN; // 8 for ENCRYPT pub const MAX_EAD_SIZE_LEN: usize = SCALE_FACTOR * 64; +/// Maximum length of a [`ConnId`] (`C_x`). +/// +/// This length includes the leading CBOR encoding byte. +/// +/// This needs to be <= 24; allowing longer connection identifiers requires extending [`ConnId`]'s +/// invariants to also allow more than one leading byte, and [`ConnIdType`] to extend its +/// processing. +const MAX_CONNID_ENCODED_LEN: usize = 8; + pub type BytesSuites = [u8; SUITES_LEN]; pub type BytesSupportedSuites = [u8; SUPPORTED_SUITES_LEN]; pub const EDHOC_SUITES: BytesSuites = [0, 1, 2, 3, 4, 5, 6, 24, 25]; // all but private cipher suites @@ -124,13 +133,35 @@ pub type EADMessageBuffer = EdhocMessageBuffer; // TODO: make it of size MAX_EAD /// /// Semantically, this is a byte string of some length. /// -/// This currently only supports numeric values (see -/// https://github.com/openwsn-berkeley/lakers/issues/258), but may support larger values in the -/// future. +/// Its legal values are constrained to only contain a single CBOR item that is either a byte +/// string or a number in -24..=23, all in preferred encoding. #[derive(Debug, PartialEq, Eq, Copy, Clone)] // TODO: This should not be needed, there is nothing special about the value 0. #[derive(Default)] -pub struct ConnId(u8); +pub struct ConnId([u8; MAX_CONNID_ENCODED_LEN]); + +enum ConnIdType { + SingleByte, + ByteString(usize), +} + +impl ConnIdType { + fn classify(byte: u8) -> Option { + if byte >> 5 <= 1 && byte & 0x1f < 24 { + return Some(ConnIdType::SingleByte); + } else if byte >> 5 == 2 && byte & 0x1f < 24 { + return Some(ConnIdType::ByteString((byte & 0x1f).into())); + } + None + } + + fn length(&self) -> usize { + match self { + ConnIdType::SingleByte => 1, + ConnIdType::ByteString(n) => 1 + n, + } + } +} impl ConnId { /// Construct a ConnId from the result of [`cbor_decoder::int_raw`], which is a @@ -141,7 +172,16 @@ impl ConnId { pub const fn from_int_raw(raw: u8) -> Self { debug_assert!(raw >> 5 <= 1, "Major type is not an integer"); debug_assert!(raw & 0x1f < 24, "Value is not immediate"); - Self(raw) + let mut s = [0; MAX_CONNID_ENCODED_LEN]; + s[0] = raw; + Self(s) + } + + fn classify(&self) -> ConnIdType { + let Some(t) = ConnIdType::classify(self.0[0]) else { + unreachable!("Type invariant requires valid classification") + }; + t } /// Read a connection identifier from a given decoder. @@ -150,12 +190,20 @@ impl ConnId { /// exceed the maximum allowed ConnId length, or to contain a byte string that should have been /// encoded as a small integer. pub fn from_decoder(decoder: &mut CBORDecoder<'_>) -> Result { - Ok(Self(decoder.int_raw()?)) + let mut s = [0; MAX_CONNID_ENCODED_LEN]; + let len = ConnIdType::classify(decoder.current()?) + .ok_or(CBORError::DecodingError)? + .length(); + s[..len].copy_from_slice(decoder.read_slice(len)?); + Ok(Self(s)) } /// The bytes that form the identifier (an arbitrary byte string) pub fn as_slice(&self) -> &[u8] { - core::slice::from_ref(&self.0) + match self.classify() { + ConnIdType::SingleByte => &self.0[..1], + ConnIdType::ByteString(n) => &self.0[1..1 + n], + } } /// The CBOR encoding of the identifier. @@ -171,13 +219,13 @@ impl ConnId { /// /// For other IDs, this contains an extra byte header (but those are currently unsupported): /// - /// ```should_panic + /// ``` /// # use lakers_shared::ConnId; /// let c_i = ConnId::from_slice(&[0xff]).unwrap(); /// assert_eq!(c_i.as_cbor(), &[0x41, 0xff]); /// ``` pub fn as_cbor(&self) -> &[u8] { - core::slice::from_ref(&self.0) + &self.0[..self.classify().length()] } /// Try to construct a ConnId from a slice. @@ -191,20 +239,23 @@ impl ConnId { /// let c_i = ConnId::from_slice(c_i).unwrap(); /// assert!(c_i.as_slice() == &[0x04]); /// - /// // So far, longer slices are unsupported - /// assert!(ConnId::from_slice(&[0x12, 0x34]).is_none()); + /// let long = ConnId::from_slice(&[0x12, 0x34]).unwrap(); + /// assert!(long.as_slice() == &[0x12, 0x34]); /// ``` pub fn from_slice(input: &[u8]) -> Option { - if input.len() != 1 { - return None; - } - if input[0] >> 5 > 1 { - return None; - } - if input[0] & 0x1f >= 24 { - return None; + let mut s = [0; MAX_CONNID_ENCODED_LEN]; + if let &[single_byte] = input { + if matches!( + ConnIdType::classify(single_byte), + Some(ConnIdType::SingleByte) + ) { + s[0] = single_byte; + return Some(Self(s)); + } } - Some(Self(input[0])) + s[0] = input.len() as u8 | 0x40; + s.get_mut(1..1 + input.len())?.copy_from_slice(input); + Some(Self(s)) } } @@ -840,7 +891,7 @@ mod cbor_decoder { } /// Consume and return *n* bytes starting at the current position. - fn read_slice(&mut self, n: usize) -> Result<&'a [u8], CBORError> { + pub fn read_slice(&mut self, n: usize) -> Result<&'a [u8], CBORError> { if let Some(b) = self .pos .checked_add(n) From 1ebdcfca4f15962ec3d9c3ff8ef7aeb2eaa81d67 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 01:04:54 +0000 Subject: [PATCH 4/8] edhoc: Encode C_x as CBOR This used the wrong encoding function, which made no difference before when only short C_x were supported. --- lib/src/edhoc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/edhoc.rs b/lib/src/edhoc.rs index e877fc6e..5c6ab238 100644 --- a/lib/src/edhoc.rs +++ b/lib/src/edhoc.rs @@ -511,7 +511,7 @@ fn encode_message_1( output.content[2 + raw_suites_len] = P256_ELEM_LEN as u8; // length of the byte string output.content[3 + raw_suites_len..3 + raw_suites_len + P256_ELEM_LEN] .copy_from_slice(&g_x[..]); - let c_i = c_i.as_slice(); + let c_i = c_i.as_cbor(); output.len = 3 + raw_suites_len + P256_ELEM_LEN + c_i.len(); output.content[3 + raw_suites_len + P256_ELEM_LEN..][..c_i.len()].copy_from_slice(c_i); @@ -858,7 +858,7 @@ fn encode_plaintext_2( ead_2: &Option, ) -> Result { let mut plaintext_2: BufferPlaintext2 = BufferPlaintext2::new(); - let c_r = c_r.as_slice(); + let c_r = c_r.as_cbor(); plaintext_2 .extend_from_slice(c_r) From 77fc8ef370da92754fa6f9908167f129fe738f44 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 01:20:26 +0000 Subject: [PATCH 5/8] ConnId: Fix hax complaints --- shared/src/lib.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 99db64bb..51aa62e4 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -243,19 +243,20 @@ impl ConnId { /// assert!(long.as_slice() == &[0x12, 0x34]); /// ``` pub fn from_slice(input: &[u8]) -> Option { - let mut s = [0; MAX_CONNID_ENCODED_LEN]; - if let &[single_byte] = input { - if matches!( - ConnIdType::classify(single_byte), - Some(ConnIdType::SingleByte) - ) { - s[0] = single_byte; - return Some(Self(s)); + if input.len() > MAX_CONNID_ENCODED_LEN - 1 { + None + } else { + let mut s = [0; MAX_CONNID_ENCODED_LEN]; + if input.len() == 1 + && matches!(ConnIdType::classify(input[0]), Some(ConnIdType::SingleByte)) + { + s[0] = input[0]; + } else { + s[0] = input.len() as u8 | 0x40; + s[1..1 + input.len()].copy_from_slice(input); } + Some(Self(s)) } - s[0] = input.len() as u8 | 0x40; - s.get_mut(1..1 + input.len())?.copy_from_slice(input); - Some(Self(s)) } } From 7238db0ed8ce4e4807492b84f8f072e0b1faa8e9 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 11:11:44 +0000 Subject: [PATCH 6/8] ConnId: Update docs, move asserts, slim down length type --- shared/src/lib.rs | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 51aa62e4..6c356810 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -91,11 +91,7 @@ pub const MAX_EAD_SIZE_LEN: usize = SCALE_FACTOR * 64; /// Maximum length of a [`ConnId`] (`C_x`). /// -/// This length includes the leading CBOR encoding byte. -/// -/// This needs to be <= 24; allowing longer connection identifiers requires extending [`ConnId`]'s -/// invariants to also allow more than one leading byte, and [`ConnIdType`] to extend its -/// processing. +/// This length includes the leading CBOR encoding byte(s). const MAX_CONNID_ENCODED_LEN: usize = 8; pub type BytesSuites = [u8; SUITES_LEN]; @@ -140,25 +136,40 @@ pub type EADMessageBuffer = EdhocMessageBuffer; // TODO: make it of size MAX_EAD #[derive(Default)] pub struct ConnId([u8; MAX_CONNID_ENCODED_LEN]); +/// Classifier for the content of [`ConnId`]; used internally in its implementation. enum ConnIdType { + /// The ID contains a single positive or negative number, expressed in its first byte. SingleByte, - ByteString(usize), + /// The ID contains a byte string, and the first byte of the ID indicates its length. + /// + /// It is expected that if longer connection IDs than 1+0+n are ever supported, this will be + /// renamed to ByteString10n, and longer variants get their own class. + ByteString(u8), } impl ConnIdType { + const _IMPL_CONSTRAINTS: () = assert!( + MAX_CONNID_ENCODED_LEN <= 1 + 23, + "Longer connection IDs require more elaborate decoding here" + ); + + /// Returns a classifier based on an initial byte. + /// + /// Its signature will need to change if ever connection IDs longer than 1+0+n are supported. fn classify(byte: u8) -> Option { if byte >> 5 <= 1 && byte & 0x1f < 24 { return Some(ConnIdType::SingleByte); } else if byte >> 5 == 2 && byte & 0x1f < 24 { - return Some(ConnIdType::ByteString((byte & 0x1f).into())); + return Some(ConnIdType::ByteString(byte & 0x1f)); } None } + /// Returns the number of bytes in the [`ConnId`]'s buffer. fn length(&self) -> usize { match self { ConnIdType::SingleByte => 1, - ConnIdType::ByteString(n) => 1 + n, + ConnIdType::ByteString(n) => (1 + n).into(), } } } @@ -169,14 +180,22 @@ impl ConnId { /// type. /// /// Evolving from u8-only values, this could later interact with the decoder directly. + #[deprecated( + note = "This API is only capable of generating a limited sub-set of the supported identifiers." + )] pub const fn from_int_raw(raw: u8) -> Self { debug_assert!(raw >> 5 <= 1, "Major type is not an integer"); debug_assert!(raw & 0x1f < 24, "Value is not immediate"); + // We might allow '' (the empty bytes tring, byte 40) as well, but the again, this API is + // already deprecated. let mut s = [0; MAX_CONNID_ENCODED_LEN]; s[0] = raw; Self(s) } + /// The connection ID classification of this connection ID + /// + /// Due to the invariants of this type, this classification infallible. fn classify(&self) -> ConnIdType { let Some(t) = ConnIdType::classify(self.0[0]) else { unreachable!("Type invariant requires valid classification") @@ -202,7 +221,7 @@ impl ConnId { pub fn as_slice(&self) -> &[u8] { match self.classify() { ConnIdType::SingleByte => &self.0[..1], - ConnIdType::ByteString(n) => &self.0[1..1 + n], + ConnIdType::ByteString(n) => &self.0[1..1 + usize::from(n)], } } @@ -217,7 +236,7 @@ impl ConnId { /// assert_eq!(c_i.as_cbor(), &[0x04]); /// ``` /// - /// For other IDs, this contains an extra byte header (but those are currently unsupported): + /// For other IDs, this contains an extra byte header: /// /// ``` /// # use lakers_shared::ConnId; @@ -228,7 +247,7 @@ impl ConnId { &self.0[..self.classify().length()] } - /// Try to construct a ConnId from a slice. + /// Try to construct a [`ConnId`] from a slice that represents its string value. /// /// This is the inverse of [Self::as_slice], and returns None if the identifier is too long /// (or, if only the compact 48 values are supported, outside of that range). @@ -239,8 +258,8 @@ impl ConnId { /// let c_i = ConnId::from_slice(c_i).unwrap(); /// assert!(c_i.as_slice() == &[0x04]); /// - /// let long = ConnId::from_slice(&[0x12, 0x34]).unwrap(); - /// assert!(long.as_slice() == &[0x12, 0x34]); + /// let c_i = ConnId::from_slice(&[0x12, 0x34]).unwrap(); + /// assert!(c_i.as_slice() == &[0x12, 0x34]); /// ``` pub fn from_slice(input: &[u8]) -> Option { if input.len() > MAX_CONNID_ENCODED_LEN - 1 { From 286775626303e76e43cd9d06b0390733daf4faaf Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 11:18:23 +0000 Subject: [PATCH 7/8] ConnID: With quadruple_size, support the maximum easy lengths --- shared/src/lib.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 6c356810..d900924f 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -92,7 +92,12 @@ pub const MAX_EAD_SIZE_LEN: usize = SCALE_FACTOR * 64; /// Maximum length of a [`ConnId`] (`C_x`). /// /// This length includes the leading CBOR encoding byte(s). -const MAX_CONNID_ENCODED_LEN: usize = 8; +// If ints had a const `.clamp()` feature, this could be (8 * SCALE_FACTOR).clamp(1, 23). +const MAX_CONNID_ENCODED_LEN: usize = if cfg!(feature = "quadruple_sizes") { + 24 +} else { + 8 +}; pub type BytesSuites = [u8; SUITES_LEN]; pub type BytesSupportedSuites = [u8; SUPPORTED_SUITES_LEN]; From bf8a7f27c1ffc03eee2cd9055bc827326e488d49 Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 4 Nov 2024 13:42:47 +0000 Subject: [PATCH 8/8] context_2 calculaton: Use C_x as CBOR item rather than slice This made no difference as long as all C_x were numeric and had identical slice and CBOR representations. --- lib/src/edhoc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/edhoc.rs b/lib/src/edhoc.rs index 5c6ab238..0b53c586 100644 --- a/lib/src/edhoc.rs +++ b/lib/src/edhoc.rs @@ -776,7 +776,7 @@ fn encode_kdf_context( let mut output: BytesMaxContextBuffer = [0x00; MAX_KDF_CONTEXT_LEN]; let mut output_len = if let Some(c_r) = c_r { - let c_r = c_r.as_slice(); + let c_r = c_r.as_cbor(); output[..c_r.len()].copy_from_slice(c_r); c_r.len() } else {