From de3addba74530ca34f8eb5d26e12c8a0c7c2184a Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Thu, 21 Jan 2021 17:38:22 +0100 Subject: [PATCH 1/4] force PIN changes --- src/ctap/config_command.rs | 56 +++++++++++++++++++++++++++++++++++++ src/ctap/mod.rs | 18 ++++++++---- src/ctap/pin_protocol_v1.rs | 49 ++++++++++++++++++++++++++++++++ src/ctap/storage.rs | 29 +++++++++++++++++-- src/ctap/storage/key.rs | 3 ++ 5 files changed, 146 insertions(+), 9 deletions(-) diff --git a/src/ctap/config_command.rs b/src/ctap/config_command.rs index 726634c..e96ce7a 100644 --- a/src/ctap/config_command.rs +++ b/src/ctap/config_command.rs @@ -248,6 +248,62 @@ mod test { ); } + #[test] + fn test_process_set_min_pin_length_force_pin_change_implicit() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + let key_agreement_key = crypto::ecdh::SecKey::gensk(&mut rng); + let pin_uv_auth_token = [0x55; 32]; + let mut pin_protocol_v1 = PinProtocolV1::new_test(key_agreement_key, pin_uv_auth_token); + + persistent_store.set_pin(&[0x88; 16], 4).unwrap(); + // Increase min PIN, force PIN change. + let min_pin_length = 6; + let mut config_params = create_min_pin_config_params(min_pin_length, None); + let pin_uv_auth_param = Some(vec![ + 0x81, 0x37, 0x37, 0xF3, 0xD8, 0x69, 0xBD, 0x74, 0xFE, 0x88, 0x30, 0x8C, 0xC4, 0x2E, + 0xA8, 0xC8, + ]); + config_params.pin_uv_auth_param = pin_uv_auth_param; + let config_response = + process_config(&mut persistent_store, &mut pin_protocol_v1, config_params); + assert_eq!(config_response, Ok(ResponseData::AuthenticatorConfig)); + assert_eq!(persistent_store.min_pin_length(), Ok(min_pin_length)); + assert_eq!(persistent_store.has_force_pin_change(), Ok(true)); + } + + #[test] + fn test_process_set_min_pin_length_force_pin_change_explicit() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + let key_agreement_key = crypto::ecdh::SecKey::gensk(&mut rng); + let pin_uv_auth_token = [0x55; 32]; + let mut pin_protocol_v1 = PinProtocolV1::new_test(key_agreement_key, pin_uv_auth_token); + + persistent_store.set_pin(&[0x88; 16], 4).unwrap(); + let pin_uv_auth_param = Some(vec![ + 0xE3, 0x74, 0xF4, 0x27, 0xBE, 0x7D, 0x40, 0xB5, 0x71, 0xB6, 0xB4, 0x1A, 0xD2, 0xC1, + 0x53, 0xD7, + ]); + let set_min_pin_length_params = SetMinPinLengthParams { + new_min_pin_length: Some(persistent_store.min_pin_length().unwrap()), + min_pin_length_rp_ids: None, + force_change_pin: Some(true), + }; + let config_params = AuthenticatorConfigParameters { + sub_command: ConfigSubCommand::SetMinPinLength, + sub_command_params: Some(ConfigSubCommandParams::SetMinPinLength( + set_min_pin_length_params, + )), + pin_uv_auth_param, + pin_uv_auth_protocol: Some(1), + }; + let config_response = + process_config(&mut persistent_store, &mut pin_protocol_v1, config_params); + assert_eq!(config_response, Ok(ResponseData::AuthenticatorConfig)); + assert_eq!(persistent_store.has_force_pin_change(), Ok(true)); + } + #[test] fn test_process_config_vendor_prototype() { let mut rng = ThreadRng256 {}; diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 6ffd108..8a4479a 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -1006,6 +1006,10 @@ where ); options_map.insert(String::from("credMgmt"), true); options_map.insert(String::from("setMinPINLength"), true); + options_map.insert( + String::from("forcePINChange"), + self.persistent_store.has_force_pin_change()?, + ); Ok(ResponseData::AuthenticatorGetInfo( AuthenticatorGetInfoResponse { versions: vec![ @@ -1256,13 +1260,15 @@ mod test { expected_response.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_response.extend( [ - 0x04, 0xA5, 0x62, 0x72, 0x6B, 0xF5, 0x62, 0x75, 0x70, 0xF5, 0x68, 0x63, 0x72, 0x65, + 0x04, 0xA6, 0x62, 0x72, 0x6B, 0xF5, 0x62, 0x75, 0x70, 0xF5, 0x68, 0x63, 0x72, 0x65, 0x64, 0x4D, 0x67, 0x6D, 0x74, 0xF5, 0x69, 0x63, 0x6C, 0x69, 0x65, 0x6E, 0x74, 0x50, - 0x69, 0x6E, 0xF4, 0x6F, 0x73, 0x65, 0x74, 0x4D, 0x69, 0x6E, 0x50, 0x49, 0x4E, 0x4C, - 0x65, 0x6E, 0x67, 0x74, 0x68, 0xF5, 0x05, 0x19, 0x04, 0x00, 0x06, 0x81, 0x01, 0x08, - 0x18, 0x70, 0x09, 0x81, 0x63, 0x75, 0x73, 0x62, 0x0A, 0x81, 0xA2, 0x63, 0x61, 0x6C, - 0x67, 0x26, 0x64, 0x74, 0x79, 0x70, 0x65, 0x6A, 0x70, 0x75, 0x62, 0x6C, 0x69, 0x63, - 0x2D, 0x6B, 0x65, 0x79, 0x0D, 0x04, 0x0F, 0x18, 0x20, 0x10, 0x08, 0x14, 0x18, 0x96, + 0x69, 0x6E, 0xF4, 0x6E, 0x66, 0x6F, 0x72, 0x63, 0x65, 0x50, 0x49, 0x4E, 0x43, 0x68, + 0x61, 0x6E, 0x67, 0x65, 0xF4, 0x6F, 0x73, 0x65, 0x74, 0x4D, 0x69, 0x6E, 0x50, 0x49, + 0x4E, 0x4C, 0x65, 0x6E, 0x67, 0x74, 0x68, 0xF5, 0x05, 0x19, 0x04, 0x00, 0x06, 0x81, + 0x01, 0x08, 0x18, 0x70, 0x09, 0x81, 0x63, 0x75, 0x73, 0x62, 0x0A, 0x81, 0xA2, 0x63, + 0x61, 0x6C, 0x67, 0x26, 0x64, 0x74, 0x79, 0x70, 0x65, 0x6A, 0x70, 0x75, 0x62, 0x6C, + 0x69, 0x63, 0x2D, 0x6B, 0x65, 0x79, 0x0D, 0x04, 0x0F, 0x18, 0x20, 0x10, 0x08, 0x14, + 0x18, 0x96, ] .iter(), ); diff --git a/src/ctap/pin_protocol_v1.rs b/src/ctap/pin_protocol_v1.rs index 0e46573..ae1b1df 100644 --- a/src/ctap/pin_protocol_v1.rs +++ b/src/ctap/pin_protocol_v1.rs @@ -328,6 +328,9 @@ impl PinProtocolV1 { let token_encryption_key = crypto::aes256::EncryptionKey::new(&shared_secret); let pin_decryption_key = crypto::aes256::DecryptionKey::new(&token_encryption_key); self.verify_pin_hash_enc(rng, persistent_store, &pin_decryption_key, pin_hash_enc)?; + if persistent_store.has_force_pin_change()? { + return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID); + } // Assuming PIN_TOKEN_LENGTH % block_size == 0 here. let iv = [0u8; 16]; @@ -821,6 +824,28 @@ mod test { ); } + #[test] + fn test_process_get_pin_token_force_pin_change() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + set_standard_pin(&mut persistent_store); + assert_eq!(persistent_store.force_pin_change(), Ok(())); + let mut pin_protocol_v1 = PinProtocolV1::new(&mut rng); + let pk = pin_protocol_v1.key_agreement_key.genpk(); + let shared_secret = pin_protocol_v1.key_agreement_key.exchange_x_sha256(&pk); + let key_agreement = CoseKey::from(pk); + let pin_hash_enc = encrypt_standard_pin_hash(&shared_secret); + assert_eq!( + pin_protocol_v1.process_get_pin_token( + &mut rng, + &mut persistent_store, + key_agreement, + pin_hash_enc + ), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID), + ); + } + #[test] fn test_process_get_pin_uv_auth_token_using_pin_with_permissions() { let mut rng = ThreadRng256 {}; @@ -885,6 +910,30 @@ mod test { ); } + #[test] + fn test_process_get_pin_token_force_pin_change_force_pin_change() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + set_standard_pin(&mut persistent_store); + assert_eq!(persistent_store.force_pin_change(), Ok(())); + let mut pin_protocol_v1 = PinProtocolV1::new(&mut rng); + let pk = pin_protocol_v1.key_agreement_key.genpk(); + let shared_secret = pin_protocol_v1.key_agreement_key.exchange_x_sha256(&pk); + let key_agreement = CoseKey::from(pk); + let pin_hash_enc = encrypt_standard_pin_hash(&shared_secret); + assert_eq!( + pin_protocol_v1.process_get_pin_uv_auth_token_using_pin_with_permissions( + &mut rng, + &mut persistent_store, + key_agreement, + pin_hash_enc, + 0x03, + Some(String::from("example.com")), + ), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID), + ); + } + #[test] fn test_process() { let mut rng = ThreadRng256 {}; diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index ffc5dd6..74c11a6 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -384,7 +384,9 @@ impl PersistentStore { let mut pin_properties = [0; 1 + PIN_AUTH_LENGTH]; pin_properties[0] = pin_code_point_length; pin_properties[1..].clone_from_slice(pin_hash); - Ok(self.store.insert(key::PIN_PROPERTIES, &pin_properties)?) + self.store.insert(key::PIN_PROPERTIES, &pin_properties)?; + // If this second transaction fails, you are forced to retry. + Ok(self.store.remove(key::FORCE_PIN_CHANGE)?) } /// Returns the number of remaining PIN retries. @@ -541,9 +543,18 @@ impl PersistentStore { Ok(()) } + /// Returns whether the PIN needs to be changed before its next usage. + pub fn has_force_pin_change(&self) -> Result { + match self.store.find(key::FORCE_PIN_CHANGE)? { + None => Ok(false), + Some(value) if value.len() == 1 && value[0] == 1 => Ok(true), + _ => Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR), + } + } + + /// Marks the PIN as outdated with respect to the new PIN policy. pub fn force_pin_change(&mut self) -> Result<(), Ctap2StatusCode> { - // TODO(kaczmarczyck) implement storage logic - Ok(()) + Ok(self.store.insert(key::FORCE_PIN_CHANGE, &[1])?) } } @@ -1148,6 +1159,18 @@ mod test { } } + #[test] + fn test_force_pin_change() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + + assert!(!persistent_store.has_force_pin_change().unwrap()); + assert_eq!(persistent_store.force_pin_change(), Ok(())); + assert!(persistent_store.has_force_pin_change().unwrap()); + assert_eq!(persistent_store.set_pin(&[0x88; 16], 8), Ok(())); + assert!(!persistent_store.has_force_pin_change().unwrap()); + } + #[test] fn test_serialize_deserialize_credential() { let mut rng = ThreadRng256 {}; diff --git a/src/ctap/storage/key.rs b/src/ctap/storage/key.rs index c6e46e2..1c0e21e 100644 --- a/src/ctap/storage/key.rs +++ b/src/ctap/storage/key.rs @@ -88,6 +88,9 @@ make_partition! { /// board may configure `MAX_SUPPORTED_RESIDENT_KEYS` depending on the storage size. CREDENTIALS = 1700..2000; + /// If this entry exists and equals 1, the PIN needs to be changed. + FORCE_PIN_CHANGE = 2040; + /// The secret of the CredRandom feature. CRED_RANDOM_SECRET = 2041; From 3408c0a2edf7f6f880803c9d844e779c954a5322 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Thu, 21 Jan 2021 18:24:25 +0100 Subject: [PATCH 2/4] makes test_get_info more readable --- src/ctap/mod.rs | 75 +++++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 8a4479a..6f10589 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -1181,7 +1181,7 @@ mod test { MakeCredentialOptions, PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, }; use super::*; - use cbor::{cbor_array, cbor_map}; + use cbor::{cbor_array, cbor_array_vec, cbor_map}; use crypto::rng256::ThreadRng256; const CLOCK_FREQUENCY_HZ: usize = 32768; @@ -1237,43 +1237,44 @@ mod test { let mut ctap_state = CtapState::new(&mut rng, user_immediately_present, DUMMY_CLOCK_VALUE); let info_reponse = ctap_state.process_command(&[0x04], DUMMY_CHANNEL_ID, DUMMY_CLOCK_VALUE); - let mut expected_response = vec![0x00, 0xAD, 0x01]; - // The version array differs with CTAP1, always including 2.0 and 2.1. - #[cfg(not(feature = "with_ctap1"))] - let version_count = 2; - #[cfg(feature = "with_ctap1")] - let version_count = 3; - expected_response.push(0x80 + version_count); - #[cfg(feature = "with_ctap1")] - expected_response.extend(&[0x66, 0x55, 0x32, 0x46, 0x5F, 0x56, 0x32]); - expected_response.extend( - [ - 0x68, 0x46, 0x49, 0x44, 0x4F, 0x5F, 0x32, 0x5F, 0x30, 0x6C, 0x46, 0x49, 0x44, 0x4F, - 0x5F, 0x32, 0x5F, 0x31, 0x5F, 0x50, 0x52, 0x45, 0x02, 0x84, 0x6B, 0x68, 0x6D, 0x61, - 0x63, 0x2D, 0x73, 0x65, 0x63, 0x72, 0x65, 0x74, 0x6B, 0x63, 0x72, 0x65, 0x64, 0x50, - 0x72, 0x6F, 0x74, 0x65, 0x63, 0x74, 0x6C, 0x6D, 0x69, 0x6E, 0x50, 0x69, 0x6E, 0x4C, - 0x65, 0x6E, 0x67, 0x74, 0x68, 0x68, 0x63, 0x72, 0x65, 0x64, 0x42, 0x6C, 0x6F, 0x62, - 0x03, 0x50, - ] - .iter(), - ); - expected_response.extend(&ctap_state.persistent_store.aaguid().unwrap()); - expected_response.extend( - [ - 0x04, 0xA6, 0x62, 0x72, 0x6B, 0xF5, 0x62, 0x75, 0x70, 0xF5, 0x68, 0x63, 0x72, 0x65, - 0x64, 0x4D, 0x67, 0x6D, 0x74, 0xF5, 0x69, 0x63, 0x6C, 0x69, 0x65, 0x6E, 0x74, 0x50, - 0x69, 0x6E, 0xF4, 0x6E, 0x66, 0x6F, 0x72, 0x63, 0x65, 0x50, 0x49, 0x4E, 0x43, 0x68, - 0x61, 0x6E, 0x67, 0x65, 0xF4, 0x6F, 0x73, 0x65, 0x74, 0x4D, 0x69, 0x6E, 0x50, 0x49, - 0x4E, 0x4C, 0x65, 0x6E, 0x67, 0x74, 0x68, 0xF5, 0x05, 0x19, 0x04, 0x00, 0x06, 0x81, - 0x01, 0x08, 0x18, 0x70, 0x09, 0x81, 0x63, 0x75, 0x73, 0x62, 0x0A, 0x81, 0xA2, 0x63, - 0x61, 0x6C, 0x67, 0x26, 0x64, 0x74, 0x79, 0x70, 0x65, 0x6A, 0x70, 0x75, 0x62, 0x6C, - 0x69, 0x63, 0x2D, 0x6B, 0x65, 0x79, 0x0D, 0x04, 0x0F, 0x18, 0x20, 0x10, 0x08, 0x14, - 0x18, 0x96, - ] - .iter(), - ); + let expected_cbor = cbor_map_options! { + 0x01 => cbor_array_vec![vec![ + #[cfg(feature = "with_ctap1")] + String::from(U2F_VERSION_STRING), + String::from(FIDO2_VERSION_STRING), + String::from(FIDO2_1_VERSION_STRING), + ]], + 0x02 => cbor_array_vec![vec![ + String::from("hmac-secret"), + String::from("credProtect"), + String::from("minPinLength"), + String::from("credBlob"), + ]], + 0x03 => ctap_state.persistent_store.aaguid().unwrap(), + 0x04 => cbor_map! { + "rk" => true, + "up" => true, + "clientPin" => false, + "credMgmt" => true, + "setMinPINLength" => true, + "forcePINChange" => false, + }, + 0x05 => 1024, + 0x06 => cbor_array_vec![vec![1]], + 0x07 => MAX_CREDENTIAL_COUNT_IN_LIST.map(|c| c as u64), + 0x08 => CREDENTIAL_ID_SIZE as u64, + 0x09 => cbor_array_vec![vec!["usb"]], + 0x0A => cbor_array_vec![vec![ES256_CRED_PARAM]], + 0x0C => DEFAULT_CRED_PROTECT.map(|c| c as u64), + 0x0D => ctap_state.persistent_store.min_pin_length().unwrap() as u64, + 0x0F => MAX_CRED_BLOB_LENGTH as u64, + 0x10 => MAX_RP_IDS_LENGTH as u64, + 0x14 => ctap_state.persistent_store.remaining_credentials().unwrap() as u64, + }; - assert_eq!(info_reponse, expected_response); + let mut response_cbor = vec![0x00]; + assert!(cbor::write(expected_cbor, &mut response_cbor)); + assert_eq!(info_reponse, response_cbor); } fn create_minimal_make_credential_parameters() -> AuthenticatorMakeCredentialParameters { From 5fe111698bb6993cd72255075bb6d407c45a837d Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Thu, 21 Jan 2021 18:47:00 +0100 Subject: [PATCH 3/4] remove resolved TODO --- src/ctap/config_command.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ctap/config_command.rs b/src/ctap/config_command.rs index e96ce7a..5e4daf3 100644 --- a/src/ctap/config_command.rs +++ b/src/ctap/config_command.rs @@ -44,7 +44,6 @@ fn process_set_min_pin_length( force_change_pin |= new_min_pin_length > old_length; } if force_change_pin { - // TODO(kaczmarczyck) actually force a PIN change in PinProtocolV1 persistent_store.force_pin_change()?; } persistent_store.set_min_pin_length(new_min_pin_length)?; From c38f00624a1a315d5533952c4be45faf8f1943c5 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Fri, 22 Jan 2021 10:55:11 +0100 Subject: [PATCH 4/4] use transactions, and how to store a bool --- src/ctap/pin_protocol_v1.rs | 1 + src/ctap/storage.rs | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/ctap/pin_protocol_v1.rs b/src/ctap/pin_protocol_v1.rs index ae1b1df..eb537f0 100644 --- a/src/ctap/pin_protocol_v1.rs +++ b/src/ctap/pin_protocol_v1.rs @@ -328,6 +328,7 @@ impl PinProtocolV1 { let token_encryption_key = crypto::aes256::EncryptionKey::new(&shared_secret); let pin_decryption_key = crypto::aes256::DecryptionKey::new(&token_encryption_key); self.verify_pin_hash_enc(rng, persistent_store, &pin_decryption_key, pin_hash_enc)?; + // TODO(kaczmarczyck) can this be moved up in the specification? if persistent_store.has_force_pin_change()? { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID); } diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 74c11a6..b85f918 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -30,6 +30,7 @@ use arrayref::array_ref; use cbor::cbor_array_vec; use core::convert::TryInto; use crypto::rng256::Rng256; +use persistent_store::StoreUpdate; // Those constants may be modified before compilation to tune the behavior of the key. // @@ -384,9 +385,15 @@ impl PersistentStore { let mut pin_properties = [0; 1 + PIN_AUTH_LENGTH]; pin_properties[0] = pin_code_point_length; pin_properties[1..].clone_from_slice(pin_hash); - self.store.insert(key::PIN_PROPERTIES, &pin_properties)?; - // If this second transaction fails, you are forced to retry. - Ok(self.store.remove(key::FORCE_PIN_CHANGE)?) + Ok(self.store.transaction(&[ + StoreUpdate::Insert { + key: key::PIN_PROPERTIES, + value: &pin_properties[..], + }, + StoreUpdate::Remove { + key: key::FORCE_PIN_CHANGE, + }, + ])?) } /// Returns the number of remaining PIN retries. @@ -547,14 +554,14 @@ impl PersistentStore { pub fn has_force_pin_change(&self) -> Result { match self.store.find(key::FORCE_PIN_CHANGE)? { None => Ok(false), - Some(value) if value.len() == 1 && value[0] == 1 => Ok(true), + Some(value) if value.is_empty() => Ok(true), _ => Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR), } } /// Marks the PIN as outdated with respect to the new PIN policy. pub fn force_pin_change(&mut self) -> Result<(), Ctap2StatusCode> { - Ok(self.store.insert(key::FORCE_PIN_CHANGE, &[1])?) + Ok(self.store.insert(key::FORCE_PIN_CHANGE, &[])?) } }