From 04278d91d84fa4cedc0126ac2861fa3497f9f0d6 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Wed, 8 Jul 2020 16:17:15 +0200 Subject: [PATCH] adds code style improvements, including a new enum for permissions --- src/ctap/command.rs | 4 +- src/ctap/mod.rs | 15 +++- src/ctap/pin_protocol_v1.rs | 142 +++++++++++++++++++++++++++--------- src/ctap/response.rs | 7 +- src/ctap/storage.rs | 28 +++---- 5 files changed, 140 insertions(+), 56 deletions(-) diff --git a/src/ctap/command.rs b/src/ctap/command.rs index c541f1f..c3627f7 100644 --- a/src/ctap/command.rs +++ b/src/ctap/command.rs @@ -332,7 +332,9 @@ impl TryFrom for AuthenticatorClientPinParameters { let min_pin_length = min_pin_length .map(extract_unsigned) .transpose()? - .map(|m| m as u8); + .map(u8::try_from) + .transpose() + .map_err(|_| Ctap2StatusCode::CTAP2_ERR_PIN_POLICY_VIOLATION)?; #[cfg(feature = "with_ctap2_1")] let min_pin_length_rp_ids = match min_pin_length_rp_ids { Some(entry) => Some( diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 6fe9b7a..6df8ad2 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -38,6 +38,8 @@ use self::data_formats::{ PublicKeyCredentialUserEntity, SignatureAlgorithm, }; use self::hid::ChannelID; +#[cfg(feature = "with_ctap2_1")] +use self::pin_protocol_v1::PinPermission; use self::pin_protocol_v1::PinProtocolV1; use self::response::{ AuthenticatorGetAssertionResponse, AuthenticatorGetInfoResponse, @@ -414,8 +416,11 @@ where return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID); } #[cfg(feature = "with_ctap2_1")] - self.pin_protocol_v1 - .has_make_credential_permission(&rp_id)?; + { + self.pin_protocol_v1 + .has_permission(PinPermission::MakeCredential)?; + self.pin_protocol_v1.has_permission_for_rp_id(&rp_id)?; + } UP_FLAG | UV_FLAG | AT_FLAG | ed_flag } None => { @@ -595,7 +600,11 @@ where return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID); } #[cfg(feature = "with_ctap2_1")] - self.pin_protocol_v1.has_get_assertion_permission(&rp_id)?; + { + self.pin_protocol_v1 + .has_permission(PinPermission::GetAssertion)?; + self.pin_protocol_v1.has_permission_for_rp_id(&rp_id)?; + } UV_FLAG } None => { diff --git a/src/ctap/pin_protocol_v1.rs b/src/ctap/pin_protocol_v1.rs index 3f94980..32ca180 100644 --- a/src/ctap/pin_protocol_v1.rs +++ b/src/ctap/pin_protocol_v1.rs @@ -1,4 +1,4 @@ -// Copyright 2019 Google LLC +// Copyright 2020 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -63,9 +63,9 @@ fn encrypt_hmac_secret_output( let aes_enc_key = crypto::aes256::EncryptionKey::new(shared_secret); let aes_dec_key = crypto::aes256::DecryptionKey::new(&aes_enc_key); // The specification specifically asks for a zero IV. - let iv = [0; 16]; + let iv = [0u8; 16]; - let mut cred_random_secret = [0; 32]; + let mut cred_random_secret = [0u8; 32]; cred_random_secret.clone_from_slice(cred_random); // Initialization of 4 blocks in any case makes this function more readable. @@ -76,7 +76,7 @@ fn encrypt_hmac_secret_output( } cbc_decrypt(&aes_dec_key, iv, &mut blocks[..block_len]); - let mut decrypted_salt1 = [0; 32]; + let mut decrypted_salt1 = [0u8; 32]; decrypted_salt1[..16].clone_from_slice(&blocks[0]); let output1 = hmac_256::(&cred_random_secret, &decrypted_salt1[..]); decrypted_salt1[16..].clone_from_slice(&blocks[1]); @@ -85,7 +85,7 @@ fn encrypt_hmac_secret_output( } if block_len == 4 { - let mut decrypted_salt2 = [0; 32]; + let mut decrypted_salt2 = [0u8; 32]; decrypted_salt2[..16].clone_from_slice(&blocks[2]); decrypted_salt2[16..].clone_from_slice(&blocks[3]); let output2 = hmac_256::(&cred_random_secret, &decrypted_salt2[..]); @@ -110,7 +110,7 @@ fn check_and_store_new_pin( if new_pin_enc.len() != PIN_PADDED_LENGTH { return false; } - let iv = [0; 16]; + let iv = [0u8; 16]; // Assuming PIN_PADDED_LENGTH % block_size == 0 here. let mut blocks = [[0u8; 16]; PIN_PADDED_LENGTH / 16]; for i in 0..PIN_PADDED_LENGTH / 16 { @@ -136,12 +136,31 @@ fn check_and_store_new_pin( // TODO(kaczmarczyck) check last byte == 0x00 return false; } - let mut pin_hash = [0; 16]; + let mut pin_hash = [0u8; 16]; pin_hash.copy_from_slice(&Sha256::hash(&pin[..])[..16]); persistent_store.set_pin_hash(&pin_hash); true } +#[cfg(feature = "with_ctap2_1")] +// TODO remove when all variants are used +#[allow(dead_code)] +pub enum PinPermission { + MakeCredential = 0x01, + GetAssertion = 0x02, + CredentialManagement = 0x04, + BioEnrollment = 0x08, + PlatformConfiguration = 0x10, + AuthenticatorConfiguration = 0x20, +} + +#[cfg(feature = "with_ctap2_1")] +impl PinPermission { + pub fn check(self, stored_bits: u8) -> bool { + self as u8 & stored_bits != 0 + } +} + pub struct PinProtocolV1 { key_agreement_key: crypto::ecdh::SecKey, pin_uv_auth_token: [u8; PIN_TOKEN_LENGTH], @@ -187,7 +206,7 @@ impl PinProtocolV1 { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID); } - let iv = [0; 16]; + let iv = [0u8; 16]; let mut blocks = [[0u8; 16]; 1]; blocks[0].copy_from_slice(&pin_hash_enc[0..PIN_AUTH_LENGTH]); cbc_decrypt(aes_dec_key, iv, &mut blocks); @@ -309,7 +328,7 @@ impl PinProtocolV1 { self.check_pin_hash_enc(rng, persistent_store, &aes_dec_key, pin_hash_enc)?; // Assuming PIN_TOKEN_LENGTH % block_size == 0 here. - let iv = [0; 16]; + let iv = [0u8; 16]; let mut blocks = [[0u8; 16]; PIN_TOKEN_LENGTH / 16]; for (i, item) in blocks.iter_mut().take(PIN_TOKEN_LENGTH / 16).enumerate() { item.copy_from_slice(&self.pin_uv_auth_token[i * 16..(i + 1) * 16]); @@ -569,19 +588,16 @@ impl PinProtocolV1 { } #[cfg(feature = "with_ctap2_1")] - pub fn has_make_credential_permission(&mut self, rp_id: &str) -> Result<(), Ctap2StatusCode> { - self.has_permission(0x01, rp_id) + pub fn has_permission(&self, permission: PinPermission) -> Result<(), Ctap2StatusCode> { + if permission.check(self.permissions) { + Ok(()) + } else { + Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + } } #[cfg(feature = "with_ctap2_1")] - pub fn has_get_assertion_permission(&mut self, rp_id: &str) -> Result<(), Ctap2StatusCode> { - self.has_permission(0x02, rp_id) - } - - // TODO(kaczmarczyck) use permissons for new commands - - #[cfg(feature = "with_ctap2_1")] - fn has_permission(&mut self, bitmask: u8, rp_id: &str) -> Result<(), Ctap2StatusCode> { + pub fn has_permission_for_rp_id(&mut self, rp_id: &str) -> Result<(), Ctap2StatusCode> { if let Some(permissions_rp_id) = &self.permissions_rp_id { if rp_id != permissions_rp_id { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID); @@ -589,11 +605,7 @@ impl PinProtocolV1 { } else { self.permissions_rp_id = Some(String::from(rp_id)); } - if self.permissions & bitmask != 0 { - Ok(()) - } else { - Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) - } + Ok(()) } } @@ -603,12 +615,12 @@ mod test { use crypto::rng256::ThreadRng256; fn set_standard_pin(persistent_store: &mut PersistentStore) { - let mut pin = [0x00; 64]; + let mut pin = [0u8; 64]; pin[0] = 0x31; pin[1] = 0x32; pin[2] = 0x33; pin[3] = 0x34; - let mut pin_hash = [0; 16]; + let mut pin_hash = [0u8; 16]; pin_hash.copy_from_slice(&Sha256::hash(&pin[..])[..16]); persistent_store.set_pin_hash(&pin_hash); } @@ -620,7 +632,7 @@ mod test { blocks[0][1] = 0x32; blocks[0][2] = 0x33; blocks[0][3] = 0x34; - let iv = [0; 16]; + let iv = [0u8; 16]; cbc_encrypt(&aes_enc_key, iv, &mut blocks); let mut encrypted_pin = Vec::with_capacity(64); @@ -632,7 +644,7 @@ mod test { fn encrypt_standard_pin_hash(shared_secret: &[u8; 32]) -> Vec { let aes_enc_key = crypto::aes256::EncryptionKey::new(shared_secret); - let mut pin = [0x00; 64]; + let mut pin = [0u8; 64]; pin[0] = 0x31; pin[1] = 0x32; pin[2] = 0x33; @@ -641,7 +653,7 @@ mod test { let mut blocks = [[0u8; 16]; 1]; blocks[0].copy_from_slice(&pin_hash[..16]); - let iv = [0; 16]; + let iv = [0u8; 16]; cbc_encrypt(&aes_enc_key, iv, &mut blocks); let mut encrypted_pin_hash = Vec::with_capacity(16); @@ -699,7 +711,7 @@ mod test { &mut rng, &mut persistent_store, &aes_dec_key, - pin_hash_enc.clone() + pin_hash_enc ), Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_BLOCKED) ); @@ -1089,19 +1101,77 @@ mod test { fn test_has_permission() { let mut rng = ThreadRng256 {}; let mut pin_protocol_v1 = PinProtocolV1::new(&mut rng); - pin_protocol_v1.permissions = 0x03; - assert_eq!(pin_protocol_v1.has_permission(0x01, "example.com"), Ok(())); + pin_protocol_v1.permissions = 0x7F; + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::MakeCredential), + Ok(()) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::GetAssertion), + Ok(()) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::CredentialManagement), + Ok(()) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::BioEnrollment), + Ok(()) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::PlatformConfiguration), + Ok(()) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::AuthenticatorConfiguration), + Ok(()) + ); + pin_protocol_v1.permissions = 0x00; + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::MakeCredential), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::GetAssertion), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::CredentialManagement), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::BioEnrollment), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::PlatformConfiguration), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + ); + assert_eq!( + pin_protocol_v1.has_permission(PinPermission::AuthenticatorConfiguration), + Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + ); + } + + #[cfg(feature = "with_ctap2_1")] + #[test] + fn test_has_permission_for_rp_id() { + let mut rng = ThreadRng256 {}; + let mut pin_protocol_v1 = PinProtocolV1::new(&mut rng); + assert_eq!( + pin_protocol_v1.has_permission_for_rp_id("example.com"), + Ok(()) + ); assert_eq!( pin_protocol_v1.permissions_rp_id, Some(String::from("example.com")) ); - assert_eq!(pin_protocol_v1.has_permission(0x01, "example.com"), Ok(())); assert_eq!( - pin_protocol_v1.has_permission(0x01, "counter-example.com"), - Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) + pin_protocol_v1.has_permission_for_rp_id("example.com"), + Ok(()) ); assert_eq!( - pin_protocol_v1.has_permission(0x04, "example.com"), + pin_protocol_v1.has_permission_for_rp_id("counter-example.com"), Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID) ); } diff --git a/src/ctap/response.rs b/src/ctap/response.rs index bbd591d..3f16a75 100644 --- a/src/ctap/response.rs +++ b/src/ctap/response.rs @@ -288,8 +288,9 @@ mod test { #[test] fn test_get_info_into_cbor() { + let versions = vec!["FIDO_2_0".to_string()]; let get_info_response = AuthenticatorGetInfoResponse { - versions: vec!["FIDO_2_0".to_string()], + versions: versions.clone(), extensions: None, aaguid: [0x00; 16], options: None, @@ -313,12 +314,12 @@ mod test { ResponseData::AuthenticatorGetInfo(get_info_response).into(); #[cfg(not(feature = "with_ctap2_1"))] let expected_cbor = cbor_map_options! { - 0x01 => cbor_array_vec![vec!["FIDO_2_0"]], + 0x01 => cbor_array_vec![versions], 0x03 => vec![0x00; 16], }; #[cfg(feature = "with_ctap2_1")] let expected_cbor = cbor_map_options! { - 0x01 => cbor_array_vec![vec!["FIDO_2_0"]], + 0x01 => cbor_array_vec![versions], 0x03 => vec![0x00; 16], 0x0D => 4, }; diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index fffd80f..415defd 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -66,7 +66,8 @@ const AAGUID: usize = 7; const MIN_PIN_LENGTH: usize = 8; #[cfg(feature = "with_ctap2_1")] const MIN_PIN_LENGTH_RP_IDS: usize = 9; -// Different NUM_TAGS make the storage incompatible, so we use max(8,10). +// Different NUM_TAGS depending on the CTAP version make the storage incompatible, +// so we use the maximum. const NUM_TAGS: usize = 10; const MAX_PIN_RETRIES: u8 = 6; @@ -417,19 +418,20 @@ impl PersistentStore { } Some((index, entry)) => { debug_assert_eq!(entry.data.len(), 1); - if entry.data[0] > 0 { - let new_value = entry.data[0].saturating_sub(1); - self.store - .replace( - index, - StoreEntry { - tag: PIN_RETRIES, - data: &[new_value], - sensitive: false, - }, - ) - .unwrap(); + if entry.data[0] == 0 { + return; } + let new_value = entry.data[0].saturating_sub(1); + self.store + .replace( + index, + StoreEntry { + tag: PIN_RETRIES, + data: &[new_value], + sensitive: false, + }, + ) + .unwrap(); } } }