From 0d8f0589185e4004f2d816821ff360683dd6dc00 Mon Sep 17 00:00:00 2001 From: chrysn Date: Fri, 29 Sep 2023 16:00:36 +0200 Subject: [PATCH 1/2] feat(api)!: Isolate EAD implementation details BREAKING CHANGE: EADItem's constructor changed, and its fields became private. --- consts/src/lib.rs | 60 +++++++++++++++++++++++++------ ead/edhoc-ead-zeroconf/src/lib.rs | 18 +++------- 2 files changed, 54 insertions(+), 24 deletions(-) diff --git a/consts/src/lib.rs b/consts/src/lib.rs index fb48fc74..8d49086d 100644 --- a/consts/src/lib.rs +++ b/consts/src/lib.rs @@ -88,23 +88,61 @@ mod common { #[derive(Debug)] pub struct EADItem { - pub label: u8, - pub is_critical: bool, + pub(crate) label: u8, + pub(crate) is_critical: bool, // TODO[ead]: have adjustable (smaller) length for this buffer - pub value: Option, + pub(crate) value: Option, } - pub trait EADTrait { - fn new() -> Self; + #[derive(Debug)] + #[non_exhaustive] + /// Error created from [EADTrait::new] + pub enum EADNewError { + /// The value given exceeds the size that is statically allocated per EAD item. + SizeExceeded, + /// The label value given at construction exceeds the range of EAD labels the library can + /// handle. + InexpressibleLabel, + /// The label indicates use as padding, but non-zero values were encountered. + InvalidPadding, + } + + pub trait EADTrait: Sized { + /// Create a new EAD option + fn new(label: u16, is_critical: bool, value: Option<&[u8]>) -> Result; + + // It may make sense to add a `new_padding()` function that allows adding padding with + // neither the null check nor the need for the caller to allocate a slice of zeros. + + fn label(&self) -> u16; + fn is_critical(&self) -> bool; + fn value(&self) -> Option<&[u8]>; } impl EADTrait for EADItem { - fn new() -> Self { - EADItem { - label: 0, - is_critical: false, - value: None, - } + #[inline(always)] // Assist const propagation that removes error states + fn new(label: u16, is_critical: bool, value: Option<&[u8]>) -> Result { + Ok(EADItem { + label: label + .try_into() + .map_err(|_| EADNewError::InexpressibleLabel)?, + is_critical, + value: value + .map(|v| v.try_into().map_err(|_| EADNewError::SizeExceeded)) + .transpose()?, + }) + } + + fn label(&self) -> u16 { + self.label.into() + } + + fn is_critical(&self) -> bool { + self.is_critical + } + + fn value(&self) -> Option<&[u8]> { + self.value.as_ref().map(|v| &v.content[..v.len]) } } diff --git a/ead/edhoc-ead-zeroconf/src/lib.rs b/ead/edhoc-ead-zeroconf/src/lib.rs index ac1fcb5b..c10c146c 100644 --- a/ead/edhoc-ead-zeroconf/src/lib.rs +++ b/ead/edhoc-ead-zeroconf/src/lib.rs @@ -38,13 +38,10 @@ pub fn ead_initiator_set_global_state(new_state: EADInitiatorState) { } pub fn i_prepare_ead_1() -> Option { - let mut ead_1 = EADItem::new(); - - // this ead item is critical - ead_1.label = EAD_ZEROCONF_LABEL; - ead_1.is_critical = true; - // TODO: build Voucher_Info (LOC_W, ENC_ID), and append it to the buffer + let mut ead_1 = EADItem::new(EAD_ZEROCONF_LABEL.into(), true, None) + // Const propagation will remove this. + .unwrap(); ead_initiator_set_global_state(EADInitiatorState { protocol_state: EADInitiatorProtocolState::WaitEAD2, @@ -65,7 +62,7 @@ pub fn i_process_ead_2(ead_2: EADItem) -> Result<(), ()> { } pub fn i_prepare_ead_3() -> Option { - Some(EADItem::new()) + Some(EADItem::new(0, false, None).unwrap()) } // responder side @@ -116,13 +113,8 @@ pub fn r_process_ead_1(ead_1: EADItem) -> Result<(), ()> { } pub fn r_prepare_ead_2() -> Option { - let mut ead_2 = EADItem::new(); - - // add the label to the buffer (non-critical) - ead_2.label = EAD_ZEROCONF_LABEL; - ead_2.is_critical = true; - // TODO: append Voucher (H(message_1), CRED_V) to the buffer + let ead_2 = EADItem::new(EAD_ZEROCONF_LABEL.into(), true, None).unwrap(); // NOTE: see the note in lib.rs::test_ead // state.protocol_state = EADResponderProtocolState::WaitMessage3; From 6f730cb2e8aefe2ea320d2ce63d467898e4b4295 Mon Sep 17 00:00:00 2001 From: chrysn Date: Fri, 29 Sep 2023 16:21:36 +0200 Subject: [PATCH 2/2] feat: Allow full EAD label range This increases the usable size of EAD labels to +-65535. It also adds a bit of CBOR processing, to the point where it may make sense to use a proper CBOR library instead; that is likely most easily achieved after completion of [105]. [105]: https://github.com/openwsn-berkeley/edhoc-rs/issues/105 --- consts/src/lib.rs | 41 +++++---- ead/edhoc-ead-zeroconf/src/lib.rs | 4 +- hacspec/src/lib.rs | 145 ++++++++++++++++++++++++------ lib/src/edhoc.rs | 2 +- 4 files changed, 141 insertions(+), 51 deletions(-) diff --git a/consts/src/lib.rs b/consts/src/lib.rs index 8d49086d..bf759b81 100644 --- a/consts/src/lib.rs +++ b/consts/src/lib.rs @@ -88,7 +88,7 @@ mod common { #[derive(Debug)] pub struct EADItem { - pub(crate) label: u8, + pub(crate) label: u16, pub(crate) is_critical: bool, // TODO[ead]: have adjustable (smaller) length for this buffer pub(crate) value: Option, @@ -100,9 +100,6 @@ mod common { pub enum EADNewError { /// The value given exceeds the size that is statically allocated per EAD item. SizeExceeded, - /// The label value given at construction exceeds the range of EAD labels the library can - /// handle. - InexpressibleLabel, /// The label indicates use as padding, but non-zero values were encountered. InvalidPadding, } @@ -123,9 +120,7 @@ mod common { #[inline(always)] // Assist const propagation that removes error states fn new(label: u16, is_critical: bool, value: Option<&[u8]>) -> Result { Ok(EADItem { - label: label - .try_into() - .map_err(|_| EADNewError::InexpressibleLabel)?, + label, is_critical, value: value .map(|v| v.try_into().map_err(|_| EADNewError::SizeExceeded)) @@ -134,7 +129,7 @@ mod common { } fn label(&self) -> u16 { - self.label.into() + self.label } fn is_critical(&self) -> bool { @@ -149,7 +144,7 @@ mod common { pub const MAX_MESSAGE_SIZE_LEN: usize = 64; pub const MAX_EAD_SIZE_LEN: usize = 64; pub type EADMessageBuffer = EdhocMessageBuffer; // TODO: make it of size MAX_EAD_SIZE_LEN - pub const EAD_ZEROCONF_LABEL: u8 = 0x1; // NOTE: in lake-authz-draft-02 it is still TBD1 + pub const EAD_ZEROCONF_LABEL: u16 = 0x1; // NOTE: in lake-authz-draft-02 it is still TBD1 pub const ID_CRED_LEN: usize = 4; pub const SUITES_LEN: usize = 9; @@ -169,8 +164,11 @@ mod common { pub const MAX_BUFFER_LEN: usize = 220; pub const CBOR_BYTE_STRING: u8 = 0x58u8; pub const CBOR_UINT_1BYTE: u8 = 0x18u8; + pub const CBOR_UINT_2BYTE: u8 = 0x19u8; pub const CBOR_NEG_INT_1BYTE_START: u8 = 0x20u8; pub const CBOR_NEG_INT_1BYTE_END: u8 = 0x37u8; + pub const CBOR_NEG_INT_1BYTE: u8 = 0x38u8; + pub const CBOR_NEG_INT_2BYTE: u8 = 0x39u8; pub const CBOR_UINT_1BYTE_START: u8 = 0x0u8; pub const CBOR_UINT_1BYTE_END: u8 = 0x17u8; pub const CBOR_MAJOR_TEXT_STRING: u8 = 0x60u8; @@ -305,7 +303,7 @@ mod hacspec { #[derive(Debug)] pub struct EADItemHacspec { - pub label: U8, + pub label: U16, pub is_critical: bool, // TODO[ead]: have adjustable (smaller) length for this buffer pub value: Option, @@ -320,15 +318,15 @@ mod hacspec { impl EADItemHacspecTrait for EADItemHacspec { fn new() -> Self { EADItemHacspec { - label: U8(0), + label: U16(0), is_critical: false, value: None, } } fn from_public_item(item: &EADItem) -> Self { EADItemHacspec { - label: U8(item.label), - is_critical: item.is_critical, + label: U16(item.label()), + is_critical: item.is_critical(), value: match &item.value { Some(value) => Some(EdhocMessageBufferHacspec::from_public_buffer(value)), None => None, @@ -336,14 +334,15 @@ mod hacspec { } } fn to_public_item(&self) -> EADItem { - EADItem { - label: self.label.declassify(), - is_critical: self.is_critical, - value: match &self.value { - Some(value) => Some(value.to_public_buffer()), - None => None, - }, - } + let value_full = self + .value + .as_ref() + .map(|v| (v.content.to_public_array(), v.len)); + let value = value_full + .as_ref() + .map(|(value, len)| &value[..(*len as usize)]); + + EADItem::new(self.label.declassify(), self.is_critical, value).unwrap() } } diff --git a/ead/edhoc-ead-zeroconf/src/lib.rs b/ead/edhoc-ead-zeroconf/src/lib.rs index c10c146c..bfe56f5f 100644 --- a/ead/edhoc-ead-zeroconf/src/lib.rs +++ b/ead/edhoc-ead-zeroconf/src/lib.rs @@ -39,7 +39,7 @@ pub fn ead_initiator_set_global_state(new_state: EADInitiatorState) { pub fn i_prepare_ead_1() -> Option { // TODO: build Voucher_Info (LOC_W, ENC_ID), and append it to the buffer - let mut ead_1 = EADItem::new(EAD_ZEROCONF_LABEL.into(), true, None) + let mut ead_1 = EADItem::new(EAD_ZEROCONF_LABEL, true, None) // Const propagation will remove this. .unwrap(); @@ -114,7 +114,7 @@ pub fn r_process_ead_1(ead_1: EADItem) -> Result<(), ()> { pub fn r_prepare_ead_2() -> Option { // TODO: append Voucher (H(message_1), CRED_V) to the buffer - let ead_2 = EADItem::new(EAD_ZEROCONF_LABEL.into(), true, None).unwrap(); + let ead_2 = EADItem::new(EAD_ZEROCONF_LABEL, true, None).unwrap(); // NOTE: see the note in lib.rs::test_ead // state.protocol_state = EADResponderProtocolState::WaitMessage3; diff --git a/hacspec/src/lib.rs b/hacspec/src/lib.rs index 7e8f49e3..83182281 100644 --- a/hacspec/src/lib.rs +++ b/hacspec/src/lib.rs @@ -746,6 +746,12 @@ fn is_cbor_uint_2bytes(byte: U8) -> bool { return byte.declassify() == CBOR_UINT_1BYTE; } +/// Check for: an unsigned integer encoded as three bytes +#[inline(always)] +fn is_cbor_uint_3bytes(byte: U8) -> bool { + return byte.declassify() == CBOR_UINT_2BYTE; +} + /// Check for: a negative integer encoded as a single byte #[inline(always)] fn is_cbor_neg_int_1byte(byte: U8) -> bool { @@ -753,6 +759,18 @@ fn is_cbor_neg_int_1byte(byte: U8) -> bool { return byte >= CBOR_NEG_INT_1BYTE_START && byte <= CBOR_NEG_INT_1BYTE_END; } +/// Check for: an unsigned integer encoded as two bytes +#[inline(always)] +fn is_cbor_neg_int_2bytes(byte: U8) -> bool { + return byte.declassify() == CBOR_NEG_INT_1BYTE; +} + +/// Check for: an unsigned integer encoded as three bytes +#[inline(always)] +fn is_cbor_neg_int_3bytes(byte: U8) -> bool { + return byte.declassify() == CBOR_NEG_INT_2BYTE; +} + /// Check for: a bstr denoted by a single byte which encodes both type and content length #[inline(always)] fn is_cbor_bstr_1byte_prefix(byte: U8) -> bool { @@ -773,6 +791,81 @@ fn is_cbor_array_1byte_prefix(byte: U8) -> bool { return byte >= CBOR_MAJOR_ARRAY && byte <= CBOR_MAJOR_ARRAY_MAX; } +/// If the first CBOR item in `message` is a CBOR (unsigned or negative) integer, return its +/// argument value, whether the number is negative, and how many bytes were consumed. Note that the +/// argument of a negative number is offset by 1: `(0, true, _)` means -1, `(65535, true, _)` means +/// -65536 +// This would really profit from using a regular CBOR library, or at very least looking like one; +// that step is left for later refactoring. +pub fn parse_cbor16(message: &BytesMessageBuffer, offset: usize) -> Option<(u16, bool, usize)> { + let label = message[offset]; + if is_cbor_uint_1byte(label) { + // CBOR unsigned integer (0..=23) + Some((label.declassify().into(), false, 1)) + } else if is_cbor_uint_2bytes(label) { + Some((message[offset + 1].declassify().into(), false, 2)) + } else if is_cbor_uint_3bytes(label) { + Some(( + (u16::from(message[offset + 1].declassify()) << 8) + + u16::from(message[offset + 2].declassify()), + false, + 3, + )) + } else if is_cbor_neg_int_1byte(label) { + // CBOR negative integer (-1..=-24) + Some(( + (label.declassify() - CBOR_NEG_INT_1BYTE_START).into(), + true, + 1, + )) + } else if is_cbor_neg_int_2bytes(label) { + Some((message[offset + 1].declassify().into(), true, 2)) + } else if is_cbor_neg_int_3bytes(label) { + Some(( + (u16::from(message[offset + 1].declassify()) << 8) + + u16::from(message[offset + 2].declassify()), + true, + 3, + )) + } else { + None + } +} + +/// Encode a positive or negative integer into the message at the given offset, and return how many +/// bytes were written. Note that as with parse_cbor16, the argument is offset by 1 from the +/// absolute numeric value. +pub fn encode_cbor16( + message: &mut BytesMessageBuffer, + offset: usize, + argument: u16, + is_negative: bool, +) -> usize { + let major_bits = if is_negative { + CBOR_NEG_INT_1BYTE_START + } else { + CBOR_UINT_1BYTE_START + }; + + match argument { + 0..=0x17 => { + message[offset] = U8(major_bits | (u8::try_from(argument).unwrap())); + 1 + } + ..=0xff => { + message[offset] = U8(major_bits | CBOR_UINT_1BYTE); + message[offset + 1] = U8(argument.try_into().unwrap()); + 2 + } + _ => { + message[offset] = U8(major_bits | CBOR_UINT_2BYTE); + message[offset + 1] = U8((argument >> 8).try_into().unwrap()); + message[offset + 2] = U8(argument as u8); + 3 + } + } +} + fn parse_suites_i( rcvd_message_1: &BufferMessage1, ) -> Result<(BytesSuites, usize, usize), EDHOCError> { @@ -847,30 +940,24 @@ fn parse_ead( let mut ead_value = None::; // assume label is a single byte integer (negative or positive) - let label = message.content[offset]; - let res_label = if is_cbor_uint_1byte(label) { - // CBOR unsigned integer (0..=23) - Ok((label.declassify() as u8, false)) - } else if is_cbor_neg_int_1byte(label) { - // CBOR negative integer (-1..=-24) - Ok((label.declassify() - (CBOR_NEG_INT_1BYTE_START - 1), true)) - } else { - Err(EDHOCError::ParsingError) - }; + let res_label = parse_cbor16(&message.content, offset).ok_or(EDHOCError::ParsingError); if res_label.is_ok() { - let (label, is_critical) = res_label.unwrap(); - if message.len > (offset + 1) { + let (mut label, is_critical, consumed) = res_label.unwrap(); + if message.len > (offset + consumed) { // EAD value is present let buffer = EdhocMessageBufferHacspec::from_slice( &message.content, - offset + 1, - message.len - (offset + 1), + offset + consumed, + message.len - (offset + consumed), ); ead_value = Some(buffer); } + if is_critical { + label = label.checked_add(1).ok_or(EDHOCError::EADError)?; + } ead_item = Some(EADItemHacspec { - label: U8(label), + label: U16(label), is_critical, value: ead_value, }); @@ -964,19 +1051,23 @@ fn parse_message_1( fn encode_ead_item(ead_1: &EADItemHacspec) -> EdhocMessageBufferHacspec { let mut output = EdhocMessageBufferHacspec::new(); - // encode label - if ead_1.is_critical { - output.content[0] = ead_1.label + U8(CBOR_NEG_INT_1BYTE_START - 1); + let label = if ead_1.is_critical { + // A critical (padding) 0 might be constructed but makes no sense and will be encoded as + // regular padding. + ead_1.label.declassify().saturating_sub(1) } else { - output.content[0] = ead_1.label; - } - output.len = 1; + ead_1.label.declassify() + }; + // encode label + let written = encode_cbor16(&mut output.content, 0, label, ead_1.is_critical); + output.len = written; // encode value if let Some(ead_1_value) = &ead_1.value { - output.content = output - .content - .update_slice(1, &ead_1_value.content, 0, ead_1_value.len); + output.content = + output + .content + .update_slice(written, &ead_1_value.content, 0, ead_1_value.len); output.len += ead_1_value.len; } @@ -1621,7 +1712,7 @@ mod tests { const MESSAGE_1_TV_SUITE_ONLY_C: &str = "0382021819"; // message with an array having too many cipher suites (more than 9) const MESSAGE_1_TV_SUITE_ONLY_ERR: &str = "038A02020202020202020202"; - const EAD_DUMMY_LABEL_TV: u8 = 0x01; + const EAD_DUMMY_LABEL_TV: u16 = 0x01; const EAD_DUMMY_VALUE_TV: &str = "cccccc"; const EAD_DUMMY_CRITICAL_TV: &str = "20cccccc"; const MESSAGE_1_WITH_DUMMY_EAD_NO_VALUE_TV: &str = @@ -2131,7 +2222,7 @@ mod tests { let ead_tv = EdhocMessageBufferHacspec::from_hex(EAD_DUMMY_CRITICAL_TV); let ead_item = EADItemHacspec { - label: U8(EAD_DUMMY_LABEL_TV), + label: U16(EAD_DUMMY_LABEL_TV), is_critical: true, value: Some(EdhocMessageBufferHacspec::from_hex(EAD_DUMMY_VALUE_TV)), }; @@ -2149,7 +2240,7 @@ mod tests { let c_i_tv = U8(C_I_TV); let message_1_ead_tv = BufferMessage1::from_hex(MESSAGE_1_WITH_DUMMY_CRITICAL_EAD_TV); let ead_item = EADItemHacspec { - label: U8(EAD_DUMMY_LABEL_TV), + label: U16(EAD_DUMMY_LABEL_TV), is_critical: true, value: Some(EdhocMessageBufferHacspec::from_hex(EAD_DUMMY_VALUE_TV)), }; diff --git a/lib/src/edhoc.rs b/lib/src/edhoc.rs index 8c12a296..cb0905c2 100644 --- a/lib/src/edhoc.rs +++ b/lib/src/edhoc.rs @@ -1533,7 +1533,7 @@ mod tests { const MESSAGE_1_TV_SUITE_ONLY_C: &str = "0382021819"; // message with an array having too many cipher suites (more than 9) const MESSAGE_1_TV_SUITE_ONLY_ERR: &str = "038A02020202020202020202"; - const EAD_DUMMY_LABEL_TV: u8 = 0x01; + const EAD_DUMMY_LABEL_TV: u16 = 0x01; const EAD_DUMMY_VALUE_TV: &str = "cccccc"; const EAD_DUMMY_CRITICAL_TV: &str = "20cccccc"; const MESSAGE_1_WITH_DUMMY_EAD_NO_VALUE_TV: &str =