From 51d0fbb73eb42325fb2a0832810fd9e1d4339743 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Wed, 29 Jan 2020 12:25:33 +0100 Subject: Update nitrokey dependency to 0.6.0 nitrokey 0.6.0 introduced the SerialNumber struct (instead of representing serial numbers as strings). We no longer have to manually format the serial number as SerialNumber implements Display. Import subrepo nitrokey/:nitrokey at 2a8ce725407f32db5ad61c37475719737c9b5c9c --- nitrocli/CHANGELOG.md | 2 +- nitrocli/Cargo.lock | 4 +- nitrocli/Cargo.toml | 2 +- nitrocli/src/commands.rs | 6 +- nitrocli/src/pinentry.rs | 4 +- nitrokey/CHANGELOG.md | 15 ++ nitrokey/Cargo.toml | 2 +- nitrokey/TODO.md | 2 + nitrokey/examples/list-devices.rs | 2 +- nitrokey/src/auth.rs | 37 ++--- nitrokey/src/config.rs | 14 +- nitrokey/src/device/mod.rs | 284 +++++++++++++++++++++++++++----------- nitrokey/src/device/storage.rs | 14 +- nitrokey/src/error.rs | 6 +- nitrokey/src/lib.rs | 4 +- nitrokey/src/util.rs | 61 +++++--- nitrokey/tests/device.rs | 15 +- 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 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` to `Result`. + - Change the type of the field `DeviceInfo.serial_number` from + `Option` to `Option`. +- 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`. +- 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 "] 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 { - fn new(device: T, temp_password: Vec) -> 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 { #[derive(Debug)] pub struct User<'a, T: Device<'a>> { device: T, - temp_password: Vec, + 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, + 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 { 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()) }) } } 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, 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 for Admin<'a, T> { - fn new(device: T, temp_password: Vec) -> 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 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 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, 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 { + // 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, /// 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, + /// The serial number of the device, or `None` if the device does not expose its serial number. + pub serial_number: Option, } 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 { +fn get_hidapi_serial_number(serial_number: &str) -> Option { 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 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; - /// 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 { - result_from_string(unsafe { nitrokey_sys::NK_device_serial_number() }) + fn get_serial_number(&self) -> Result { + 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 { - 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), /// 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 { - 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 { +pub fn run_with_string(ptr: *const c_char, op: F) -> Result +where + F: FnOnce(&str) -> Result, +{ 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 { + 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(value: T) -> Result { @@ -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, Error> { - let mut data = vec![0u8; length]; - OsRng.fill_bytes(&mut data[..]); - Ok(data) +pub fn generate_password(length: usize) -> Result { + 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 { 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 { 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) }; -- cgit v1.2.1