From f4b791ed9149b25e0489bb2d5a392af46f1dbac8 Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Sat, 9 May 2020 15:55:55 +0200 Subject: [PATCH 1/9] Encode credentials as a protocol buffer message This permits to decode a credential of a different version without failing. --- libraries/cbor/src/macros.rs | 22 ++++-- libraries/cbor/src/writer.rs | 2 +- src/ctap/data_formats.rs | 126 +++++++++++++++++++++-------------- src/ctap/mod.rs | 4 ++ src/ctap/storage.rs | 7 +- 5 files changed, 102 insertions(+), 59 deletions(-) diff --git a/libraries/cbor/src/macros.rs b/libraries/cbor/src/macros.rs index 5a3d8f5..e5f3781 100644 --- a/libraries/cbor/src/macros.rs +++ b/libraries/cbor/src/macros.rs @@ -41,18 +41,30 @@ macro_rules! cbor_map_options { }; ( $( $key:expr => $value:expr ),* ) => { + cbor_extend_map_options! ( + ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new(), + $( $key => $value, )* + ) + }; +} + +#[macro_export] +macro_rules! cbor_extend_map_options { + // Add trailing comma if missing. + ( $initial:expr, $( $key:expr => $value:expr ),+ ) => { + cbor_extend_map_options! ( $initial, $($key => $value,)+ ) + }; + + ( $initial:expr, $( $key:expr => $value:expr, )* ) => { { // The import is unused if the list is empty. #[allow(unused_imports)] use $crate::values::{IntoCborKey, IntoCborValueOption}; - let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new(); + let mut _map = $initial; $( - { - let opt: Option<$crate::values::Value> = $value.into_cbor_value_option(); - if let Some(val) = opt { + if let Some(val) = $value.into_cbor_value_option() { _map.insert($key.into_cbor_key(), val); } - } )* $crate::values::Value::Map(_map) } diff --git a/libraries/cbor/src/writer.rs b/libraries/cbor/src/writer.rs index 1273160..0764851 100644 --- a/libraries/cbor/src/writer.rs +++ b/libraries/cbor/src/writer.rs @@ -36,7 +36,7 @@ impl<'a> Writer<'a> { return false; } match value { - Value::KeyValue(KeyType::Unsigned(unsigned)) => self.start_item(0, unsigned as u64), + Value::KeyValue(KeyType::Unsigned(unsigned)) => self.start_item(0, unsigned), Value::KeyValue(KeyType::Negative(negative)) => { self.start_item(1, -(negative + 1) as u64) } diff --git a/src/ctap/data_formats.rs b/src/ctap/data_formats.rs index 8e6c7d9..6873d8a 100644 --- a/src/ctap/data_formats.rs +++ b/src/ctap/data_formats.rs @@ -445,59 +445,83 @@ pub struct PublicKeyCredentialSource { pub user_handle: Vec, // not optional, but nullable pub other_ui: Option, pub cred_random: Option>, + + /// Contains the unknown fields when parsing a CBOR value. + /// + /// Those fields could either be deleted fields from older versions (they should have reserved + /// tags) or fields from newer versions (the tags should not be reserved). If this is empty, + /// then the parsed credential is probably from the same version (but not necessarily). + pub unknown_fields: BTreeMap, +} + +// We simulate protocol buffers in CBOR with maps. Each field of a message is associated with a +// unique tag, implemented with a CBOR unsigned key. +#[repr(u64)] +enum PublicKeyCredentialSourceField { + CredentialId = 0, + PrivateKey = 1, + RpId = 2, + UserHandle = 3, + OtherUi = 4, + CredRandom = 5, + // When a field is removed, its tag should be reserved and not used for new fields. We document + // those reserved tags below. + // Reserved tags: none. +} + +impl From for cbor::KeyType { + fn from(field: PublicKeyCredentialSourceField) -> cbor::KeyType { + (field as u64).into() + } } impl From for cbor::Value { fn from(credential: PublicKeyCredentialSource) -> cbor::Value { - let mut private_key = [0u8; 32]; + use PublicKeyCredentialSourceField::*; + let mut private_key = [0; 32]; credential.private_key.to_bytes(&mut private_key); - let other_ui = match credential.other_ui { - None => cbor_null!(), - Some(other_ui) => cbor_text!(other_ui), - }; - let cred_random = match credential.cred_random { - None => cbor_null!(), - Some(cred_random) => cbor_bytes!(cred_random), - }; - cbor_array! { - credential.credential_id, - private_key, - credential.rp_id, - credential.user_handle, - other_ui, - cred_random, + cbor_extend_map_options! { + credential.unknown_fields, + CredentialId => Some(credential.credential_id), + PrivateKey => Some(private_key.to_vec()), + RpId => Some(credential.rp_id), + UserHandle => Some(credential.user_handle), + OtherUi => credential.other_ui, + CredRandom => credential.cred_random } } } -impl TryFrom for PublicKeyCredentialSource { - type Error = Ctap2StatusCode; +impl PublicKeyCredentialSource { + pub fn parse_cbor(cbor_value: cbor::Value) -> Option { + use PublicKeyCredentialSourceField::*; + let mut map = match cbor_value { + cbor::Value::Map(x) => x, + _ => return None, + }; - fn try_from(cbor_value: cbor::Value) -> Result { - use cbor::{SimpleValue, Value}; - - let fields = read_array(&cbor_value)?; - if fields.len() != 6 { - return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR); - } - let credential_id = read_byte_string(&fields[0])?; - let private_key = read_byte_string(&fields[1])?; + let credential_id = read_byte_string(&map.remove(&CredentialId.into())?).ok()?; + let private_key = read_byte_string(&map.remove(&PrivateKey.into())?).ok()?; if private_key.len() != 32 { - return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR); + return None; } - let private_key = ecdsa::SecKey::from_bytes(array_ref!(private_key, 0, 32)) - .ok_or(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR)?; - let rp_id = read_text_string(&fields[2])?; - let user_handle = read_byte_string(&fields[3])?; - let other_ui = match &fields[4] { - Value::Simple(SimpleValue::NullValue) => None, - cbor_value => Some(read_text_string(cbor_value)?), - }; - let cred_random = match &fields[5] { - Value::Simple(SimpleValue::NullValue) => None, - cbor_value => Some(read_byte_string(cbor_value)?), - }; - Ok(PublicKeyCredentialSource { + let private_key = ecdsa::SecKey::from_bytes(array_ref!(private_key, 0, 32))?; + let rp_id = read_text_string(&map.remove(&RpId.into())?).ok()?; + let user_handle = read_byte_string(&map.remove(&UserHandle.into())?).ok()?; + let other_ui = map + .remove(&OtherUi.into()) + .as_ref() + .map(read_text_string) + .transpose() + .ok()?; + let cred_random = map + .remove(&CredRandom.into()) + .as_ref() + .map(read_byte_string) + .transpose() + .ok()?; + let unknown_fields = map; + Some(PublicKeyCredentialSource { key_type: PublicKeyCredentialType::PublicKey, credential_id, private_key, @@ -505,6 +529,7 @@ impl TryFrom for PublicKeyCredentialSource { user_handle, other_ui, cred_random, + unknown_fields, }) } } @@ -1218,11 +1243,12 @@ mod test { user_handle: b"foo".to_vec(), other_ui: None, cred_random: None, + unknown_fields: BTreeMap::new(), }; assert_eq!( - PublicKeyCredentialSource::try_from(cbor::Value::from(credential.clone())), - Ok(credential.clone()) + PublicKeyCredentialSource::parse_cbor(cbor::Value::from(credential.clone())), + Some(credential.clone()) ); let credential = PublicKeyCredentialSource { @@ -1231,8 +1257,8 @@ mod test { }; assert_eq!( - PublicKeyCredentialSource::try_from(cbor::Value::from(credential.clone())), - Ok(credential.clone()) + PublicKeyCredentialSource::parse_cbor(cbor::Value::from(credential.clone())), + Some(credential.clone()) ); let credential = PublicKeyCredentialSource { @@ -1241,15 +1267,15 @@ mod test { }; assert_eq!( - PublicKeyCredentialSource::try_from(cbor::Value::from(credential.clone())), - Ok(credential) + PublicKeyCredentialSource::parse_cbor(cbor::Value::from(credential.clone())), + Some(credential) ); } #[test] fn test_credential_source_invalid_cbor() { - assert!(PublicKeyCredentialSource::try_from(cbor_false!()).is_err()); - assert!(PublicKeyCredentialSource::try_from(cbor_array!(false)).is_err()); - assert!(PublicKeyCredentialSource::try_from(cbor_array!(b"foo".to_vec())).is_err()); + assert!(PublicKeyCredentialSource::parse_cbor(cbor_false!()).is_none()); + assert!(PublicKeyCredentialSource::parse_cbor(cbor_array!(false)).is_none()); + assert!(PublicKeyCredentialSource::parse_cbor(cbor_array!(b"foo".to_vec())).is_none()); } } diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index a39a983..55a1e40 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -335,6 +335,7 @@ where user_handle: vec![], other_ui: None, cred_random: None, + unknown_fields: BTreeMap::new(), }) } @@ -501,6 +502,7 @@ where .user_display_name .map(|s| truncate_to_char_boundary(&s, 64).to_string()), cred_random, + unknown_fields: BTreeMap::new(), }; self.persistent_store.store_credential(credential_source)?; random_id @@ -1279,6 +1281,7 @@ mod test { user_handle: vec![], other_ui: None, cred_random: None, + unknown_fields: BTreeMap::new(), }; assert!(ctap_state .persistent_store @@ -1476,6 +1479,7 @@ mod test { user_handle: vec![], other_ui: None, cred_random: None, + unknown_fields: BTreeMap::new(), }; assert!(ctap_state .persistent_store diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index f0a2ca4..41f4ebe 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -16,9 +16,9 @@ use crate::crypto::rng256::Rng256; use crate::ctap::data_formats::PublicKeyCredentialSource; use crate::ctap::status_code::Ctap2StatusCode; use crate::ctap::PIN_AUTH_LENGTH; +use alloc::collections::BTreeMap; use alloc::string::String; use alloc::vec::Vec; -use core::convert::TryInto; use ctap2::embedded_flash::{self, StoreConfig, StoreEntry, StoreError, StoreIndex}; #[cfg(any(test, feature = "ram_storage"))] @@ -420,8 +420,7 @@ impl From for Ctap2StatusCode { } fn deserialize_credential(data: &[u8]) -> Option { - let cbor = cbor::read(data).ok()?; - cbor.try_into().ok() + PublicKeyCredentialSource::parse_cbor(cbor::read(data).ok()?) } fn serialize_credential(credential: PublicKeyCredentialSource) -> Result, Ctap2StatusCode> { @@ -454,6 +453,7 @@ mod test { user_handle, other_ui: None, cred_random: None, + unknown_fields: BTreeMap::new(), } } @@ -623,6 +623,7 @@ mod test { user_handle: vec![0x00], other_ui: None, cred_random: None, + unknown_fields: BTreeMap::new(), }; assert_eq!(found_credential, Some(expected_credential)); } From e6fdcacd329628b2df739f68d5d3ed6e5bc7d683 Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Mon, 11 May 2020 15:18:27 +0200 Subject: [PATCH 2/9] Remove mention to protobuf --- src/ctap/data_formats.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ctap/data_formats.rs b/src/ctap/data_formats.rs index 6873d8a..f84720c 100644 --- a/src/ctap/data_formats.rs +++ b/src/ctap/data_formats.rs @@ -454,8 +454,8 @@ pub struct PublicKeyCredentialSource { pub unknown_fields: BTreeMap, } -// We simulate protocol buffers in CBOR with maps. Each field of a message is associated with a -// unique tag, implemented with a CBOR unsigned key. +// We serialize credentials for the persistent storage using CBOR maps. Each field of a credential +// is associated with a unique tag, implemented with a CBOR unsigned key. #[repr(u64)] enum PublicKeyCredentialSourceField { CredentialId = 0, From 491721b421ca07ae3b88f1cd7c7657714376ad14 Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Mon, 11 May 2020 16:07:59 +0200 Subject: [PATCH 3/9] Rename extend_cbor_map_options --- libraries/cbor/src/macros.rs | 21 ++++++++++----------- src/ctap/data_formats.rs | 8 +++++--- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/libraries/cbor/src/macros.rs b/libraries/cbor/src/macros.rs index e5f3781..51bfc83 100644 --- a/libraries/cbor/src/macros.rs +++ b/libraries/cbor/src/macros.rs @@ -41,32 +41,31 @@ macro_rules! cbor_map_options { }; ( $( $key:expr => $value:expr ),* ) => { - cbor_extend_map_options! ( - ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new(), - $( $key => $value, )* - ) + { + let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new(); + extend_cbor_map_options! ( &mut _map, $( $key => $value, )* ); + $crate::values::Value::Map(_map) + } }; } #[macro_export] -macro_rules! cbor_extend_map_options { +macro_rules! extend_cbor_map_options { // Add trailing comma if missing. - ( $initial:expr, $( $key:expr => $value:expr ),+ ) => { - cbor_extend_map_options! ( $initial, $($key => $value,)+ ) + ( &mut $initial:expr, $( $key:expr => $value:expr ),+ ) => { + extend_cbor_map_options! ( &mut $initial, $($key => $value,)+ ) }; - ( $initial:expr, $( $key:expr => $value:expr, )* ) => { + ( &mut $initial:expr, $( $key:expr => $value:expr, )* ) => { { // The import is unused if the list is empty. #[allow(unused_imports)] use $crate::values::{IntoCborKey, IntoCborValueOption}; - let mut _map = $initial; $( if let Some(val) = $value.into_cbor_value_option() { - _map.insert($key.into_cbor_key(), val); + $initial.insert($key.into_cbor_key(), val); } )* - $crate::values::Value::Map(_map) } }; } diff --git a/src/ctap/data_formats.rs b/src/ctap/data_formats.rs index f84720c..e227d87 100644 --- a/src/ctap/data_formats.rs +++ b/src/ctap/data_formats.rs @@ -480,15 +480,17 @@ impl From for cbor::Value { use PublicKeyCredentialSourceField::*; let mut private_key = [0; 32]; credential.private_key.to_bytes(&mut private_key); - cbor_extend_map_options! { - credential.unknown_fields, + let mut result = credential.unknown_fields; + extend_cbor_map_options! { + &mut result, CredentialId => Some(credential.credential_id), PrivateKey => Some(private_key.to_vec()), RpId => Some(credential.rp_id), UserHandle => Some(credential.user_handle), OtherUi => credential.other_ui, CredRandom => credential.cred_random - } + }; + cbor::Value::Map(result) } } From 479de32a9544e15badb23c575dcfbfa083e6266d Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Mon, 11 May 2020 16:40:11 +0200 Subject: [PATCH 4/9] Rename $initial to $map --- libraries/cbor/src/macros.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/cbor/src/macros.rs b/libraries/cbor/src/macros.rs index 51bfc83..81d303e 100644 --- a/libraries/cbor/src/macros.rs +++ b/libraries/cbor/src/macros.rs @@ -52,18 +52,18 @@ macro_rules! cbor_map_options { #[macro_export] macro_rules! extend_cbor_map_options { // Add trailing comma if missing. - ( &mut $initial:expr, $( $key:expr => $value:expr ),+ ) => { - extend_cbor_map_options! ( &mut $initial, $($key => $value,)+ ) + ( &mut $map:expr, $( $key:expr => $value:expr ),+ ) => { + extend_cbor_map_options! ( &mut $map, $($key => $value,)+ ) }; - ( &mut $initial:expr, $( $key:expr => $value:expr, )* ) => { + ( &mut $map:expr, $( $key:expr => $value:expr, )* ) => { { // The import is unused if the list is empty. #[allow(unused_imports)] use $crate::values::{IntoCborKey, IntoCborValueOption}; $( if let Some(val) = $value.into_cbor_value_option() { - $initial.insert($key.into_cbor_key(), val); + $map.insert($key.into_cbor_key(), val); } )* } From ca6f910c26bf567b95133c9175f07ed8408d29b2 Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Wed, 13 May 2020 11:09:32 +0200 Subject: [PATCH 5/9] Remove unknown fields --- libraries/cbor/src/macros.rs | 25 +++------- src/ctap/data_formats.rs | 89 ++++++++++++++++-------------------- src/ctap/mod.rs | 4 -- src/ctap/storage.rs | 6 +-- 4 files changed, 49 insertions(+), 75 deletions(-) diff --git a/libraries/cbor/src/macros.rs b/libraries/cbor/src/macros.rs index 81d303e..5a3d8f5 100644 --- a/libraries/cbor/src/macros.rs +++ b/libraries/cbor/src/macros.rs @@ -41,31 +41,20 @@ macro_rules! cbor_map_options { }; ( $( $key:expr => $value:expr ),* ) => { - { - let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new(); - extend_cbor_map_options! ( &mut _map, $( $key => $value, )* ); - $crate::values::Value::Map(_map) - } - }; -} - -#[macro_export] -macro_rules! extend_cbor_map_options { - // Add trailing comma if missing. - ( &mut $map:expr, $( $key:expr => $value:expr ),+ ) => { - extend_cbor_map_options! ( &mut $map, $($key => $value,)+ ) - }; - - ( &mut $map:expr, $( $key:expr => $value:expr, )* ) => { { // The import is unused if the list is empty. #[allow(unused_imports)] use $crate::values::{IntoCborKey, IntoCborValueOption}; + let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new(); $( - if let Some(val) = $value.into_cbor_value_option() { - $map.insert($key.into_cbor_key(), val); + { + let opt: Option<$crate::values::Value> = $value.into_cbor_value_option(); + if let Some(val) = opt { + _map.insert($key.into_cbor_key(), val); } + } )* + $crate::values::Value::Map(_map) } }; } diff --git a/src/ctap/data_formats.rs b/src/ctap/data_formats.rs index e227d87..98a0524 100644 --- a/src/ctap/data_formats.rs +++ b/src/ctap/data_formats.rs @@ -433,6 +433,9 @@ impl TryFrom<&cbor::Value> for SignatureAlgorithm { } // https://www.w3.org/TR/webauthn/#public-key-credential-source +// +// Note that we only use the WebAuthn definition as an example. This data-structure is not specified +// by FIDO. In particular we may choose how we serialize and deserialize it. #[derive(Clone)] #[cfg_attr(test, derive(PartialEq))] #[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))] @@ -445,18 +448,10 @@ pub struct PublicKeyCredentialSource { pub user_handle: Vec, // not optional, but nullable pub other_ui: Option, pub cred_random: Option>, - - /// Contains the unknown fields when parsing a CBOR value. - /// - /// Those fields could either be deleted fields from older versions (they should have reserved - /// tags) or fields from newer versions (the tags should not be reserved). If this is empty, - /// then the parsed credential is probably from the same version (but not necessarily). - pub unknown_fields: BTreeMap, } // We serialize credentials for the persistent storage using CBOR maps. Each field of a credential // is associated with a unique tag, implemented with a CBOR unsigned key. -#[repr(u64)] enum PublicKeyCredentialSourceField { CredentialId = 0, PrivateKey = 1, @@ -480,50 +475,48 @@ impl From for cbor::Value { use PublicKeyCredentialSourceField::*; let mut private_key = [0; 32]; credential.private_key.to_bytes(&mut private_key); - let mut result = credential.unknown_fields; - extend_cbor_map_options! { - &mut result, + cbor_map_options! { CredentialId => Some(credential.credential_id), PrivateKey => Some(private_key.to_vec()), RpId => Some(credential.rp_id), UserHandle => Some(credential.user_handle), OtherUi => credential.other_ui, CredRandom => credential.cred_random - }; - cbor::Value::Map(result) + } } } -impl PublicKeyCredentialSource { - pub fn parse_cbor(cbor_value: cbor::Value) -> Option { - use PublicKeyCredentialSourceField::*; - let mut map = match cbor_value { - cbor::Value::Map(x) => x, - _ => return None, - }; +impl TryFrom<&cbor::Value> for PublicKeyCredentialSource { + type Error = Ctap2StatusCode; - let credential_id = read_byte_string(&map.remove(&CredentialId.into())?).ok()?; - let private_key = read_byte_string(&map.remove(&PrivateKey.into())?).ok()?; + fn try_from(cbor_value: &cbor::Value) -> Result { + use PublicKeyCredentialSourceField::*; + let map = read_map(cbor_value)?; + let credential_id = read_byte_string(ok_or_missing(map.get(&CredentialId.into()))?)?; + let private_key = read_byte_string(ok_or_missing(map.get(&PrivateKey.into()))?)?; if private_key.len() != 32 { - return None; + return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR); } - let private_key = ecdsa::SecKey::from_bytes(array_ref!(private_key, 0, 32))?; - let rp_id = read_text_string(&map.remove(&RpId.into())?).ok()?; - let user_handle = read_byte_string(&map.remove(&UserHandle.into())?).ok()?; - let other_ui = map - .remove(&OtherUi.into()) - .as_ref() - .map(read_text_string) - .transpose() - .ok()?; + let private_key = ecdsa::SecKey::from_bytes(array_ref!(private_key, 0, 32)) + .ok_or(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR)?; + let rp_id = read_text_string(ok_or_missing(map.get(&RpId.into()))?)?; + let user_handle = read_byte_string(ok_or_missing(map.get(&UserHandle.into()))?)?; + let other_ui = map.get(&OtherUi.into()).map(read_text_string).transpose()?; let cred_random = map - .remove(&CredRandom.into()) - .as_ref() + .get(&CredRandom.into()) .map(read_byte_string) - .transpose() - .ok()?; - let unknown_fields = map; - Some(PublicKeyCredentialSource { + .transpose()?; + // We don't return whether there were unknown fields in the CBOR value. This means that + // deserialization is not injective. In particular deserialization is only an inverse of + // serialization at a given version of OpenSK. This is not a problem because: + // 1. When a field is deprecated, its tag is reserved and never reused in future versions, + // including to be reintroduced with the same semantics. In other words, removing a field + // is permanent. + // 2. OpenSK is never used with a more recent version of the storage. In particular, OpenSK + // is never rolled-back. + // As a consequence, the unknown fields are only reserved fields and don't need to be + // preserved. + Ok(PublicKeyCredentialSource { key_type: PublicKeyCredentialType::PublicKey, credential_id, private_key, @@ -531,7 +524,6 @@ impl PublicKeyCredentialSource { user_handle, other_ui, cred_random, - unknown_fields, }) } } @@ -1245,12 +1237,11 @@ mod test { user_handle: b"foo".to_vec(), other_ui: None, cred_random: None, - unknown_fields: BTreeMap::new(), }; assert_eq!( - PublicKeyCredentialSource::parse_cbor(cbor::Value::from(credential.clone())), - Some(credential.clone()) + PublicKeyCredentialSource::try_from(&cbor::Value::from(credential.clone())), + Ok(credential.clone()) ); let credential = PublicKeyCredentialSource { @@ -1259,8 +1250,8 @@ mod test { }; assert_eq!( - PublicKeyCredentialSource::parse_cbor(cbor::Value::from(credential.clone())), - Some(credential.clone()) + PublicKeyCredentialSource::try_from(&cbor::Value::from(credential.clone())), + Ok(credential.clone()) ); let credential = PublicKeyCredentialSource { @@ -1269,15 +1260,15 @@ mod test { }; assert_eq!( - PublicKeyCredentialSource::parse_cbor(cbor::Value::from(credential.clone())), - Some(credential) + PublicKeyCredentialSource::try_from(&cbor::Value::from(credential.clone())), + Ok(credential) ); } #[test] fn test_credential_source_invalid_cbor() { - assert!(PublicKeyCredentialSource::parse_cbor(cbor_false!()).is_none()); - assert!(PublicKeyCredentialSource::parse_cbor(cbor_array!(false)).is_none()); - assert!(PublicKeyCredentialSource::parse_cbor(cbor_array!(b"foo".to_vec())).is_none()); + assert!(PublicKeyCredentialSource::try_from(&cbor_false!()).is_err()); + assert!(PublicKeyCredentialSource::try_from(&cbor_array!(false)).is_err()); + assert!(PublicKeyCredentialSource::try_from(&cbor_array!(b"foo".to_vec())).is_err()); } } diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 55a1e40..a39a983 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -335,7 +335,6 @@ where user_handle: vec![], other_ui: None, cred_random: None, - unknown_fields: BTreeMap::new(), }) } @@ -502,7 +501,6 @@ where .user_display_name .map(|s| truncate_to_char_boundary(&s, 64).to_string()), cred_random, - unknown_fields: BTreeMap::new(), }; self.persistent_store.store_credential(credential_source)?; random_id @@ -1281,7 +1279,6 @@ mod test { user_handle: vec![], other_ui: None, cred_random: None, - unknown_fields: BTreeMap::new(), }; assert!(ctap_state .persistent_store @@ -1479,7 +1476,6 @@ mod test { user_handle: vec![], other_ui: None, cred_random: None, - unknown_fields: BTreeMap::new(), }; assert!(ctap_state .persistent_store diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 41f4ebe..2a1a182 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -16,9 +16,9 @@ use crate::crypto::rng256::Rng256; use crate::ctap::data_formats::PublicKeyCredentialSource; use crate::ctap::status_code::Ctap2StatusCode; use crate::ctap::PIN_AUTH_LENGTH; -use alloc::collections::BTreeMap; use alloc::string::String; use alloc::vec::Vec; +use core::convert::TryFrom; use ctap2::embedded_flash::{self, StoreConfig, StoreEntry, StoreError, StoreIndex}; #[cfg(any(test, feature = "ram_storage"))] @@ -420,7 +420,7 @@ impl From for Ctap2StatusCode { } fn deserialize_credential(data: &[u8]) -> Option { - PublicKeyCredentialSource::parse_cbor(cbor::read(data).ok()?) + PublicKeyCredentialSource::try_from(&cbor::read(data).ok()?).ok() } fn serialize_credential(credential: PublicKeyCredentialSource) -> Result, Ctap2StatusCode> { @@ -453,7 +453,6 @@ mod test { user_handle, other_ui: None, cred_random: None, - unknown_fields: BTreeMap::new(), } } @@ -623,7 +622,6 @@ mod test { user_handle: vec![0x00], other_ui: None, cred_random: None, - unknown_fields: BTreeMap::new(), }; assert_eq!(found_credential, Some(expected_credential)); } From 146e6f083b9ceb7b45260ff1d6fa82f5a24772aa Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Thu, 14 May 2020 21:32:16 +0200 Subject: [PATCH 6/9] Don't rely on unification for array element type --- src/ctap/data_formats.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctap/data_formats.rs b/src/ctap/data_formats.rs index 98a0524..63f74f1 100644 --- a/src/ctap/data_formats.rs +++ b/src/ctap/data_formats.rs @@ -473,7 +473,7 @@ impl From for cbor::KeyType { impl From for cbor::Value { fn from(credential: PublicKeyCredentialSource) -> cbor::Value { use PublicKeyCredentialSourceField::*; - let mut private_key = [0; 32]; + let mut private_key = [0u8; 32]; credential.private_key.to_bytes(&mut private_key); cbor_map_options! { CredentialId => Some(credential.credential_id), From 721a5ba08b4926a38deaa2dca05f873d8d82a076 Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Thu, 14 May 2020 21:48:55 +0200 Subject: [PATCH 7/9] Update reproducible hashes --- reproducible/reference_binaries_macos-10.15.sha256sum | 10 +++++----- reproducible/reference_binaries_ubuntu-18.04.sha256sum | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/reproducible/reference_binaries_macos-10.15.sha256sum b/reproducible/reference_binaries_macos-10.15.sha256sum index 9d3264a..700519f 100644 --- a/reproducible/reference_binaries_macos-10.15.sha256sum +++ b/reproducible/reference_binaries_macos-10.15.sha256sum @@ -1,9 +1,9 @@ b113945b033eb229e3821542f5889769e5fd2e2ae3cb85c6d13a4e05a44a9866 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840dk.bin -d81264ffbce075a78067b666db1988cd0e44e25c0e3f708ada28e90ff29f7339 target/nrf52840dk_merged.hex +9dfb62c7826994bf6e2bae972ea4dc03f1c62f4b49cea379c1f0aff9bbcfe208 target/nrf52840dk_merged.hex 346016903ddf244a239162b7c703aafe7ec70a115175e2204892e874f930f6be third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle.bin -0c50decccd94e93612f3342ab833a73959d96a754aa6845b755a779a2240c484 target/nrf52840_dongle_merged.hex +c0467efe4a0536b5d835da7e000baccc06849c144907550a4ce6a6142fa60031 target/nrf52840_dongle_merged.hex adcc4caaea86f7b0d54111d3080802e7389a4e69a8f17945d026ee732ea8daa4 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle_dfu.bin -bbcbd0e72e40257cea94a2fee829d6b2c365e8473f2fb8b6ae685cda4d925f7d target/nrf52840_dongle_dfu_merged.hex +c74ff231e13e518652fae9f2dee13cd55492dd9348eadb42453a4f688bb399fd target/nrf52840_dongle_dfu_merged.hex 97a7dbdb7c3caa345307d5ff7f7607dad5c2cdc523b43c68d3b741ddce318e92 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_mdk_dfu.bin -d42f1ec737bd945ae2ddc74a5c179573b36fa720f9a7149b2af67a0e3667fac7 target/nrf52840_mdk_dfu_merged.hex -166523347144bcf9b936f5c265dfb2d7de9ab28569881276df344a6a42423f1b target/tab/ctap2.tab +b13a33e67a9858c829bdcb38bfd895e1960bdabf2ed365adeaf80a068e773f96 target/nrf52840_mdk_dfu_merged.hex +bcd1a523dc6d236ae8ac3924ae7887efa4c990da24fd445f2e03cad7522f0a1f target/tab/ctap2.tab diff --git a/reproducible/reference_binaries_ubuntu-18.04.sha256sum b/reproducible/reference_binaries_ubuntu-18.04.sha256sum index 3aca095..cc3241a 100644 --- a/reproducible/reference_binaries_ubuntu-18.04.sha256sum +++ b/reproducible/reference_binaries_ubuntu-18.04.sha256sum @@ -1,9 +1,9 @@ 921d6fc31f7235456dd41abc7e634a37ee87b5016b80c979d20ac5d3fcfc6b6b third_party/tock/target/thumbv7em-none-eabi/release/nrf52840dk.bin -776dab253e0903bffc54a9aeb09323717a4d69e6f1db8a4d88641378a89e47fe target/nrf52840dk_merged.hex +9f1fdb14c3f84a91be7c22ce59f3842f87e5154379832de7cd48a09bc6df9797 target/nrf52840dk_merged.hex aab5bdc406b1e874b83872c9358d310070b3ce948ec0e20c054fb923ec879249 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle.bin -945bfb77e7852d5d6425436208517c478af4ef66a675eded14f768de81512ab4 target/nrf52840_dongle_merged.hex +a607473c599a867ecbce8ed748c36c4b3731d69ad6c369765c14675057817149 target/nrf52840_dongle_merged.hex 26b8513e76058e86a01a4b408411ce429834eb2843993eb1671f2487b160bc9a third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle_dfu.bin -55efa672a5fce6f8757efe16a2c3444d11f8f5a575fc0fedbc2892b587c6ecac target/nrf52840_dongle_dfu_merged.hex +36fd56412e1dc72c0b61eea3a6de60343ccf8eeeb4bda664994f02e046ba375d target/nrf52840_dongle_dfu_merged.hex 7cc558a66505e8cf8170aab50e6ddcb28f349fd7ced35ce841ccec33a533bea1 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_mdk_dfu.bin -ed2beb9efd3bab6c91f7d0c6e3c1622d4920f5476844bd7551d51ba524ad4a71 target/nrf52840_mdk_dfu_merged.hex -ee12b35c75402db7fe191f2793209c51657c98662be2b6ff41ae09bccdc7e62a target/tab/ctap2.tab +5592d1617f6a647cd63ed19f398caa150b1f471c606f5b1afac1cffe73e3029a target/nrf52840_mdk_dfu_merged.hex +2c663f6c90f57af1c751085cb868ff905cdd1b71773cea10c31a898f201012b0 target/tab/ctap2.tab From 4e3162c475e5c889a6bf69f8ecc5193f1a896dfe Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Fri, 15 May 2020 19:43:37 +0200 Subject: [PATCH 8/9] Parse credentials by value --- src/ctap/data_formats.rs | 62 ++++++++++++++++++++++++++++------------ src/ctap/storage.rs | 5 ++-- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/ctap/data_formats.rs b/src/ctap/data_formats.rs index 63f74f1..d55121e 100644 --- a/src/ctap/data_formats.rs +++ b/src/ctap/data_formats.rs @@ -486,25 +486,28 @@ impl From for cbor::Value { } } -impl TryFrom<&cbor::Value> for PublicKeyCredentialSource { +impl TryFrom for PublicKeyCredentialSource { type Error = Ctap2StatusCode; - fn try_from(cbor_value: &cbor::Value) -> Result { + fn try_from(cbor_value: cbor::Value) -> Result { use PublicKeyCredentialSourceField::*; - let map = read_map(cbor_value)?; - let credential_id = read_byte_string(ok_or_missing(map.get(&CredentialId.into()))?)?; - let private_key = read_byte_string(ok_or_missing(map.get(&PrivateKey.into()))?)?; + let mut map = extract_map(cbor_value)?; + let credential_id = extract_byte_string(ok_or_missing(map.remove(&CredentialId.into()))?)?; + let private_key = extract_byte_string(ok_or_missing(map.remove(&PrivateKey.into()))?)?; if private_key.len() != 32 { return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR); } let private_key = ecdsa::SecKey::from_bytes(array_ref!(private_key, 0, 32)) .ok_or(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR)?; - let rp_id = read_text_string(ok_or_missing(map.get(&RpId.into()))?)?; - let user_handle = read_byte_string(ok_or_missing(map.get(&UserHandle.into()))?)?; - let other_ui = map.get(&OtherUi.into()).map(read_text_string).transpose()?; + let rp_id = extract_text_string(ok_or_missing(map.remove(&RpId.into()))?)?; + let user_handle = extract_byte_string(ok_or_missing(map.remove(&UserHandle.into()))?)?; + let other_ui = map + .remove(&OtherUi.into()) + .map(extract_text_string) + .transpose()?; let cred_random = map - .get(&CredRandom.into()) - .map(read_byte_string) + .remove(&CredRandom.into()) + .map(extract_byte_string) .transpose()?; // We don't return whether there were unknown fields in the CBOR value. This means that // deserialization is not injective. In particular deserialization is only an inverse of @@ -671,6 +674,13 @@ pub fn read_byte_string(cbor_value: &cbor::Value) -> Result, Ctap2Status } } +fn extract_byte_string(cbor_value: cbor::Value) -> Result, Ctap2StatusCode> { + match cbor_value { + cbor::Value::KeyValue(cbor::KeyType::ByteString(byte_string)) => Ok(byte_string), + _ => Err(Ctap2StatusCode::CTAP2_ERR_CBOR_UNEXPECTED_TYPE), + } +} + pub(super) fn read_text_string(cbor_value: &cbor::Value) -> Result { match cbor_value { cbor::Value::KeyValue(cbor::KeyType::TextString(text_string)) => { @@ -680,6 +690,13 @@ pub(super) fn read_text_string(cbor_value: &cbor::Value) -> Result Result { + match cbor_value { + cbor::Value::KeyValue(cbor::KeyType::TextString(text_string)) => Ok(text_string), + _ => Err(Ctap2StatusCode::CTAP2_ERR_CBOR_UNEXPECTED_TYPE), + } +} + pub(super) fn read_array(cbor_value: &cbor::Value) -> Result<&Vec, Ctap2StatusCode> { match cbor_value { cbor::Value::Array(array) => Ok(array), @@ -696,6 +713,15 @@ pub(super) fn read_map( } } +fn extract_map( + cbor_value: cbor::Value, +) -> Result, Ctap2StatusCode> { + match cbor_value { + cbor::Value::Map(map) => Ok(map), + _ => Err(Ctap2StatusCode::CTAP2_ERR_CBOR_UNEXPECTED_TYPE), + } +} + pub(super) fn read_bool(cbor_value: &cbor::Value) -> Result { match cbor_value { cbor::Value::Simple(cbor::SimpleValue::FalseValue) => Ok(false), @@ -704,9 +730,7 @@ pub(super) fn read_bool(cbor_value: &cbor::Value) -> Result, -) -> Result<&cbor::Value, Ctap2StatusCode> { +pub(super) fn ok_or_missing(value_option: Option) -> Result { value_option.ok_or(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER) } @@ -1240,7 +1264,7 @@ mod test { }; assert_eq!( - PublicKeyCredentialSource::try_from(&cbor::Value::from(credential.clone())), + PublicKeyCredentialSource::try_from(cbor::Value::from(credential.clone())), Ok(credential.clone()) ); @@ -1250,7 +1274,7 @@ mod test { }; assert_eq!( - PublicKeyCredentialSource::try_from(&cbor::Value::from(credential.clone())), + PublicKeyCredentialSource::try_from(cbor::Value::from(credential.clone())), Ok(credential.clone()) ); @@ -1260,15 +1284,15 @@ mod test { }; assert_eq!( - PublicKeyCredentialSource::try_from(&cbor::Value::from(credential.clone())), + PublicKeyCredentialSource::try_from(cbor::Value::from(credential.clone())), Ok(credential) ); } #[test] fn test_credential_source_invalid_cbor() { - assert!(PublicKeyCredentialSource::try_from(&cbor_false!()).is_err()); - assert!(PublicKeyCredentialSource::try_from(&cbor_array!(false)).is_err()); - assert!(PublicKeyCredentialSource::try_from(&cbor_array!(b"foo".to_vec())).is_err()); + assert!(PublicKeyCredentialSource::try_from(cbor_false!()).is_err()); + assert!(PublicKeyCredentialSource::try_from(cbor_array!(false)).is_err()); + assert!(PublicKeyCredentialSource::try_from(cbor_array!(b"foo".to_vec())).is_err()); } } diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 2a1a182..f0a2ca4 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -18,7 +18,7 @@ use crate::ctap::status_code::Ctap2StatusCode; use crate::ctap::PIN_AUTH_LENGTH; use alloc::string::String; use alloc::vec::Vec; -use core::convert::TryFrom; +use core::convert::TryInto; use ctap2::embedded_flash::{self, StoreConfig, StoreEntry, StoreError, StoreIndex}; #[cfg(any(test, feature = "ram_storage"))] @@ -420,7 +420,8 @@ impl From for Ctap2StatusCode { } fn deserialize_credential(data: &[u8]) -> Option { - PublicKeyCredentialSource::try_from(&cbor::read(data).ok()?).ok() + let cbor = cbor::read(data).ok()?; + cbor.try_into().ok() } fn serialize_credential(credential: PublicKeyCredentialSource) -> Result, Ctap2StatusCode> { From 0a5c5eafda44ad4551b4aacfbbe3c4dc3172c117 Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Fri, 15 May 2020 19:55:51 +0200 Subject: [PATCH 9/9] Update reproducible hashes --- reproducible/reference_binaries_macos-10.15.sha256sum | 10 +++++----- reproducible/reference_binaries_ubuntu-18.04.sha256sum | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/reproducible/reference_binaries_macos-10.15.sha256sum b/reproducible/reference_binaries_macos-10.15.sha256sum index 700519f..b4211e9 100644 --- a/reproducible/reference_binaries_macos-10.15.sha256sum +++ b/reproducible/reference_binaries_macos-10.15.sha256sum @@ -1,9 +1,9 @@ b113945b033eb229e3821542f5889769e5fd2e2ae3cb85c6d13a4e05a44a9866 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840dk.bin -9dfb62c7826994bf6e2bae972ea4dc03f1c62f4b49cea379c1f0aff9bbcfe208 target/nrf52840dk_merged.hex +11a43fdafe73f59a5c715d1c850b28b1e0572cedd6acc93c860a3a36f6a3d4ac target/nrf52840dk_merged.hex 346016903ddf244a239162b7c703aafe7ec70a115175e2204892e874f930f6be third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle.bin -c0467efe4a0536b5d835da7e000baccc06849c144907550a4ce6a6142fa60031 target/nrf52840_dongle_merged.hex +6a9ecb26886e266e3f92bc57ba8d0f6441ecfbfffdfe60ab981113b92524d3a3 target/nrf52840_dongle_merged.hex adcc4caaea86f7b0d54111d3080802e7389a4e69a8f17945d026ee732ea8daa4 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle_dfu.bin -c74ff231e13e518652fae9f2dee13cd55492dd9348eadb42453a4f688bb399fd target/nrf52840_dongle_dfu_merged.hex +55baecf731cf87966e1674fe6198aba2b955d33228192fb5b958e09b828aa3ea target/nrf52840_dongle_dfu_merged.hex 97a7dbdb7c3caa345307d5ff7f7607dad5c2cdc523b43c68d3b741ddce318e92 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_mdk_dfu.bin -b13a33e67a9858c829bdcb38bfd895e1960bdabf2ed365adeaf80a068e773f96 target/nrf52840_mdk_dfu_merged.hex -bcd1a523dc6d236ae8ac3924ae7887efa4c990da24fd445f2e03cad7522f0a1f target/tab/ctap2.tab +407b4bbc970901ce788ab3759571cf7e671f54a8c6a8546b7aff21ef96411df8 target/nrf52840_mdk_dfu_merged.hex +5947ae9da7436dc41fd9e89cb1faacb197282d03d0e755e2ac0099dc87484b4a target/tab/ctap2.tab diff --git a/reproducible/reference_binaries_ubuntu-18.04.sha256sum b/reproducible/reference_binaries_ubuntu-18.04.sha256sum index cc3241a..5e45b7f 100644 --- a/reproducible/reference_binaries_ubuntu-18.04.sha256sum +++ b/reproducible/reference_binaries_ubuntu-18.04.sha256sum @@ -1,9 +1,9 @@ 921d6fc31f7235456dd41abc7e634a37ee87b5016b80c979d20ac5d3fcfc6b6b third_party/tock/target/thumbv7em-none-eabi/release/nrf52840dk.bin -9f1fdb14c3f84a91be7c22ce59f3842f87e5154379832de7cd48a09bc6df9797 target/nrf52840dk_merged.hex +223072356135c3d1f2114c34a29dd498da4a958722167b425c844333b48f82ef target/nrf52840dk_merged.hex aab5bdc406b1e874b83872c9358d310070b3ce948ec0e20c054fb923ec879249 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle.bin -a607473c599a867ecbce8ed748c36c4b3731d69ad6c369765c14675057817149 target/nrf52840_dongle_merged.hex +98afad474b45a9ec8bddfcac6e61b149a25d5ed241e98bf1fa1174fa2efc91d2 target/nrf52840_dongle_merged.hex 26b8513e76058e86a01a4b408411ce429834eb2843993eb1671f2487b160bc9a third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_dongle_dfu.bin -36fd56412e1dc72c0b61eea3a6de60343ccf8eeeb4bda664994f02e046ba375d target/nrf52840_dongle_dfu_merged.hex +d164e7670a6a6e9b59e158d0f5357547cc205a8d34cd9e73b0232b57d4f9538f target/nrf52840_dongle_dfu_merged.hex 7cc558a66505e8cf8170aab50e6ddcb28f349fd7ced35ce841ccec33a533bea1 third_party/tock/target/thumbv7em-none-eabi/release/nrf52840_mdk_dfu.bin -5592d1617f6a647cd63ed19f398caa150b1f471c606f5b1afac1cffe73e3029a target/nrf52840_mdk_dfu_merged.hex -2c663f6c90f57af1c751085cb868ff905cdd1b71773cea10c31a898f201012b0 target/tab/ctap2.tab +b6bd82285985ddbe4201ee2e37cef960d7c663a41ca5f382c5770d8c675e1f6f target/nrf52840_mdk_dfu_merged.hex +e338e3b091ae73f41d4663f49f539e93d5a9b3585a8bf1a791577e9486828cc1 target/tab/ctap2.tab