From f95e2be7422243bbbb07ae07f6b026bd6d578099 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Thu, 7 Jun 2018 00:41:20 +0200 Subject: Remove NK_lock_device call from PasswordSafe::drop When enabled, the password safe can be used without authentication. The lock device can be used to lock the password safe. Currently, PasswordSafe::drop calls this command to make sure that other applications cannot access the password safe without authentication. On the Nitrokey Storage, locking the device may also disable the encrypted or hidden volume. As using the password safe should not have side effects on the storage volumes, this patch removes the call to the lock device command from the Drop implementation. Instead, the user should call this method after making sure that it does not have side effects. A feature request for a command that only locks the password safe without side effects is submitted to the Nitrokey Storage firmware repository: https://github.com/Nitrokey/nitrokey-storage-firmware/issues/65 --- TODO.md | 5 ++++- src/pws.rs | 44 +++++++++++++++++++++++++++++++------------- src/tests/pws.rs | 4 ++++ 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/TODO.md b/TODO.md index e2fb3fe..b666ce3 100644 --- a/TODO.md +++ b/TODO.md @@ -35,6 +35,9 @@ - Differentiate empty strings and errors (see `result_from_string`). - Check integer conversions. - Consider implementing `Into` for `(Device, CommandError)` -- Check error handling in PasswordSafe::drop(). +- Lock password safe in `PasswordSafe::drop()` (see [nitrokey-storage-firmware + issue 65][]). - Disable creation of multiple password safes at the same time. - Check timing in Storage tests. + +[nitrokey-storage-firmware issue 65]: https://github.com/Nitrokey/nitrokey-storage-firmware/issues/65 diff --git a/src/pws.rs b/src/pws.rs index 85726c5..903ecef 100644 --- a/src/pws.rs +++ b/src/pws.rs @@ -17,27 +17,39 @@ pub const SLOT_COUNT: u8 = 16; /// the [`GetPasswordSafe`][] trait. Note that the device must live at least as long as the /// password safe. /// +/// Once the password safe has been unlocked, it can be accessed without a password. Therefore it +/// is mandatory to call [`lock`][] on the corresponding device after the password store is used. +/// As this command may have side effects on the Nitrokey Storage, it cannot be called +/// automatically once the password safe is destroyed. +/// /// # Examples /// /// Open a password safe and access a password: /// /// ```no_run -/// use nitrokey::GetPasswordSafe; +/// use nitrokey::{Device, GetPasswordSafe, PasswordSafe}; /// # use nitrokey::CommandError; /// +/// fn use_password_safe(pws: &PasswordSafe) -> Result<(), CommandError> { +/// let name = pws.get_slot_name(0)?; +/// let login = pws.get_slot_login(0)?; +/// let password = pws.get_slot_login(0)?; +/// println!("Credentials for {}: login {}, password {}", name, login, password); +/// Ok(()) +/// } +/// /// # fn try_main() -> Result<(), CommandError> { /// let device = nitrokey::connect()?; /// let pws = device.get_password_safe("123456")?; -/// let name = pws.get_slot_name(0)?; -/// let login = pws.get_slot_login(0)?; -/// let password = pws.get_slot_login(0)?; -/// println!("Credentials for {}: login {}, password {}", name, login, password); +/// use_password_safe(&pws); +/// device.lock(); /// # Ok(()) /// # } /// ``` /// /// [`SLOT_COUNT`]: constant.SLOT_COUNT.html /// [`get_password_safe`]: trait.GetPasswordSafe.html#method.get_password_safe +/// [`lock`]: trait.Device.html#method.lock /// [`GetPasswordSafe`]: trait.GetPasswordSafe.html pub struct PasswordSafe<'a> { _device: &'a Device, @@ -50,9 +62,12 @@ pub struct PasswordSafe<'a> { /// /// [`PasswordSafe`]: struct.PasswordSafe.html pub trait GetPasswordSafe { - /// Enables and returns the password safe. This method consumes the device. You can go back - /// to the device using the [`device`][] method of the returned password safe. If the method - /// fails, the current device will be returned as part of the error. + /// Enables and returns the password safe. + /// + /// The underlying device must always live at least as long as a password safe retrieved from + /// it. It is mandatory to lock the underlying device using [`lock`][] after the password safe + /// has been used. Otherwise, other applications can access the password store without + /// authentication. /// /// # Errors /// @@ -62,7 +77,7 @@ pub trait GetPasswordSafe { /// # Example /// /// ```no_run - /// use nitrokey::{GetPasswordSafe, PasswordSafe}; + /// use nitrokey::{Device, GetPasswordSafe, PasswordSafe}; /// # use nitrokey::CommandError; /// /// fn use_password_safe(pws: &PasswordSafe) {} @@ -70,7 +85,10 @@ pub trait GetPasswordSafe { /// # fn try_main() -> Result<(), CommandError> { /// let device = nitrokey::connect()?; /// match device.get_password_safe("123456") { - /// Ok(pws) => use_password_safe(&pws), + /// Ok(pws) => { + /// use_password_safe(&pws); + /// device.lock(); + /// }, /// Err(err) => println!("Could not open the password safe: {:?}", err), /// }; /// # Ok(()) @@ -78,6 +96,7 @@ pub trait GetPasswordSafe { /// ``` /// /// [`device`]: struct.PasswordSafe.html#method.device + /// [`lock`]: trait.Device.html#method.lock /// [`InvalidString`]: enum.CommandError.html#variant.InvalidString /// [`WrongPassword`]: enum.CommandError.html#variant.WrongPassword fn get_password_safe(&self, user_pin: &str) -> Result; @@ -317,9 +336,8 @@ impl<'a> PasswordSafe<'a> { impl<'a> Drop for PasswordSafe<'a> { fn drop(&mut self) { - unsafe { - nitrokey_sys::NK_lock_device(); - } + // TODO: disable the password safe -- NK_lock_device has side effects on the Nitrokey + // Storage, see https://github.com/Nitrokey/nitrokey-storage-firmware/issues/65 } } diff --git a/src/tests/pws.rs b/src/tests/pws.rs index d6125a9..02e33cd 100644 --- a/src/tests/pws.rs +++ b/src/tests/pws.rs @@ -1,3 +1,4 @@ +use device::Device; use nitrokey_sys; use pws::{GetPasswordSafe, PasswordSafe, SLOT_COUNT}; use tests::util::{Target, ADMIN_PASSWORD, USER_PASSWORD}; @@ -36,6 +37,9 @@ fn drop() { assert_eq!(Ok(String::from("name")), result); } let result = result_from_string(unsafe { nitrokey_sys::NK_get_password_safe_slot_name(1) }); + assert_eq!(Ok(String::from("name")), result); + device.lock(); + let result = result_from_string(unsafe { nitrokey_sys::NK_get_password_safe_slot_name(1) }); assert_eq!(Err(CommandError::NotAuthorized), result); } -- cgit v1.2.3