Skip to content

Commit

Permalink
Clean up empty RP dirs if key store is full
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
robin-nitrokey committed Apr 28, 2023
1 parent db14bcf commit 708d5f7
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 28 deletions.
56 changes: 44 additions & 12 deletions src/ctap2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,8 @@ impl<UP: UserPresence, T: TrussedRequirements> 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);
Expand All @@ -382,21 +384,32 @@ impl<UP: UserPresence, T: TrussedRequirements> 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
Expand Down Expand Up @@ -1673,6 +1686,25 @@ impl<UP: UserPresence, T: TrussedRequirements> crate::Authenticator<UP, T> {

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 {
Expand Down
17 changes: 1 addition & 16 deletions src/ctap2/credential_management.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 708d5f7

Please sign in to comment.