Merge pull request #30 from Nitrokey/fix-deletion

Fix factory-reset-app

Cargo.toml

@@ -47,6 +47,7 @@ p256 = { version = "0.13.2", default-features = false, features = ["ecdsa-core"]
 salty = "0.3.0"
 p256-cortex-m4 = { version = "0.1.0-alpha.6", features = ["prehash", "sec1-signatures"] }
 admin-app = "0.1.0"
+bitflags = "2.5.0"
 
 [dev-dependencies]
 admin-app = { version = "0.1.0", features = ["migration-tests"] }

src/core_api.rs

@@ -1,5 +1,6 @@
 //! Implementations of the core APIs for the SE050 backend
 
+use bitflags::bitflags;
 use cbor_smol::{cbor_deserialize, cbor_serialize};
 use crypto_bigint::{
     subtle::{ConstantTimeEq, ConstantTimeGreater, CtOption},
@@ -82,6 +83,13 @@ struct WrappedKeyData {
     ty: WrappedKeyType,
 }
 
+bitflags! {
+    pub(crate) struct ItemsToDelete: u8 {
+        const KEYS = 0b00000001;
+        const PINS = 0b00000010;
+    }
+}
+
 /// The bool returned points at wether the mechanism is raw RSA
 fn bits_and_kind_from_mechanism(mechanism: Mechanism) -> Result<(usize, key::Kind, bool), Error> {
     match mechanism {
@@ -224,7 +232,7 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> Se050Backend<Twi, D> {
         se050_keystore: &mut impl Keystore,
     ) -> Result<reply::Delete, Error> {
         let (parsed_key, _parsed_ty) = parse_key_id(*key, ns).ok_or_else(|| {
-            debug!("Failed to parse key id");
+            debug!("Failed to parse key id to delete: {key:?}");
             Error::RequestNotAvailable
         })?;
         debug!("Deleting {key:?}, {parsed_key:?}, {_parsed_ty:?}");
@@ -2011,7 +2019,7 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> Se050Backend<Twi, D> {
     ) -> Result<reply::Clear, Error> {
         debug!("Clearing: {:?}", req.key);
         let (parsed_key, _parsed_ty) = parse_key_id(req.key, ns).ok_or_else(|| {
-            debug!("Failed to parse key id");
+            debug!("Failed to parse key id to clear: {:?}", req.key);
             Error::RequestNotAvailable
         })?;
 
@@ -2050,7 +2058,7 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> Se050Backend<Twi, D> {
             return Err(Error::RequestNotAvailable);
         }
         let (parsed_key, _parsed_ty) = parse_key_id(req.key, ns).ok_or_else(|| {
-            debug!("Failed to parse key id");
+            debug!("Failed to parse key id to wrap: {:?}", req.key);
             // Let non-se050 keys be wrapped by the core backend
             Error::RequestNotAvailable
         })?;
@@ -2238,7 +2246,7 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> Se050Backend<Twi, D> {
         ns: NamespaceValue,
     ) -> Result<reply::Exists, Error> {
         let (parsed_key, parsed_ty) = parse_key_id(req.key, ns).ok_or_else(|| {
-            debug!("Failed to parse key id");
+            debug!("Failed to parse key id to check if exists: {:?}", req.key);
             // Let non-se050 keys be checked by the core backend
             Error::RequestNotAvailable
         })?;
@@ -2306,17 +2314,39 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> Se050Backend<Twi, D> {
         Ok(reply::Exists { exists })
     }
 
+    fn delete_object(&mut self, object_id: ObjectId, buf: &mut [u8]) -> Result<(), Error> {
+        self.se
+            .run_command(&DeleteSecureObject { object_id }, buf)
+            .map_err(|_err| {
+                error!("Failed to delete object: {object_id:02x?} {_err:?}");
+                Error::FunctionFailed
+            })?;
+        Ok(())
+    }
+
     pub(crate) fn delete_all_items(
         &mut self,
-        locations: &[Location],
+        kinds: ItemsToDelete,
+        // Locations for deleted keys
+        key_locations: &[Location],
         ns: NamespaceValue,
     ) -> Result<usize, Error> {
         let buf = &mut [0; 1024];
         let buf2 = &mut [0; 128];
         let mut count = 0u16;
         let mut offset = 0;
+        let mut cont = true;
+
+        let should_delete_internal = key_locations.contains(&Location::Internal);
+        let should_delete_external = key_locations.contains(&Location::External);
+        let should_delete_volatile = key_locations.contains(&Location::Volatile);
+        // Persistent keys do not have a "location"
+        let should_delete_persistent = should_delete_internal || should_delete_external;
+
+        let should_delete_keys = kinds.contains(ItemsToDelete::KEYS);
+        let should_delete_pins = kinds.contains(ItemsToDelete::PINS);
 
-        loop {
+        while cont {
             let to_delete = self
                 .se
                 .run_command(
@@ -2330,69 +2360,82 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> Se050Backend<Twi, D> {
                     error!("Failed to read id_list: {_err:?}");
                     Error::FunctionFailed
                 })?;
+            cont = to_delete.more.is_more();
 
             assert_eq!(to_delete.ids.len() % 4, 0);
             offset += (to_delete.ids.len() / 4) as u16;
             for obj_slice in to_delete.ids.chunks_exact(4) {
                 let obj = ObjectId(obj_slice.try_into().unwrap());
-                debug!("Dealing with obj: {obj:02x?}");
+                debug_now!("Dealing with obj: {obj_slice:02x?}");
                 if !object_in_range(obj) {
                     continue;
                 }
-                debug!("In range");
+                debug!("In range for a non-native object");
                 let Some((ns_to_check, parsed_obj_id)) = ParsedObjectId::parse(obj) else {
                     debug!("Failed to parse object id");
                     continue;
                 };
                 if ns_to_check != ns {
                     continue;
                 }
-                debug!("In ns");
+                debug!("In namespace");
                 debug!("Got parsed: {parsed_obj_id:02x?}");
                 match parsed_obj_id {
-                    ParsedObjectId::PersistentKey(obj)
-                        if locations.contains(&Location::Internal)
-                            || locations.contains(&Location::External) =>
-                    {
+                    ParsedObjectId::PersistentKey(obj) => {
+                        if !should_delete_persistent || !should_delete_keys {
+                            continue;
+                        }
+
+                        self.delete_object(obj.0, buf2)?;
                         count += 1;
                         offset = offset.saturating_sub(1);
-                        self.se
-                            .run_command(&DeleteSecureObject { object_id: obj.0 }, buf2)
-                            .map_err(|_err| {
-                                error!("Failed to delete object: {obj:02x?} {_err:?}");
-                                Error::FunctionFailed
-                            })?;
                     }
-                    ParsedObjectId::VolatileKey(obj) if locations.contains(&Location::Volatile) => {
+                    ParsedObjectId::VolatileKey(obj) => {
+                        if !should_delete_volatile || !should_delete_keys {
+                            continue;
+                        }
+
+                        self.delete_object(obj.0, buf2)?;
                         count += 1;
                         offset = offset.saturating_sub(1);
-                        self.se
-                            .run_command(&DeleteSecureObject { object_id: obj.0 }, buf2)
-                            .map_err(|_err| {
-                                error!("Failed to delete object: {obj:02x?} {_err:?}");
-                                Error::FunctionFailed
-                            })?;
                     }
-                    ParsedObjectId::VolatileRsaKey(obj)
-                        if locations.contains(&Location::Volatile) =>
-                    {
+                    ParsedObjectId::VolatileRsaKey(obj) => {
+                        if !should_delete_volatile || !should_delete_keys {
+                            continue;
+                        }
+
                         let deleted_count = self.delete_volatile_rsa_key_on_se050(obj)?;
                         // VolatileRsaKeys can appear twice (intermediary and real key. They should be counted as one)
                         count += if deleted_count == 0 { 0 } else { 1 };
                         offset = offset.saturating_sub(deleted_count);
                     }
-                    ParsedObjectId::Pin(_)
-                    | ParsedObjectId::PinWithDerived(_)
-                    | ParsedObjectId::SaltValue(_) => {}
-                    ParsedObjectId::VolatileKey(_) | ParsedObjectId::VolatileRsaKey(_) => {}
-                    ParsedObjectId::PersistentKey(_) => {}
-                }
-            }
+                    ParsedObjectId::Pin(se_id) => {
+                        if !should_delete_pins {
+                            continue;
+                        }
 
-            if !to_delete.more.is_more() {
-                break;
+                        debug!("Deleting simple pin");
+                        self.delete_object(se_id.0, buf2)?;
+                        count += 1;
+                        offset = offset.saturating_sub(1);
+                    }
+                    ParsedObjectId::PinWithDerived(se_id) => {
+                        if !should_delete_pins {
+                            continue;
+                        }
+
+                        use crate::trussed_auth_impl::data::PinData;
+                        let deleted_count = PinData::delete_with_derived(&mut self.se, se_id)?;
+                        count += if deleted_count == 0 { 0 } else { 1 };
+                        offset = offset.saturating_sub(deleted_count);
+                    }
+                    // Salt value is not part of any namespace
+                    // It is only deteled through the full device factory reset
+                    ParsedObjectId::SaltValue(_) => {}
+                }
             }
         }
+        debug_now!("Deleted {count} items");
         Ok(count.into())
     }
 
@@ -2408,7 +2451,7 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> Se050Backend<Twi, D> {
         debug!("Deleted core: {core_count}");
         debug!("Deleted fs: {_fs_count}");
 
-        let count = self.delete_all_items(&[req.location], ns)?;
+        let count = self.delete_all_items(ItemsToDelete::KEYS, &[req.location], ns)?;
 
         Ok(reply::DeleteAllKeys {
             count: core_count + count,

src/staging.rs

@@ -19,6 +19,7 @@ use trussed_wrap_key_to_file::{
     reply as ext_reply, WrapKeyToFileExtension, WrapKeyToFileReply, WrapKeyToFileRequest,
 };
 
+use crate::core_api::ItemsToDelete;
 use crate::{core_api::CORE_DIR, Se050Backend, BACKEND_DIR};
 
 impl<Twi: I2CForT1, D: DelayUs<u32>> ExtensionImpl<WrapKeyToFileExtension>
@@ -154,6 +155,7 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> ExtensionImpl<ManageExtension> for Se050Bac
                     Error::RequestNotAvailable
                 })?;
                 self.delete_all_items(
+                    ItemsToDelete::KEYS | ItemsToDelete::PINS,
                     &[Location::Volatile, Location::External, Location::Internal],
                     ns,
                 )?;

src/trussed_auth_impl.rs

@@ -22,7 +22,7 @@ use trussed::{
 };
 use trussed_auth::MAX_HW_KEY_LEN;
 
-mod data;
+pub(crate) mod data;
 
 use crate::{
     namespacing::{namespace, NamespaceValue, ObjectKind},
@@ -436,9 +436,16 @@ impl<Twi: I2CForT1, D: DelayUs<u32>> ExtensionImpl<trussed_auth::AuthExtension>
                 Ok(reply::DeletePin {}.into())
             }
             AuthRequest::DeleteAllPins(request::DeleteAllPins) => {
+                use crate::core_api::ItemsToDelete;
                 let fs = &mut fs(resources, core_ctx);
+                // Satisfy the borrow checker
+                // The `once` trick makes it loose the information that drop is a noop :/
+                drop(global_fs);
 
                 delete_all_pins(fs, self.metadata_location, &mut self.se)?;
+
+                // Ensure that any remaining PIN for the application is also deleted
+                self.delete_all_items(ItemsToDelete::PINS, &[], ns)?;
                 Ok(reply::DeleteAllPins.into())
             }
             AuthRequest::PinRetries(request) => {

src/trussed_auth_impl/data.rs

@@ -439,7 +439,8 @@ impl PinData {
     pub fn delete_with_derived<Twi: I2CForT1, D: DelayUs<u32>>(
         se050: &mut Se05X<Twi, D>,
         se_id: PinObjectIdWithDerived,
-    ) -> Result<(), Error> {
+    ) -> Result<u16, Error> {
+        let mut count = 0;
         let buf = &mut [0; 1024];
         debug!("checking existence");
         let exists = se050
@@ -467,6 +468,24 @@ impl PinData {
                     debug!("Failed deletion: {_err:?}");
                     _err
                 })?;
+            count += 1;
+        }
+
+        let exists = se050
+            .run_command(
+                &CheckObjectExists {
+                    object_id: se_id.pin_id(),
+                },
+                buf,
+            )
+            .map_err(|_err| {
+                error!("Failed existence check: {_err:?}");
+                Error::ReadFailed
+            })?
+            .result
+            .is_success();
+        if !exists {
+            return Ok(count);
         }
 
         debug!("Writing userid ");
@@ -525,31 +544,33 @@ impl PinData {
                 error!("Failed to delete user id: {err:?}");
                 err
             })?;
-        Ok(())
+        count += 1;
+        Ok(count)
     }
 
     pub fn delete<Twi: I2CForT1, D: DelayUs<u32>>(
         self,
         fs: &mut impl Filestore,
         location: Location,
         se050: &mut Se05X<Twi, D>,
-    ) -> Result<(), Error> {
+    ) -> Result<u16, Error> {
         debug!("Deleting {self:02x?}");
-        match self.se_id {
+        let deleted_count = match self.se_id {
             PinSeId::WithDerived(se_id) => Self::delete_with_derived(se050, se_id)?,
             PinSeId::Raw(se_id) => {
                 let buf = &mut [0; 1024];
                 debug!("Deleting simple");
                 se050.run_command(&DeleteSecureObject { object_id: se_id.0 }, buf)?;
+                1
             }
-        }
+        };
 
         debug!("Removing file {}", self.id.path());
         fs.remove_file(&self.id.path(), location).map_err(|_err| {
             error!("Removing file failed: {_err:?}");
             Error::WriteFailed
         })?;
-        Ok(())
+        Ok(deleted_count)
     }
 
     /// Returns (tried, max)