Skip to content

Commit

Permalink
Merge pull request trussed-dev#14 from Nitrokey/overwrite-resident
Browse files Browse the repository at this point in the history 
Fix overwriting resident keys
  • Loading branch information
@robin-nitrokey
robin-nitrokey authored May 2, 2023
2 parents 7f6a093 + 708d5f7 commit 857899b
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 41 deletions.
66 changes: 51 additions & 15 deletions src/ctap2.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,27 +372,44 @@ 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);
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 {
return Err(Error::KeyStoreFull);
error!("maximum resident credential count reached");
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 @@ -844,7 +861,7 @@ impl<UP: UserPresence, T: TrussedRequirements> Authenticator for crate::Authenti
let sub_parameters = &parameters.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(),
Expand Down Expand Up @@ -1669,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
42 changes: 16 additions & 26 deletions src/ctap2/credential_management.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ where
UP: UserPresence,
T: TrussedRequirements,
{
pub fn get_creds_metadata(&mut self) -> Result<Response> {
pub fn get_creds_metadata(&mut self) -> Response {
info!("get metadata");
let mut response: Response = Default::default();

Expand All @@ -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 =
Expand All @@ -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 {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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<DirEntry>) {
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<Response> {
Expand All @@ -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())?;
Expand Down Expand Up @@ -479,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 857899b

Please sign in to comment.