From db14bcf54a5780f595317cd20f7d19b0563719d7 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Fri, 28 Apr 2023 19:33:47 +0200 Subject: [PATCH 1/2] Allow empty RP dirs in get_creds_metadata If we overwrite an existing resident credential, the corresponding RP directory can be empty when we call CredentialManagement::get_creds_metadata to count the existing credentials. This causes an error in the current implementation. This patch changes CredentialManagement::get_creds_metadata to gracefully handle empty RP directories. Fixes https://github.com/Nitrokey/nitrokey-3-firmware/issues/254 --- src/ctap2.rs | 10 +++++++--- src/ctap2/credential_management.rs | 25 +++++++++++++++---------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/ctap2.rs b/src/ctap2.rs index ea1ad40..0f279a5 100644 --- a/src/ctap2.rs +++ b/src/ctap2.rs @@ -375,9 +375,13 @@ impl Authenticator for crate::Authenti // then check the maximum number of RK credentials if let Some(max_count) = self.config.max_resident_credential_count { let mut cm = credential_management::CredentialManagement::new(self); - let metadata = cm.get_creds_metadata()?; - let count = metadata.existing_resident_credentials_count.unwrap_or(max_count); + let metadata = cm.get_creds_metadata(); + let count = metadata + .existing_resident_credentials_count + .unwrap_or(max_count); + debug!("resident cred count: {} (max: {})", count, max_count); if count >= max_count { + error!("maximum resident credential count reached"); return Err(Error::KeyStoreFull); } } @@ -844,7 +848,7 @@ impl Authenticator for crate::Authenti let sub_parameters = ¶meters.sub_command_params; match parameters.sub_command { // 0x1 - Subcommand::GetCredsMetadata => cred_mgmt.get_creds_metadata(), + Subcommand::GetCredsMetadata => Ok(cred_mgmt.get_creds_metadata()), // 0x2 Subcommand::EnumerateRpsBegin => cred_mgmt.first_relying_party(), diff --git a/src/ctap2/credential_management.rs b/src/ctap2/credential_management.rs index 39eaaf8..c88dc4d 100644 --- a/src/ctap2/credential_management.rs +++ b/src/ctap2/credential_management.rs @@ -62,7 +62,7 @@ where UP: UserPresence, T: TrussedRequirements, { - pub fn get_creds_metadata(&mut self) -> Result { + pub fn get_creds_metadata(&mut self) -> Response { info!("get metadata"); let mut response: Response = Default::default(); @@ -71,7 +71,8 @@ where .max_resident_credential_count .unwrap_or(MAX_RESIDENT_CREDENTIALS_GUESSTIMATE); response.existing_resident_credentials_count = Some(0); - response.max_possible_remaining_residential_credentials_count = Some(max_resident_credentials); + response.max_possible_remaining_residential_credentials_count = + Some(max_resident_credentials); let dir = PathBuf::from(b"rk"); let maybe_first_rp = @@ -81,11 +82,11 @@ where .entry; let first_rp = match maybe_first_rp { - None => return Ok(response), + None => return response, Some(rp) => rp, }; - let (mut num_rks, _) = self.count_rp_rks(PathBuf::from(first_rp.path()))?; + let (mut num_rks, _) = self.count_rp_rks(PathBuf::from(first_rp.path())); let mut last_rp = PathBuf::from(first_rp.file_name()); loop { @@ -101,12 +102,12 @@ where response.existing_resident_credentials_count = Some(num_rks); response.max_possible_remaining_residential_credentials_count = Some(max_resident_credentials.saturating_sub(num_rks)); - return Ok(response); + return response; } Some(rp) => { last_rp = PathBuf::from(rp.file_name()); info!("counting.."); - let (this_rp_rk_count, _) = self.count_rp_rks(PathBuf::from(rp.path()))?; + let (this_rp_rk_count, _) = self.count_rp_rks(PathBuf::from(rp.path())); info!("{:?}", this_rp_rk_count); num_rks += this_rp_rk_count; } @@ -265,21 +266,24 @@ where Ok(response) } - fn count_rp_rks(&mut self, rp_dir: PathBuf) -> Result<(u32, DirEntry)> { + fn count_rp_rks(&mut self, rp_dir: PathBuf) -> (u32, Option) { let maybe_first_rk = syscall!(self .trussed .read_dir_first(Location::Internal, rp_dir, None)) .entry; - let first_rk = maybe_first_rk.ok_or(Error::NoCredentials)?; + let Some(first_rk) = maybe_first_rk else { + warn!("empty RP directory"); + return (0, None); + }; // count the rest of them let mut num_rks = 1; while syscall!(self.trussed.read_dir_next()).entry.is_some() { num_rks += 1; } - Ok((num_rks, first_rk)) + (num_rks, Some(first_rk)) } pub fn first_credential(&mut self, rp_id_hash: &Bytes<32>) -> Result { @@ -291,7 +295,8 @@ where super::format_hex(&rp_id_hash[..8], &mut hex); let rp_dir = PathBuf::from(b"rk").join(&PathBuf::from(&hex)); - let (num_rks, first_rk) = self.count_rp_rks(rp_dir)?; + let (num_rks, first_rk) = self.count_rp_rks(rp_dir); + let first_rk = first_rk.ok_or(Error::NoCredentials)?; // extract data required into response let mut response = self.extract_response_from_credential_file(first_rk.path())?; From 708d5f7f526556b766ec83e5bff6afe363da72dd Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Fri, 28 Apr 2023 19:48:11 +0200 Subject: [PATCH 2/2] Clean up empty RP dirs if key store is full If we try to register a new resident credential, we delete existing credentials with the same RP and user ID. If we then cannot store the new credential because the credential limit is reached or there is a filesystem write error, we may be left with an empty RP dir. This patch adds a check to delete empty RP dirs in this case. --- src/ctap2.rs | 56 +++++++++++++++++++++++------- src/ctap2/credential_management.rs | 17 +-------- 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/src/ctap2.rs b/src/ctap2.rs index 0f279a5..73129ce 100644 --- a/src/ctap2.rs +++ b/src/ctap2.rs @@ -372,6 +372,8 @@ impl Authenticator for crate::Authenti self.delete_resident_key_by_user_id(&rp_id_hash, &credential.user.id) .ok(); + let mut key_store_full = false; + // then check the maximum number of RK credentials if let Some(max_count) = self.config.max_resident_credential_count { let mut cm = credential_management::CredentialManagement::new(self); @@ -382,21 +384,32 @@ impl Authenticator for crate::Authenti debug!("resident cred count: {} (max: {})", count, max_count); if count >= max_count { error!("maximum resident credential count reached"); - return Err(Error::KeyStoreFull); + key_store_full = true; } } - // then store key, making it resident - let credential_id_hash = self.hash(credential_id.0.as_ref()); - try_syscall!(self.trussed.write_file( - Location::Internal, - rk_path(&rp_id_hash, &credential_id_hash), - serialized_credential, - // user attribute for later easy lookup - // Some(rp_id_hash.clone()), - None, - )) - .map_err(|_| Error::KeyStoreFull)?; + if !key_store_full { + // then store key, making it resident + let credential_id_hash = self.hash(credential_id.0.as_ref()); + let result = try_syscall!(self.trussed.write_file( + Location::Internal, + rk_path(&rp_id_hash, &credential_id_hash), + serialized_credential, + // user attribute for later easy lookup + // Some(rp_id_hash.clone()), + None, + )); + key_store_full = result.is_err(); + } + + if key_store_full { + // If we previously deleted an existing cred with the same RP + UserId but then + // failed to store the new cred, the RP directory could now be empty. This is not + // a valid state so we have to delete it. + let rp_dir = rp_rk_dir(&rp_id_hash); + self.delete_rp_dir_if_empty(rp_dir); + return Err(Error::KeyStoreFull); + } } // 13. generate and return attestation statement using clientDataHash @@ -1673,6 +1686,25 @@ impl crate::Authenticator { Ok(()) } + + pub(crate) fn delete_rp_dir_if_empty(&mut self, rp_path: PathBuf) { + let maybe_first_remaining_rk = + syscall!(self + .trussed + .read_dir_first(Location::Internal, rp_path.clone(), None,)) + .entry; + + if maybe_first_remaining_rk.is_none() { + info!("deleting parent {:?} as this was its last RK", &rp_path); + syscall!(self.trussed.remove_dir(Location::Internal, rp_path,)); + } else { + info!( + "not deleting deleting parent {:?} as there is {:?}", + &rp_path, + &maybe_first_remaining_rk.unwrap().path(), + ); + } + } } fn rp_rk_dir(rp_id_hash: &Bytes<32>) -> PathBuf { diff --git a/src/ctap2/credential_management.rs b/src/ctap2/credential_management.rs index c88dc4d..4ae63c9 100644 --- a/src/ctap2/credential_management.rs +++ b/src/ctap2/credential_management.rs @@ -484,23 +484,8 @@ where .parent() // by construction, RK has a parent, its RP .unwrap(); + self.delete_rp_dir_if_empty(rp_path); - let maybe_first_remaining_rk = - syscall!(self - .trussed - .read_dir_first(Location::Internal, rp_path.clone(), None,)) - .entry; - - if maybe_first_remaining_rk.is_none() { - info!("deleting parent {:?} as this was its last RK", &rp_path); - syscall!(self.trussed.remove_dir(Location::Internal, rp_path,)); - } else { - info!( - "not deleting deleting parent {:?} as there is {:?}", - &rp_path, - &maybe_first_remaining_rk.unwrap().path(), - ); - } // just return OK let response = Default::default(); Ok(response)