diff options
| -rw-r--r-- | nitrocli/CHANGELOG.md | 2 | ||||
| -rw-r--r-- | nitrocli/Cargo.lock | 4 | ||||
| -rw-r--r-- | nitrocli/Cargo.toml | 2 | ||||
| -rw-r--r-- | nitrocli/src/commands.rs | 6 | ||||
| -rw-r--r-- | nitrocli/src/pinentry.rs | 4 | ||||
| -rw-r--r-- | nitrokey/CHANGELOG.md | 15 | ||||
| -rw-r--r-- | nitrokey/Cargo.toml | 2 | ||||
| -rw-r--r-- | nitrokey/TODO.md | 2 | ||||
| -rw-r--r-- | nitrokey/examples/list-devices.rs | 2 | ||||
| -rw-r--r-- | nitrokey/src/auth.rs | 37 | ||||
| -rw-r--r-- | nitrokey/src/config.rs | 14 | ||||
| -rw-r--r-- | nitrokey/src/device/mod.rs | 284 | ||||
| -rw-r--r-- | nitrokey/src/device/storage.rs | 14 | ||||
| -rw-r--r-- | nitrokey/src/error.rs | 6 | ||||
| -rw-r--r-- | nitrokey/src/lib.rs | 4 | ||||
| -rw-r--r-- | nitrokey/src/util.rs | 61 | ||||
| -rw-r--r-- | nitrokey/tests/device.rs | 15 | ||||
| -rw-r--r-- | nitrokey/tests/pws.rs | 2 |
18 files changed, 309 insertions, 167 deletions
diff --git a/nitrocli/CHANGELOG.md b/nitrocli/CHANGELOG.md index 02812e0..0204e6d 100644 --- a/nitrocli/CHANGELOG.md +++ b/nitrocli/CHANGELOG.md @@ -6,7 +6,7 @@ Unreleased - Replaced `argparse` with `structopt` - Removed the `argparse` dependency - Made the --verbose and --model options global -- Bumped `nitrokey` dependency to `0.5.1` +- Bumped `nitrokey` dependency to `0.6.0` 0.3.1 diff --git a/nitrocli/Cargo.lock b/nitrocli/Cargo.lock index 5f9eacf..550da4a 100644 --- a/nitrocli/Cargo.lock +++ b/nitrocli/Cargo.lock @@ -68,7 +68,7 @@ version = "0.3.1" dependencies = [ "base32 0.4.0", "libc 0.2.66", - "nitrokey 0.5.1", + "nitrokey 0.6.0", "nitrokey-test 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", "nitrokey-test-state 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "regex 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -77,7 +77,7 @@ dependencies = [ [[package]] name = "nitrokey" -version = "0.5.1" +version = "0.6.0" dependencies = [ "lazy_static 1.4.0", "libc 0.2.66", diff --git a/nitrocli/Cargo.toml b/nitrocli/Cargo.toml index b69a2bb..d1d7ae2 100644 --- a/nitrocli/Cargo.toml +++ b/nitrocli/Cargo.toml @@ -49,7 +49,7 @@ version = "0.4.0" version = "0.2" [dependencies.nitrokey] -version = "0.5.1" +version = "0.6" [dependencies.structopt] version = "0.3.7" diff --git a/nitrocli/src/commands.rs b/nitrocli/src/commands.rs index e361509..08dad04 100644 --- a/nitrocli/src/commands.rs +++ b/nitrocli/src/commands.rs @@ -343,7 +343,7 @@ fn print_status( ctx, r#"Status: model: {model} - serial number: 0x{id} + serial number: {id} firmware version: {fwv} user retry count: {urc} admin retry count: {arc}"#, @@ -393,7 +393,7 @@ pub fn list(ctx: &mut args::ExecCtx<'_>, no_connect: bool) -> Result<()> { .map(|m| m.to_string()) .unwrap_or_else(|| "unknown".into()); let serial_number = match device_info.serial_number { - Some(serial_number) => format!("0x{}", serial_number), + Some(serial_number) => serial_number.to_string(), None => { // Storage devices do not have the serial number present in // the device information. We have to connect to them to @@ -402,7 +402,7 @@ pub fn list(ctx: &mut args::ExecCtx<'_>, no_connect: bool) -> Result<()> { "N/A".to_string() } else { let device = manager.connect_path(device_info.path.clone())?; - format!("0x{}", device.get_serial_number()?) + device.get_serial_number()?.to_string() } } }; diff --git a/nitrocli/src/pinentry.rs b/nitrocli/src/pinentry.rs index fd47657..af2b4dc 100644 --- a/nitrocli/src/pinentry.rs +++ b/nitrocli/src/pinentry.rs @@ -54,7 +54,7 @@ pub trait SecretEntry: fmt::Debug { pub struct PinEntry { pin_type: PinType, model: nitrokey::Model, - serial: String, + serial: nitrokey::SerialNumber, } impl PinEntry { @@ -127,7 +127,7 @@ impl SecretEntry for PinEntry { #[derive(Debug)] pub struct PwdEntry { model: nitrokey::Model, - serial: String, + serial: nitrokey::SerialNumber, } impl PwdEntry { diff --git a/nitrokey/CHANGELOG.md b/nitrokey/CHANGELOG.md index cba0e83..e2dd8a7 100644 --- a/nitrokey/CHANGELOG.md +++ b/nitrokey/CHANGELOG.md @@ -3,6 +3,21 @@ Copyright (C) 2019-2020 Robin Krahl <robin.krahl@ireas.org> SPDX-License-Identifier: CC0-1.0 --> +# v0.6.0 (2020-02-03) +- Add `String` value to the `Error::UnexpectedError` variant. +- Always store serial numbers as structs: + - Introduce the `SerialNumber` struct. + - Change the return type of `Device::get_serial_number` from `Result<String, + _>` to `Result<SerialNumber, _>`. + - Change the type of the field `DeviceInfo.serial_number` from + `Option<String>` to `Option<SerialNumber>`. +- Use `NK_get_status` instead of `NK_read_config` to implement the + `Device::get_config` function. + +# v0.5.2 (2020-01-28) +- Use `CString` to store the temporary password instead of `Vec<u8>`. +- 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 older in the `list_devices` function. diff --git a/nitrokey/Cargo.toml b/nitrokey/Cargo.toml index 59af067..f2d859d 100644 --- a/nitrokey/Cargo.toml +++ b/nitrokey/Cargo.toml @@ -3,7 +3,7 @@ [package] name = "nitrokey" -version = "0.5.1" +version = "0.6.0" authors = ["Robin Krahl <robin.krahl@ireas.org>"] edition = "2018" homepage = "https://code.ireas.org/nitrokey-rs/" diff --git a/nitrokey/TODO.md b/nitrokey/TODO.md index 92d4b04..e50d354 100644 --- a/nitrokey/TODO.md +++ b/nitrokey/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/nitrokey/examples/list-devices.rs b/nitrokey/examples/list-devices.rs index 47fa054..0066f8c 100644 --- a/nitrokey/examples/list-devices.rs +++ b/nitrokey/examples/list-devices.rs @@ -17,7 +17,7 @@ fn main() -> Result<(), nitrokey::Error> { let model = device.get_model(); let status = device.get_status()?; println!( - "{}\t{}\t{}\t\t\t{:08x}", + "{}\t{}\t{}\t\t\t{}", device_info.path, model, status.firmware_version, status.serial_number ); } diff --git a/nitrokey/src/auth.rs b/nitrokey/src/auth.rs index cab1021..6748ca1 100644 --- a/nitrokey/src/auth.rs +++ b/nitrokey/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,9 +118,7 @@ pub trait Authenticate<'a> { } trait AuthenticatedDevice<T> { - fn new(device: T, temp_password: Vec<u8>) -> Self; - - fn temp_password_ptr(&self) -> *const c_char; + fn new(device: T, temp_password: CString) -> Self; } /// A Nitrokey device with user authentication. @@ -134,7 +133,7 @@ trait AuthenticatedDevice<T> { #[derive(Debug)] pub struct User<'a, T: Device<'a>> { device: T, - temp_password: Vec<u8>, + temp_password: CString, marker: marker::PhantomData<&'a T>, } @@ -150,7 +149,7 @@ pub struct User<'a, T: Device<'a>> { #[derive(Debug)] pub struct Admin<'a, T: Device<'a>> { device: T, - temp_password: Vec<u8>, + temp_password: CString, marker: marker::PhantomData<&'a T>, } @@ -169,7 +168,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))), @@ -234,29 +233,25 @@ 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<String, Error> { 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<String, Error> { 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()) }) } } impl<'a, T: Device<'a>> AuthenticatedDevice<T> for User<'a, T> { - fn new(device: T, temp_password: Vec<u8>) -> Self { + fn new(device: T, temp_password: CString) -> Self { User { device, temp_password, marker: marker::PhantomData, } } - - fn temp_password_ptr(&self) -> *const c_char { - self.temp_password.as_ptr() as *const c_char - } } impl<'a, T: Device<'a>> ops::Deref for Admin<'a, T> { @@ -318,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(), ) }) } @@ -337,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(), ) }) } @@ -354,36 +349,32 @@ 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()) }) } } impl<'a, T: Device<'a>> AuthenticatedDevice<T> for Admin<'a, T> { - fn new(device: T, temp_password: Vec<u8>) -> Self { + fn new(device: T, temp_password: CString) -> Self { Admin { device, temp_password, marker: marker::PhantomData, } } - - fn temp_password_ptr(&self) -> *const c_char { - self.temp_password.as_ptr() as *const c_char - } } impl<'a> Authenticate<'a> for DeviceWrapper<'a> { diff --git a/nitrokey/src/config.rs b/nitrokey/src/config.rs index cb678d7..120a51b 100644 --- a/nitrokey/src/config.rs +++ b/nitrokey/src/config.rs @@ -83,13 +83,13 @@ impl convert::TryFrom<Config> for RawConfig { } } -impl From<[u8; 5]> for RawConfig { - fn from(data: [u8; 5]) -> Self { - RawConfig { - numlock: data[0], - capslock: data[1], - scrollock: data[2], - user_password: data[3] != 0, +impl From<&nitrokey_sys::NK_status> for RawConfig { + fn from(status: &nitrokey_sys::NK_status) -> Self { + Self { + numlock: status.config_numlock, + capslock: status.config_capslock, + scrollock: status.config_scrolllock, + user_password: status.otp_user_password, } } } diff --git a/nitrokey/src/device/mod.rs b/nitrokey/src/device/mod.rs index 0234bf0..067fdf6 100644 --- a/nitrokey/src/device/mod.rs +++ b/nitrokey/src/device/mod.rs @@ -8,18 +8,17 @@ mod wrapper; use std::convert::{TryFrom, TryInto}; use std::ffi; use std::fmt; +use std::str; -use libc; use nitrokey_sys; use crate::auth::Authenticate; use crate::config::{Config, RawConfig}; -use crate::error::{CommunicationError, Error}; +use crate::error::{CommunicationError, Error, LibraryError}; use crate::otp::GenerateOtp; use crate::pws::GetPasswordSafe; use crate::util::{ - get_command_result, get_cstring, get_last_error, owned_str_from_ptr, result_from_string, - result_or_error, + get_command_result, get_cstring, owned_str_from_ptr, result_or_error, run_with_string, }; pub use pro::Pro; @@ -71,6 +70,98 @@ impl TryFrom<nitrokey_sys::NK_device_model> for Model { } } +/// Serial number of a Nitrokey device. +/// +/// The serial number can be formatted as a string using the [`ToString`][] trait, and it can be +/// parsed from a string using the [`FromStr`][] trait. It can also be represented as a 32-bit +/// unsigned integer using [`as_u32`][]. This integer is the ID of the smartcard of the Nitrokey +/// device. +/// +/// Neither the format of the string representation nor the integer representation are guaranteed +/// to stay the same for new firmware versions. +/// +/// [`as_u32`]: #method.as_u32 +/// [`FromStr`]: #impl-FromStr +/// [`ToString`]: #impl-ToString +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct SerialNumber { + value: u32, +} + +impl SerialNumber { + /// Creates an emtpty serial number. + /// + /// This function can be used to create a placeholder value or to compare a `SerialNumber` + /// instance with an empty serial number. + pub fn empty() -> Self { + SerialNumber::new(0) + } + + fn new(value: u32) -> Self { + SerialNumber { value } + } + + /// Returns the integer reprensentation of this serial number. + /// + /// This integer currently is the ID of the smartcard of the Nitrokey device. Upcoming + /// firmware versions might change the meaning of this representation, or add additional + /// components to the serial number. + // To provide a stable API even if the internal representation of SerialNumber changes, we want + // to borrow SerialNumber instead of copying it even if it might be less efficient. + #[allow(clippy::trivially_copy_pass_by_ref)] + pub fn as_u32(&self) -> u32 { + self.value + } +} + +impl fmt::Display for SerialNumber { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:#010x}", self.value) + } +} + +impl str::FromStr for SerialNumber { + type Err = Error; + + /// Try to parse a serial number from a hex string. + /// + /// The input string must be a valid hex string. Optionally, it can include a `0x` prefix. + /// + /// # Errors + /// + /// - [`InvalidHexString`][] if the given string is not a valid hex string + /// + /// # Example + /// + /// ```no_run + /// use std::convert::TryFrom; + /// use nitrokey::{DeviceInfo, Error, SerialNumber}; + /// + /// fn find_device(serial_number: &str) -> Result<Option<DeviceInfo>, Error> { + /// let serial_number: SerialNumber = serial_number.parse()?; + /// Ok(nitrokey::list_devices()? + /// .into_iter() + /// .filter(|device| device.serial_number == Some(serial_number)) + /// .next()) + /// } + /// + /// ``` + /// + /// [`InvalidHexString`]: enum.LibraryError.html#variant.InvalidHexString + fn from_str(s: &str) -> Result<SerialNumber, Error> { + // ignore leading 0x + let hex_string = if s.starts_with("0x") { + s.split_at(2).1 + } else { + s + }; + + u32::from_str_radix(hex_string, 16) + .map(SerialNumber::new) + .map_err(|_| LibraryError::InvalidHexString.into()) + } +} + /// Connection information for a Nitrokey device. #[derive(Clone, Debug, PartialEq)] pub struct DeviceInfo { @@ -78,9 +169,8 @@ pub struct DeviceInfo { pub model: Option<Model>, /// The USB device path. pub path: String, - /// The serial number as a 8-character hex string, or `None` if the device does not expose its - /// serial number. - pub serial_number: Option<String>, + /// The serial number of the device, or `None` if the device does not expose its serial number. + pub serial_number: Option<SerialNumber>, } impl TryFrom<&nitrokey_sys::NK_device_info> for DeviceInfo { @@ -110,45 +200,42 @@ impl fmt::Display for DeviceInfo { None => write!(f, "Unsupported Nitrokey model")?, } write!(f, " at {} with ", self.path)?; - match &self.serial_number { - Some(ref serial_number) => write!(f, "serial no. {}", serial_number), + match self.serial_number { + Some(serial_number) => write!(f, "serial no. {}", serial_number), None => write!(f, "an unknown serial number"), } } } -/// Parses a serial number returned by hidapi and transforms it to the Nitrokey format. +/// Parses a serial number returned by hidapi. /// /// If the serial number is all zero, this function returns `None`. Otherwise, it uses the last -/// eight characters. If these are all zero, the first eight characters are used instead. This -/// function also makes sure that the returned string is lowercase, consistent with libnitrokey’s -/// hex string formatting. +/// eight characters. If these are all zero, the first eight characters are used instead. The +/// selected substring is parse as a hex string and its integer value is returned from the +/// function. If the string cannot be parsed, this function returns `None`. /// /// The reason for this behavior is that the Nitrokey Storage does not report its serial number at /// all (all zero value), while the Nitrokey Pro with firmware 0.9 or later writes its serial /// number to the last eight characters. Nitrokey Pro devices with firmware 0.8 or earlier wrote /// their serial number to the first eight characters. -fn get_hidapi_serial_number(serial_number: &str) -> Option<String> { +fn get_hidapi_serial_number(serial_number: &str) -> Option<SerialNumber> { let len = serial_number.len(); if len < 8 { - // The serial number in the USB descriptor has 12 bytes, we need at least four of them + // The serial number in the USB descriptor has 12 bytes, we need at least four return None; } let iter = serial_number.char_indices().rev(); let first_non_null = iter.skip_while(|(_, c)| *c == '0').next(); if let Some((i, _)) = first_non_null { - if len - i < 8 { + let substr = if len - i < 8 { // The last eight characters contain at least one non-zero character --> use them - let mut serial_number = serial_number.split_at(len - 8).1.to_string(); - serial_number.make_ascii_lowercase(); - Some(serial_number) + serial_number.split_at(len - 8).1 } else { // The last eight characters are all zero --> use the first eight - let mut serial_number = serial_number.split_at(8).0.to_string(); - serial_number.make_ascii_lowercase(); - Some(serial_number) - } + serial_number.split_at(8).0 + }; + substr.parse().ok() } else { // The serial number is all zero None @@ -176,7 +263,7 @@ pub struct Status { /// The firmware version of the device. pub firmware_version: FirmwareVersion, /// The serial number of the device. - pub serial_number: u32, + pub serial_number: SerialNumber, /// The configuration of the device. pub config: Config, } @@ -188,14 +275,8 @@ impl From<nitrokey_sys::NK_status> for Status { major: status.firmware_version_major, minor: status.firmware_version_minor, }, - serial_number: status.serial_number_smart_card, - config: RawConfig { - numlock: status.config_numlock, - capslock: status.config_capslock, - scrollock: status.config_scrolllock, - user_password: status.otp_user_password, - } - .into(), + serial_number: SerialNumber::new(status.serial_number_smart_card), + config: RawConfig::from(&status).into(), } } } @@ -263,7 +344,7 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// let device = manager.connect()?; /// let status = device.get_status()?; /// println!("Firmware version: {}", status.firmware_version); - /// println!("Serial number: {:x}", status.serial_number); + /// println!("Serial number: {}", status.serial_number); /// # Ok::<(), nitrokey::Error>(()) /// ``` /// @@ -273,8 +354,9 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// [`StorageStatus`]: struct.StorageStatus.html fn get_status(&self) -> Result<Status, Error>; - /// Returns the serial number of the Nitrokey device. The serial number is the string - /// representation of a hex number. + /// Returns the serial number of the Nitrokey device. + /// + /// For display purpuses, the serial number should be formatted as an 8-digit hex string. /// /// # Example /// @@ -292,8 +374,10 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// # Ok(()) /// # } /// ``` - fn get_serial_number(&self) -> Result<String, Error> { - result_from_string(unsafe { nitrokey_sys::NK_device_serial_number() }) + fn get_serial_number(&self) -> Result<SerialNumber, Error> { + run_with_string(unsafe { nitrokey_sys::NK_device_serial_number() }, |s| { + s.parse() + }) } /// Returns the number of remaining authentication attempts for the user. The total number of @@ -388,14 +472,17 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// # } /// ``` fn get_config(&self) -> Result<Config, Error> { - let config_ptr = unsafe { nitrokey_sys::NK_read_config() }; - if config_ptr.is_null() { - return Err(get_last_error()); - } - let config_array_ptr = config_ptr as *const [u8; 5]; - let raw_config = unsafe { RawConfig::from(*config_array_ptr) }; - unsafe { libc::free(config_ptr as *mut libc::c_void) }; - Ok(raw_config.into()) + let mut raw_status = nitrokey_sys::NK_status { + firmware_version_major: 0, + firmware_version_minor: 0, + serial_number_smart_card: 0, + config_numlock: 0, + config_capslock: 0, + config_scrolllock: 0, + otp_user_password: false, + }; + get_command_result(unsafe { nitrokey_sys::NK_get_status(&mut raw_status) })?; + Ok(RawConfig::from(&raw_status).into()) } /// Changes the administrator PIN. @@ -625,44 +712,77 @@ pub(crate) fn connect_enum(model: Model) -> bool { #[cfg(test)] mod tests { - use super::get_hidapi_serial_number; + use std::str::FromStr; + + use super::{get_hidapi_serial_number, LibraryError, SerialNumber}; #[test] - fn hidapi_serial_number() { - assert_eq!(None, get_hidapi_serial_number("")); - assert_eq!(None, get_hidapi_serial_number("00000000000000000")); - assert_eq!(None, get_hidapi_serial_number("1234")); - assert_eq!( - Some("00001234".to_string()), - get_hidapi_serial_number("00001234") - ); - assert_eq!( - Some("00001234".to_string()), - get_hidapi_serial_number("000000001234") - ); - assert_eq!( - Some("00001234".to_string()), - get_hidapi_serial_number("100000001234") - ); - assert_eq!( - Some("12340000".to_string()), - get_hidapi_serial_number("123400000000") - ); - assert_eq!( - Some("00005678".to_string()), - get_hidapi_serial_number("000000000000000000005678") - ); - assert_eq!( - Some("00001234".to_string()), - get_hidapi_serial_number("000012340000000000000000") - ); - assert_eq!( - Some("0000ffff".to_string()), - get_hidapi_serial_number("00000000000000000000FFFF") - ); - assert_eq!( - Some("0000ffff".to_string()), - get_hidapi_serial_number("00000000000000000000ffff") - ); + fn test_serial_number_display() { + fn assert_str(s: &str, n: u32) { + assert_eq!(s.to_owned(), SerialNumber::new(n).to_string()); + } + + assert_str("0x00000000", 0); + assert_str("0x00001000", 0x1000); + assert_str("0x12345678", 0x12345678); + } + + #[test] + fn test_serial_number_try_from() { + fn assert_ok(v: u32, s: &str) { + assert_eq!(SerialNumber::new(v), SerialNumber::from_str(s).unwrap()); + assert_eq!( + SerialNumber::new(v), + SerialNumber::from_str(format!("0x{}", s).as_ref()).unwrap() + ); + } + + fn assert_err(s: &str) { + match SerialNumber::from_str(s).unwrap_err() { + super::Error::LibraryError(LibraryError::InvalidHexString) => {} + err => assert!( + false, + "expected InvalidHexString error, got {} (input {})", + err, s + ), + } + } + + assert_ok(0x1234, "1234"); + assert_ok(0x1234, "01234"); + assert_ok(0x1234, "001234"); + assert_ok(0x1234, "0001234"); + + assert_ok(0, "0"); + assert_ok(0xdeadbeef, "deadbeef"); + + assert_err("deadpork"); + assert_err("blubb"); + assert_err(""); + } + + #[test] + fn test_get_hidapi_serial_number() { + fn assert_none(s: &str) { + assert_eq!(None, get_hidapi_serial_number(s)); + } + + fn assert_some(n: u32, s: &str) { + assert_eq!(Some(SerialNumber::new(n)), get_hidapi_serial_number(s)); + } + + assert_none(""); + assert_none("00000000000000000"); + assert_none("blubb"); + assert_none("1234"); + + assert_some(0x1234, "00001234"); + assert_some(0x1234, "000000001234"); + assert_some(0x1234, "100000001234"); + assert_some(0x12340000, "123400000000"); + assert_some(0x5678, "000000000000000000005678"); + assert_some(0x1234, "000012340000000000000000"); + assert_some(0xffff, "00000000000000000000FFFF"); + assert_some(0xffff, "00000000000000000000ffff"); } } diff --git a/nitrokey/src/device/storage.rs b/nitrokey/src/device/storage.rs index deb2844..5669a91 100644 --- a/nitrokey/src/device/storage.rs +++ b/nitrokey/src/device/storage.rs @@ -7,7 +7,7 @@ use std::ops; use nitrokey_sys; -use crate::device::{Device, FirmwareVersion, Model, Status}; +use crate::device::{Device, FirmwareVersion, Model, SerialNumber, Status}; use crate::error::{CommandError, Error}; use crate::otp::GenerateOtp; use crate::util::{get_command_result, get_cstring, get_last_error}; @@ -678,7 +678,7 @@ impl<'a> Storage<'a> { if usage_data.write_level_min > usage_data.write_level_max || usage_data.write_level_max > 100 { - Err(Error::UnexpectedError) + Err(Error::UnexpectedError("Invalid write levels".to_owned())) } else { Ok(ops::Range { start: usage_data.write_level_min, @@ -708,10 +708,14 @@ impl<'a> Storage<'a> { match status { 0..=100 => u8::try_from(status) .map(OperationStatus::Ongoing) - .map_err(|_| Error::UnexpectedError), + .map_err(|_| { + Error::UnexpectedError("Cannot create u8 from operation status".to_owned()) + }), -1 => Ok(OperationStatus::Idle), -2 => Err(get_last_error()), - _ => Err(Error::UnexpectedError), + _ => Err(Error::UnexpectedError( + "Invalid operation status".to_owned(), + )), } } @@ -823,7 +827,7 @@ impl<'a> Device<'a> for Storage<'a> { let storage_status = self.get_storage_status()?; status.firmware_version = storage_status.firmware_version; - status.serial_number = storage_status.serial_number_smart_card; + status.serial_number = SerialNumber::new(storage_status.serial_number_smart_card); Ok(status) } diff --git a/nitrokey/src/error.rs b/nitrokey/src/error.rs index f9af594..7bea3f2 100644 --- a/nitrokey/src/error.rs +++ b/nitrokey/src/error.rs @@ -25,7 +25,7 @@ pub enum Error { /// An error that occurred during random number generation. RandError(Box<dyn error::Error>), /// An error that is caused by an unexpected value returned by libnitrokey. - UnexpectedError, + UnexpectedError(String), /// An unknown error returned by libnitrokey. UnknownError(i64), /// An error caused by a Nitrokey model that is not supported by this crate. @@ -102,7 +102,7 @@ impl error::Error for Error { Error::LibraryError(ref err) => Some(err), Error::PoisonError(ref err) => Some(err), Error::RandError(ref err) => Some(err.as_ref()), - Error::UnexpectedError => None, + Error::UnexpectedError(_) => None, Error::UnknownError(_) => None, Error::UnsupportedModelError => None, Error::Utf8Error(ref err) => Some(err), @@ -119,7 +119,7 @@ impl fmt::Display for Error { Error::LibraryError(ref err) => write!(f, "Library error: {}", err), Error::PoisonError(_) => write!(f, "Internal error: poisoned lock"), Error::RandError(ref err) => write!(f, "RNG error: {}", err), - Error::UnexpectedError => write!(f, "An unexpected error occurred"), + Error::UnexpectedError(ref s) => write!(f, "An unexpected error occurred: {}", s), Error::UnknownError(ref err) => write!(f, "Unknown error: {}", err), Error::UnsupportedModelError => write!(f, "Unsupported Nitrokey model"), Error::Utf8Error(ref err) => write!(f, "UTF-8 error: {}", err), diff --git a/nitrokey/src/lib.rs b/nitrokey/src/lib.rs index 9efad91..92247d7 100644 --- a/nitrokey/src/lib.rs +++ b/nitrokey/src/lib.rs @@ -138,8 +138,8 @@ use nitrokey_sys; pub use crate::auth::{Admin, Authenticate, User}; pub use crate::config::Config; pub use crate::device::{ - Device, DeviceInfo, DeviceWrapper, Model, OperationStatus, Pro, SdCardData, Status, Storage, - StorageProductionInfo, StorageStatus, VolumeMode, VolumeStatus, + Device, DeviceInfo, DeviceWrapper, Model, OperationStatus, Pro, SdCardData, SerialNumber, + Status, Storage, StorageProductionInfo, StorageStatus, VolumeMode, VolumeStatus, }; pub use crate::error::{CommandError, CommunicationError, Error, LibraryError}; pub use crate::otp::{ConfigureOtp, GenerateOtp, OtpMode, OtpSlotData}; diff --git a/nitrokey/src/util.rs b/nitrokey/src/util.rs index 5a56c55..b17b071 100644 --- a/nitrokey/src/util.rs +++ b/nitrokey/src/util.rs @@ -30,26 +30,38 @@ pub enum LogLevel { DebugL2, } +pub fn str_from_ptr<'a>(ptr: *const c_char) -> Result<&'a str, Error> { + unsafe { CStr::from_ptr(ptr) }.to_str().map_err(Error::from) +} + pub fn owned_str_from_ptr(ptr: *const c_char) -> Result<String, Error> { - unsafe { CStr::from_ptr(ptr) } - .to_str() - .map(String::from) - .map_err(Error::from) + str_from_ptr(ptr).map(ToOwned::to_owned) } -pub fn result_from_string(ptr: *const c_char) -> Result<String, Error> { +pub fn run_with_string<R, F>(ptr: *const c_char, op: F) -> Result<R, Error> +where + F: FnOnce(&str) -> Result<R, Error>, +{ if ptr.is_null() { - return Err(Error::UnexpectedError); + return Err(Error::UnexpectedError( + "libnitrokey returned a null pointer".to_owned(), + )); } - let s = owned_str_from_ptr(ptr)?; + let result = str_from_ptr(ptr).and_then(op); unsafe { free(ptr as *mut c_void) }; - // An empty string can both indicate an error or be a valid return value. In this case, we - // have to check the last command status to decide what to return. - if s.is_empty() { - get_last_result().map(|_| s) - } else { - Ok(s) - } + result +} + +pub fn result_from_string(ptr: *const c_char) -> Result<String, Error> { + run_with_string(ptr, |s| { + // An empty string can both indicate an error or be a valid return value. In this case, we + // have to check the last command status to decide what to return. + if s.is_empty() { + get_last_result().map(|_| s.to_owned()) + } else { + Ok(s.to_owned()) + } + }) } pub fn result_or_error<T>(value: T) -> Result<T, Error> { @@ -69,16 +81,21 @@ pub fn get_last_result() -> Result<(), Error> { } pub fn get_last_error() -> Error { - match get_last_result() { - Ok(()) => Error::UnexpectedError, - Err(err) => err, - } + get_last_result().err().unwrap_or_else(|| { + Error::UnexpectedError("Expected an error, but command status is zero".to_owned()) + }) } -pub fn generate_password(length: usize) -> Result<Vec<u8>, Error> { - let mut data = vec![0u8; length]; - OsRng.fill_bytes(&mut data[..]); - Ok(data) +pub fn generate_password(length: usize) -> Result<CString, Error> { + 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<T: Into<Vec<u8>>>(s: T) -> Result<CString, Error> { diff --git a/nitrokey/tests/device.rs b/nitrokey/tests/device.rs index a88c956..1669d9d 100644 --- a/nitrokey/tests/device.rs +++ b/nitrokey/tests/device.rs @@ -10,7 +10,7 @@ use std::{thread, time}; use nitrokey::{ Authenticate, CommandError, CommunicationError, Config, ConfigureOtp, Device, DeviceInfo, Error, GenerateOtp, GetPasswordSafe, LibraryError, OperationStatus, OtpMode, OtpSlotData, - Storage, VolumeMode, DEFAULT_ADMIN_PIN, DEFAULT_USER_PIN, + SerialNumber, Storage, VolumeMode, DEFAULT_ADMIN_PIN, DEFAULT_USER_PIN, }; use nitrokey_test::test as test_device; @@ -47,8 +47,7 @@ fn list_devices(_device: DeviceWrapper) { nitrokey::Model::Pro => { assert!(device.serial_number.is_some()); let serial_number = device.serial_number.unwrap(); - assert!(!serial_number.is_empty()); - assert_valid_serial_number(&serial_number); + assert!(serial_number != SerialNumber::empty()); } nitrokey::Model::Storage => { assert_eq!(None, device.serial_number); @@ -157,20 +156,14 @@ fn disconnect(device: DeviceWrapper) { fn get_status(device: DeviceWrapper) { let status = unwrap_ok!(device.get_status()); assert_ok!(status.firmware_version, device.get_firmware_version()); - let serial_number = format!("{:08x}", status.serial_number); - assert_ok!(serial_number, device.get_serial_number()); + assert_ok!(status.serial_number, device.get_serial_number()); assert_ok!(status.config, device.get_config()); } -fn assert_valid_serial_number(serial_number: &str) { - assert!(serial_number.is_ascii()); - assert!(serial_number.chars().all(|c| c.is_ascii_hexdigit())); -} - #[test_device] fn get_serial_number(device: DeviceWrapper) { let serial_number = unwrap_ok!(device.get_serial_number()); - assert_valid_serial_number(&serial_number); + assert!(serial_number != SerialNumber::empty()); } #[test_device] diff --git a/nitrokey/tests/pws.rs b/nitrokey/tests/pws.rs index 7169695..47e9703 100644 --- a/nitrokey/tests/pws.rs +++ b/nitrokey/tests/pws.rs @@ -16,7 +16,7 @@ use nitrokey_test::test as test_device; fn get_slot_name_direct(slot: u8) -> Result<String, Error> { let ptr = unsafe { nitrokey_sys::NK_get_password_safe_slot_name(slot) }; if ptr.is_null() { - return Err(Error::UnexpectedError); + return Err(Error::UnexpectedError("null pointer".to_owned())); } let s = unsafe { CStr::from_ptr(ptr).to_string_lossy().into_owned() }; unsafe { free(ptr as *mut c_void) }; |