From 669fbb40d894460e9603dcf6e953373e53a19347 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Tue, 28 Jan 2020 19:42:41 +0100 Subject: Use CString to store temporary passwords This patch changes the generate_password function and the User and Admin structs to use a CString instead of a Vec when storing temporary passwords. This makes sure that the strings that are passed to the C API are properly null-terminated. --- src/auth.rs | 17 +++++++++-------- src/util.rs | 4 ++-- 2 files changed, 11 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/auth.rs b/src/auth.rs index cab1021..571e198 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: MIT use std::convert::TryFrom as _; +use std::ffi::CString; use std::marker; use std::ops; use std::os::raw::c_char; @@ -117,7 +118,7 @@ pub trait Authenticate<'a> { } trait AuthenticatedDevice { - fn new(device: T, temp_password: Vec) -> Self; + fn new(device: T, temp_password: CString) -> Self; fn temp_password_ptr(&self) -> *const c_char; } @@ -134,7 +135,7 @@ trait AuthenticatedDevice { #[derive(Debug)] pub struct User<'a, T: Device<'a>> { device: T, - temp_password: Vec, + temp_password: CString, marker: marker::PhantomData<&'a T>, } @@ -150,7 +151,7 @@ pub struct User<'a, T: Device<'a>> { #[derive(Debug)] pub struct Admin<'a, T: Device<'a>> { device: T, - temp_password: Vec, + temp_password: CString, marker: marker::PhantomData<&'a T>, } @@ -169,7 +170,7 @@ where Err(err) => return Err((device, err)), }; let password_ptr = password.as_ptr(); - let temp_password_ptr = temp_password.as_ptr() as *const c_char; + let temp_password_ptr = temp_password.as_ptr(); match callback(password_ptr, temp_password_ptr) { 0 => Ok(A::new(device, temp_password)), rv => Err((device, Error::from(rv))), @@ -246,7 +247,7 @@ impl<'a, T: Device<'a>> GenerateOtp for User<'a, T> { } impl<'a, T: Device<'a>> AuthenticatedDevice for User<'a, T> { - fn new(device: T, temp_password: Vec) -> Self { + fn new(device: T, temp_password: CString) -> Self { User { device, temp_password, @@ -255,7 +256,7 @@ impl<'a, T: Device<'a>> AuthenticatedDevice for User<'a, T> { } fn temp_password_ptr(&self) -> *const c_char { - self.temp_password.as_ptr() as *const c_char + self.temp_password.as_ptr() } } @@ -373,7 +374,7 @@ impl<'a, T: Device<'a>> ConfigureOtp for Admin<'a, T> { } impl<'a, T: Device<'a>> AuthenticatedDevice for Admin<'a, T> { - fn new(device: T, temp_password: Vec) -> Self { + fn new(device: T, temp_password: CString) -> Self { Admin { device, temp_password, @@ -382,7 +383,7 @@ impl<'a, T: Device<'a>> AuthenticatedDevice for Admin<'a, T> { } fn temp_password_ptr(&self) -> *const c_char { - self.temp_password.as_ptr() as *const c_char + self.temp_password.as_ptr() } } diff --git a/src/util.rs b/src/util.rs index 5a56c55..b9b1a68 100644 --- a/src/util.rs +++ b/src/util.rs @@ -75,10 +75,10 @@ pub fn get_last_error() -> Error { } } -pub fn generate_password(length: usize) -> Result, Error> { +pub fn generate_password(length: usize) -> Result { let mut data = vec![0u8; length]; OsRng.fill_bytes(&mut data[..]); - Ok(data) + get_cstring(data) } pub fn get_cstring>>(s: T) -> Result { -- cgit v1.2.3 From 777cbd0fee8187325b0272d3264b535828d4b4ea Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Tue, 28 Jan 2020 19:47:39 +0100 Subject: Remove AuthenticatedDevice::temp_password_ptr We introduced the AuthenticatedDevice::temp_password_ptr function to reduce the number of casts needed in our code base. Since we switched from Vec to CString, we no longer have to cast the return value of as_ptr. Therefore we can remove the temp_password_ptr function to reduce code complexity. --- src/auth.rs | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) (limited to 'src') diff --git a/src/auth.rs b/src/auth.rs index 571e198..6748ca1 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -119,8 +119,6 @@ pub trait Authenticate<'a> { trait AuthenticatedDevice { fn new(device: T, temp_password: CString) -> Self; - - fn temp_password_ptr(&self) -> *const c_char; } /// A Nitrokey device with user authentication. @@ -235,13 +233,13 @@ impl<'a, T: Device<'a>> ops::DerefMut for User<'a, T> { impl<'a, T: Device<'a>> GenerateOtp for User<'a, T> { fn get_hotp_code(&mut self, slot: u8) -> Result { result_from_string(unsafe { - nitrokey_sys::NK_get_hotp_code_PIN(slot, self.temp_password_ptr()) + nitrokey_sys::NK_get_hotp_code_PIN(slot, self.temp_password.as_ptr()) }) } fn get_totp_code(&self, slot: u8) -> Result { result_from_string(unsafe { - nitrokey_sys::NK_get_totp_code_PIN(slot, 0, 0, 0, self.temp_password_ptr()) + nitrokey_sys::NK_get_totp_code_PIN(slot, 0, 0, 0, self.temp_password.as_ptr()) }) } } @@ -254,10 +252,6 @@ impl<'a, T: Device<'a>> AuthenticatedDevice for User<'a, T> { marker: marker::PhantomData, } } - - fn temp_password_ptr(&self) -> *const c_char { - self.temp_password.as_ptr() - } } impl<'a, T: Device<'a>> ops::Deref for Admin<'a, T> { @@ -319,7 +313,7 @@ impl<'a, T: Device<'a>> Admin<'a, T> { raw_config.scrollock, raw_config.user_password, false, - self.temp_password_ptr(), + self.temp_password.as_ptr(), ) }) } @@ -338,7 +332,7 @@ impl<'a, T: Device<'a>> ConfigureOtp for Admin<'a, T> { raw_data.use_enter, raw_data.use_token_id, raw_data.token_id.as_ptr(), - self.temp_password_ptr(), + self.temp_password.as_ptr(), ) }) } @@ -355,20 +349,20 @@ impl<'a, T: Device<'a>> ConfigureOtp for Admin<'a, T> { raw_data.use_enter, raw_data.use_token_id, raw_data.token_id.as_ptr(), - self.temp_password_ptr(), + self.temp_password.as_ptr(), ) }) } fn erase_hotp_slot(&mut self, slot: u8) -> Result<(), Error> { get_command_result(unsafe { - nitrokey_sys::NK_erase_hotp_slot(slot, self.temp_password_ptr()) + nitrokey_sys::NK_erase_hotp_slot(slot, self.temp_password.as_ptr()) }) } fn erase_totp_slot(&mut self, slot: u8) -> Result<(), Error> { get_command_result(unsafe { - nitrokey_sys::NK_erase_totp_slot(slot, self.temp_password_ptr()) + nitrokey_sys::NK_erase_totp_slot(slot, self.temp_password.as_ptr()) }) } } @@ -381,10 +375,6 @@ impl<'a, T: Device<'a>> AuthenticatedDevice for Admin<'a, T> { marker: marker::PhantomData, } } - - fn temp_password_ptr(&self) -> *const c_char { - self.temp_password.as_ptr() - } } impl<'a> Authenticate<'a> for DeviceWrapper<'a> { -- cgit v1.2.3 From ebd754d88330478981f65e4724cc561ceff4f9e7 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Tue, 28 Jan 2020 20:31:56 +0100 Subject: Regenerate temporary passwords with null bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we silently cut off temporary passwords that contained a null byte. With the change to CString, we returned a LibraryError instead. With this patch, we change to generate_password function to continue generating passwords until we have a password without a null byte. The chance of generating a password with a null byte is ca. 10 % for our temporary password with 25 characters. Therefore the chance of having to re-generate the password multiple times is low enough that we don’t bother with re-generating only the null bytes of the password for the time being. This should be improved in the future. --- CHANGELOG.md | 1 + TODO.md | 2 ++ src/util.rs | 12 +++++++++--- 3 files changed, 12 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/CHANGELOG.md b/CHANGELOG.md index be65865..5f03db2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ SPDX-License-Identifier: CC0-1.0 # Unreleased - Use `CString` to store the temporary password instead of `Vec`. +- Regenerate temporary passwords if they would contain a null byte. # v0.5.1 (2020-01-15) - Fix serial number formatting for Nitrokey Pro devices with firmware 0.8 or diff --git a/TODO.md b/TODO.md index 92d4b04..e50d354 100644 --- a/TODO.md +++ b/TODO.md @@ -6,5 +6,7 @@ SPDX-License-Identifier: CC0-1.0 - Clear passwords from memory. - Lock password safe in `PasswordSafe::drop()` (see [nitrokey-storage-firmware issue 65][]). +- Consider only regenerating the null bytes instead of the complete password in + `util::generate_password`. [nitrokey-storage-firmware issue 65]: https://github.com/Nitrokey/nitrokey-storage-firmware/issues/65 diff --git a/src/util.rs b/src/util.rs index b9b1a68..a0d0d1b 100644 --- a/src/util.rs +++ b/src/util.rs @@ -76,9 +76,15 @@ pub fn get_last_error() -> Error { } pub fn generate_password(length: usize) -> Result { - let mut data = vec![0u8; length]; - OsRng.fill_bytes(&mut data[..]); - get_cstring(data) + loop { + // Randomly generate a password until we get a string *without* null bytes. Otherwise + // the string would be cut off prematurely due to null-termination in C. + let mut data = vec![0u8; length]; + OsRng.fill_bytes(&mut data[..]); + if let Ok(s) = CString::new(data) { + return Ok(s); + } + } } pub fn get_cstring>>(s: T) -> Result { -- cgit v1.2.3