From 708d5f7f526556b766ec83e5bff6afe363da72dd Mon Sep 17 00:00:00 2001
From: Robin Krahl <robin@nitrokey.com>
Date: Fri, 28 Apr 2023 19:48:11 +0200
Subject: [PATCH] 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<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);
@@ -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
@@ -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 {
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)