diff --git a/libraries/opensk/src/api/key_store.rs b/libraries/opensk/src/api/key_store.rs index 6df28700..67e1aaa6 100644 --- a/libraries/opensk/src/api/key_store.rs +++ b/libraries/opensk/src/api/key_store.rs @@ -19,6 +19,7 @@ use crate::api::persist::Persist; use crate::ctap::crypto_wrapper::{aes256_cbc_decrypt, aes256_cbc_encrypt}; use crate::ctap::data_formats::CredentialProtectionPolicy; use crate::ctap::secret::Secret; +use crate::ctap::status_code::{Ctap2StatusCode, CtapResult}; use crate::ctap::{cbor_read, cbor_write}; use crate::env::{AesKey, Env, Hmac}; use alloc::vec::Vec; @@ -98,28 +99,57 @@ pub trait KeyStore { /// Returns the key for the CredRandom feature. fn cred_random(&mut self, has_uv: bool) -> Result, Error>; - /// Encrypts a PIN hash. - fn encrypt_pin_hash(&mut self, plain: &[u8; 16]) -> Result<[u8; 16], Error>; + /// Returns whether a PIN is configured. + #[cfg(not(feature = "config_command"))] + fn has_pin(&mut self) -> CtapResult; - /// Checks whether a candidate PIN hash matches the stored PIN hash. + /// Returns whether a PIN is configured. + #[cfg(feature = "config_command")] + fn has_pin(&mut self) -> CtapResult { + self.is_pin_longer_than(0) + } + + /// Returns whether the configured PIN is known to have more than `length` + /// Unicode code points. Returns false if no PIN is configured. /// - /// `stored` is the output of [`Self::encrypt_pin_hash`], read back via - /// [`Persist::pin_hash`]. `candidate` is the first 16 bytes of SHA-256 of - /// the PIN being checked. + /// The default [`Self::has_pin`] calls this with zero, which must accurately + /// report PIN presence without verifying the PIN. /// - /// Implementations that cannot decrypt `stored` may treat it as an opaque - /// marker and delegate verification to secure hardware without exposing - /// the decrypted PIN hash. + /// OpenSK only calls this with a nonzero length when a client sets the + /// minimum PIN length through `config_command`. Returning false then sets + /// `forcePINChange`, requiring the user to change their PIN. /// - /// This is also used to reject reuse of the old PIN during a forced PIN - /// change. Errors must remain distinguishable from a mismatch. Implementations - /// should avoid leaking information through timing or error details. - fn verify_pin_hash(&mut self, stored: &[u8; 16], candidate: &[u8; 16]) -> Result { - let plain = Secret::from_exposed_secret(*stored); - Ok(bool::from(plain.ct_eq(candidate))) - } + /// Backends need not retain the exact length: a guaranteed lower bound is + /// sufficient to answer true. If the length is unknown, answer true at zero + /// when a PIN exists, and false for positive thresholds. This conservatively + /// forces a PIN change on policy updates even if the PIN was already long + /// enough. Disable `config_command` if these unnecessary changes are unwanted. + /// Device or storage failures must return errors, not report an absent PIN. + #[cfg(feature = "config_command")] + fn is_pin_longer_than(&mut self, length: u8) -> CtapResult; + + /// Checks the first 16 bytes of SHA-256 of the candidate PIN. + /// + /// Return false for an incorrect PIN and an error for backend failures. + /// Software implementations must compare hashes in constant time. CTAP charges + /// a retry before this call and resets retries after successful verification. + /// Return `CTAP2_ERR_PIN_AUTH_BLOCKED` if temporary backend lockout prevented + /// checking the candidate. OpenSK then refunds the retry. An attempted but + /// incorrect PIN must return false, even if it triggers hardware lockout. + fn verify_pin_hash(&mut self, candidate: &[u8; 16]) -> CtapResult; + + /// Sets or replaces the PIN using its hash and length in Unicode code points. + /// + /// The length can inform `is_pin_longer_than` with `config_command` enabled; + /// retaining it is optional. + /// + /// CTAP checks PIN policy and, for replacement, verifies the old PIN first. + /// Backends may retain authorization from that verification for this call; + /// any retained authorization must be scoped to the current command. + /// On success, the PIN must be durable. CTAP clears the forced-change flag. + fn store_pin(&mut self, pin_hash: &[u8; 16], code_point_length: u8) -> CtapResult<()>; - /// Resets the key store. + /// Resets the key store, including any hardware PIN configuration. fn reset(&mut self) -> Result<(), Error>; } @@ -222,8 +252,28 @@ impl KeyStore for T { Ok(get_master_keys(self)?.cred_random[has_uv as usize].clone()) } - fn encrypt_pin_hash(&mut self, plain: &[u8; 16]) -> Result<[u8; 16], Error> { - Ok(*plain) + fn has_pin(&mut self) -> CtapResult { + Ok(self.persist().pin_hash()?.is_some()) + } + + #[cfg(feature = "config_command")] + fn is_pin_longer_than(&mut self, length: u8) -> CtapResult { + Ok(self + .persist() + .pin_code_point_length()? + .is_some_and(|pin_length| pin_length > length)) + } + + fn verify_pin_hash(&mut self, candidate: &[u8; 16]) -> CtapResult { + let stored = self + .persist() + .pin_hash()? + .ok_or(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED)?; + Ok(bool::from(stored.ct_eq(candidate))) + } + + fn store_pin(&mut self, pin_hash: &[u8; 16], code_point_length: u8) -> CtapResult<()> { + self.persist().set_pin(pin_hash, code_point_length) } fn reset(&mut self) -> Result<(), Error> { @@ -399,16 +449,31 @@ mod test { } #[test] - fn test_default_verify_pin_hash() { + fn test_pin_storage_and_verification() { let mut env = TestEnv::default(); let key_store = env.key_store(); - let stored = key_store.encrypt_pin_hash(&[1; 16]).unwrap(); - assert_eq!(key_store.verify_pin_hash(&stored, &[1; 16]), Ok(true)); + assert_eq!(key_store.has_pin(), Ok(false)); + #[cfg(feature = "config_command")] + assert_eq!(key_store.is_pin_longer_than(4), Ok(false)); + key_store.store_pin(&[1; 16], 4).unwrap(); + assert_eq!(key_store.has_pin(), Ok(true)); + #[cfg(feature = "config_command")] + assert_eq!(key_store.is_pin_longer_than(3), Ok(true)); + #[cfg(feature = "config_command")] + assert_eq!(key_store.is_pin_longer_than(4), Ok(false)); + assert_eq!(key_store.verify_pin_hash(&[1; 16]), Ok(true)); for index in 0..16 { let mut candidate = [1; 16]; candidate[index] ^= 1; - assert_eq!(key_store.verify_pin_hash(&stored, &candidate), Ok(false)); + assert_eq!(key_store.verify_pin_hash(&candidate), Ok(false)); } + key_store.store_pin(&[2; 16], 6).unwrap(); + #[cfg(feature = "config_command")] + assert_eq!(key_store.is_pin_longer_than(5), Ok(true)); + assert_eq!(key_store.verify_pin_hash(&[1; 16]), Ok(false)); + assert_eq!(key_store.verify_pin_hash(&[2; 16]), Ok(true)); + crate::ctap::reset(&mut env).unwrap(); + assert_eq!(env.key_store().has_pin(), Ok(false)); } fn test_wrap_unwrap_credential(signature_algorithm: SignatureAlgorithm) { diff --git a/libraries/opensk/src/ctap/client_pin.rs b/libraries/opensk/src/ctap/client_pin.rs index 10fbbfa5..fefe3ca0 100644 --- a/libraries/opensk/src/ctap/client_pin.rs +++ b/libraries/opensk/src/ctap/client_pin.rs @@ -40,6 +40,7 @@ use alloc::vec::Vec; use arrayref::array_ref; #[cfg(test)] use enum_iterator::{Sequence, all}; +use subtle::ConstantTimeEq; /// The prefix length of the PIN hash that is stored and compared. /// @@ -83,16 +84,17 @@ fn decrypt_pin( Ok(result) } -/// Stores a hash prefix of the new PIN in the persistent storage, if correct. +/// Checks PIN policy and returns the new PIN hash prefix and code point length. /// /// The new PIN is passed encrypted, so it is first decrypted and stripped from /// padding. Next, it is checked against the PIN policy. Last, it is hashed and -/// truncated for persistent storage. -fn check_and_store_new_pin( +/// truncated for storage. +fn check_new_pin( env: &mut E, shared_secret: &SharedSecret, new_pin_enc: Vec, -) -> CtapResult<()> { + old_pin_hash: Option<&[u8]>, +) -> CtapResult<(Secret<[u8; PIN_AUTH_LENGTH]>, u8)> { let pin = decrypt_pin(shared_secret, new_pin_enc)?; let min_pin_length = storage::min_pin_length(env)? as usize; let pin_length = str::from_utf8(&pin).unwrap_or("").chars().count(); @@ -103,18 +105,28 @@ fn check_and_store_new_pin( Sha::::digest_mut(&pin, &mut pin_hash); let new_pin_hash = array_ref![pin_hash, 0, PIN_AUTH_LENGTH]; if env.persist().has_force_pin_change()? - && let Some(old_pin_hash) = env.persist().pin_hash()? - && env - .key_store() - .verify_pin_hash(&old_pin_hash, new_pin_hash)? + && let Some(old_pin_hash) = old_pin_hash + && bool::from(old_pin_hash.ct_eq(new_pin_hash)) { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_POLICY_VIOLATION); } - let pin_hash = env.key_store().encrypt_pin_hash(new_pin_hash)?; + let mut result = Secret::<[u8; PIN_AUTH_LENGTH]>::default(); + result.copy_from_slice(new_pin_hash); // The PIN length is always < PIN_PADDED_LENGTH < 256. - env.persist() - .set_pin(array_ref!(pin_hash, 0, PIN_AUTH_LENGTH), pin_length as u8)?; - Ok(()) + Ok((result, pin_length as u8)) +} + +// Retry accounting belongs to CTAP, including lockout reported by hardware. +fn process_pin_verification_result( + env: &mut impl Env, + result: CtapResult, +) -> CtapResult { + if result == Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_BLOCKED) { + let failures = env.persist().pin_fails()?.saturating_sub(1); + env.persist() + .insert(crate::api::persist::keys::PIN_RETRIES, &[failures])?; + } + result } pub(crate) fn check_pin_uv_auth_protocol( @@ -204,19 +216,23 @@ impl ClientPin { pin_uv_auth_protocol: PinUvAuthProtocol, shared_secret: &SharedSecret, pin_hash_enc: Vec, - ) -> CtapResult<()> { - match env.persist().pin_hash()? { - Some(pin_hash) => { + ) -> CtapResult> { + let candidate = match env.key_store().has_pin()? { + true => { if self.consecutive_pin_mismatches >= 3 { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_BLOCKED); } storage::decr_pin_retries(env)?; - let is_valid = match shared_secret.decrypt(&pin_hash_enc) { - Ok(candidate) => match <&[u8; PIN_AUTH_LENGTH]>::try_from(&*candidate) { - Ok(candidate) => env.key_store().verify_pin_hash(&pin_hash, candidate)?, + let candidate = shared_secret.decrypt(&pin_hash_enc).ok(); + let is_valid = match candidate.as_deref() { + Some(candidate) => match <&[u8; PIN_AUTH_LENGTH]>::try_from(candidate) { + Ok(candidate) => { + let result = env.key_store().verify_pin_hash(candidate); + process_pin_verification_result(env, result)? + } Err(_) => false, }, - Err(_) => false, + None => false, }; if !is_valid { @@ -231,15 +247,16 @@ impl ClientPin { } return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID); } + candidate.unwrap() } // This status code is not explicitly mentioned in the specification. - None => return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED), - } + false => return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED), + }; storage::reset_pin_retries(env)?; #[cfg(feature = "fingerprint")] storage::reset_uv_retries(env)?; self.consecutive_pin_mismatches = 0; - Ok(()) + Ok(candidate) } fn process_get_pin_retries(&self, env: &mut E) -> CtapResult { @@ -285,13 +302,16 @@ impl ClientPin { let pin_uv_auth_param = ok_or_missing(pin_uv_auth_param)?; let new_pin_enc = ok_or_missing(new_pin_enc)?; - if env.persist().pin_hash()?.is_some() { + if env.key_store().has_pin()? { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID); } let shared_secret = self.get_shared_secret(pin_uv_auth_protocol, key_agreement)?; shared_secret.verify(&new_pin_enc, &pin_uv_auth_param)?; - check_and_store_new_pin(env, &shared_secret, new_pin_enc)?; + let (pin_hash, length) = check_new_pin(env, &shared_secret, new_pin_enc, None)?; + env.key_store().store_pin(&pin_hash, length)?; + env.persist() + .remove(crate::api::persist::keys::FORCE_PIN_CHANGE)?; storage::reset_pin_retries(env)?; Ok(()) } @@ -321,10 +341,16 @@ impl ClientPin { let mut auth_param_data = new_pin_enc.clone(); auth_param_data.extend(&pin_hash_enc); shared_secret.verify(&auth_param_data, &pin_uv_auth_param)?; - self.verify_pin_hash_enc(env, pin_uv_auth_protocol, &shared_secret, pin_hash_enc)?; + let old_pin_hash = + self.verify_pin_hash_enc(env, pin_uv_auth_protocol, &shared_secret, pin_hash_enc)?; - check_and_store_new_pin(env, &shared_secret, new_pin_enc)?; + let (pin_hash, length) = + check_new_pin(env, &shared_secret, new_pin_enc, Some(&old_pin_hash))?; + // Invalidate tokens before replacement, which may fail after committing the new PIN. self.reset_pin_uv_auth_token(env); + env.key_store().store_pin(&pin_hash, length)?; + env.persist() + .remove(crate::api::persist::keys::FORCE_PIN_CHANGE)?; Ok(()) } @@ -739,6 +765,23 @@ mod test { const DUMMY_CHANNEL: Channel = Channel::MainHid([0x12, 0x34, 0x56, 0x78]); /// Stores a PIN hash corresponding to the dummy PIN "1234". + #[test] + fn test_backend_lockout_refunds_only_unattempted_pin_verification() { + for (result, expected_failures) in [ + (Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_BLOCKED), 2), + (Ok(false), 3), + (Ok(true), 3), + (Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE), 3), + ] { + let mut env = TestEnv::default(); + env.persist() + .insert(crate::api::persist::keys::PIN_RETRIES, &[3]) + .unwrap(); + assert_eq!(process_pin_verification_result(&mut env, result), result); + assert_eq!(env.persist().pin_fails().unwrap(), expected_failures); + } + } + fn set_standard_pin(env: &mut TestEnv) { let mut pin_hash = [0u8; 16]; pin_hash.copy_from_slice(&Sha::::digest(b"1234")[..16]); @@ -880,13 +923,10 @@ mod test { let pin_hash_enc = shared_secret.encrypt(&mut env, &pin_hash).unwrap(); assert_eq!( - client_pin.verify_pin_hash_enc( - &mut env, - pin_uv_auth_protocol, - &shared_secret, - pin_hash_enc - ), - Ok(()) + &*client_pin + .verify_pin_hash_enc(&mut env, pin_uv_auth_protocol, &shared_secret, pin_hash_enc) + .unwrap(), + &pin_hash ); let pin_hash_enc = vec![0xEE; 16]; @@ -1534,7 +1574,7 @@ mod test { test_helper_decrypt_pin(PinUvAuthProtocol::V2); } - fn test_helper_check_and_store_new_pin(pin_uv_auth_protocol: PinUvAuthProtocol) { + fn test_helper_check_new_pin(pin_uv_auth_protocol: PinUvAuthProtocol) { let mut env = TestEnv::default(); let pin_protocol = PinProtocol::::new(&mut env); let shared_secret = pin_protocol @@ -1561,29 +1601,23 @@ mod test { ), ]; for (pin, result) in test_cases { - let old_pin_hash = env.persist().pin_hash().unwrap(); let new_pin_enc = encrypt_pin(&shared_secret, pin); assert_eq!( - check_and_store_new_pin(&mut env, &shared_secret, new_pin_enc), + check_new_pin(&mut env, &shared_secret, new_pin_enc, None).map(|_| ()), result ); - if result.is_ok() { - assert_ne!(old_pin_hash, env.persist().pin_hash().unwrap()); - } else { - assert_eq!(old_pin_hash, env.persist().pin_hash().unwrap()); - } } } #[test] - fn test_check_and_store_new_pin_v1() { - test_helper_check_and_store_new_pin(PinUvAuthProtocol::V1); + fn test_check_new_pin_v1() { + test_helper_check_new_pin(PinUvAuthProtocol::V1); } #[test] - fn test_check_and_store_new_pin_v2() { - test_helper_check_and_store_new_pin(PinUvAuthProtocol::V2); + fn test_check_new_pin_v2() { + test_helper_check_new_pin(PinUvAuthProtocol::V2); } /// Generates valid inputs for process_hmac_secret and returns the output. diff --git a/libraries/opensk/src/ctap/config_command.rs b/libraries/opensk/src/ctap/config_command.rs index c6698f3d..8a611f13 100644 --- a/libraries/opensk/src/ctap/config_command.rs +++ b/libraries/opensk/src/ctap/config_command.rs @@ -18,6 +18,7 @@ use super::data_formats::{ConfigSubCommand, ConfigSubCommandParams, SetMinPinLen use super::response::ResponseData; use super::status_code::Ctap2StatusCode; use crate::api::customization::Customization; +use crate::api::key_store::KeyStore; use crate::api::persist::Persist; use crate::ctap::status_code::CtapResult; use crate::ctap::storage; @@ -65,12 +66,11 @@ fn process_set_min_pin_length( return Err(Ctap2StatusCode::CTAP2_ERR_PIN_POLICY_VIOLATION); } let mut force_change_pin = force_change_pin.unwrap_or(false); - if force_change_pin && env.persist().pin_hash()?.is_none() { + let has_pin = env.key_store().has_pin()?; + if force_change_pin && !has_pin { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_NOT_SET); } - if let Some(old_length) = env.persist().pin_code_point_length()? { - force_change_pin |= new_min_pin_length > old_length; - } + force_change_pin |= has_pin && !env.key_store().is_pin_longer_than(new_min_pin_length - 1)?; // Write the RP IDs first: this is the only step that can fail because of its // input, so doing it before the other writes avoids leaving the minimum PIN // length or the force-change flag updated when the command ends up failing. diff --git a/libraries/opensk/src/ctap/fingerprint.rs b/libraries/opensk/src/ctap/fingerprint.rs index 2ce35665..38032ef5 100644 --- a/libraries/opensk/src/ctap/fingerprint.rs +++ b/libraries/opensk/src/ctap/fingerprint.rs @@ -23,6 +23,7 @@ use super::{Channel, CtapHid, KeepaliveStatus, send_packets, storage}; use crate::Transport; use crate::api::customization::Customization; use crate::api::fingerprint::{Fingerprint, FingerprintCheckError}; +use crate::api::key_store::KeyStore; use crate::api::persist::Persist; use crate::api::rng::Rng; use crate::ctap::cbor_write; @@ -140,7 +141,7 @@ pub fn perform_built_in_uv( if env.persist().template_infos()?.is_empty() { return Err(Ctap2StatusCode::CTAP2_ERR_NOT_ALLOWED); } - if (env.persist().pin_hash()?.is_some() && storage::pin_retries(env)? == 0) + if (env.key_store().has_pin()? && storage::pin_retries(env)? == 0) || storage::uv_retries(env)? == 0 { return Err(Ctap2StatusCode::CTAP2_ERR_UV_BLOCKED); diff --git a/libraries/opensk/src/ctap/mod.rs b/libraries/opensk/src/ctap/mod.rs index abbd9c3b..89f8f975 100644 --- a/libraries/opensk/src/ctap/mod.rs +++ b/libraries/opensk/src/ctap/mod.rs @@ -774,7 +774,7 @@ impl CtapState { // This case was added in FIDO 2.1. if auth_param.is_empty() { check_user_presence(env, channel)?; - if env.persist().pin_hash()?.is_none() { + if !env.key_store().has_pin()? { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_NOT_SET); } else { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID); @@ -1385,10 +1385,7 @@ impl CtapState { } let mut options = vec![ (String::from("rk"), true), - ( - String::from("clientPin"), - env.persist().pin_hash()?.is_some(), - ), + (String::from("clientPin"), env.key_store().has_pin()?), (String::from("up"), true), (String::from("pinUvAuthToken"), true), (String::from("largeBlobs"), true), diff --git a/libraries/opensk/src/ctap/storage.rs b/libraries/opensk/src/ctap/storage.rs index 8401dfe7..443e5259 100644 --- a/libraries/opensk/src/ctap/storage.rs +++ b/libraries/opensk/src/ctap/storage.rs @@ -301,7 +301,7 @@ pub fn has_user_verification(env: &mut impl Env) -> CtapResult { if !env.persist().template_infos()?.is_empty() { return Ok(true); } - Ok(env.persist().pin_hash()?.is_some()) + env.key_store().has_pin() } /// Returns whether alwaysUv is enabled.