diff --git a/CHANGELOG.md b/CHANGELOG.md index 32c89bd2..0a47dd15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ Unreleased - Added the `--only-aes-key` option to the `reset` command to build a new AES key without performing a factory reset - Added support for reading PWS passwords and OTP secrets from stdin +- Changed the `otp set`, `pws add` and `pws update` commands to check the + length of the input data to improve the error messages - Added `NITROCLI_RESOLVED_USB_PATH` environment variable to be used by extensions - Allowed entering of `base32` encoded strings containing spaces diff --git a/src/commands.rs b/src/commands.rs index 6863db78..82ab2f74 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -35,6 +35,12 @@ use crate::Context; const NITROCLI_EXT_PREFIX: &str = "nitrocli-"; +const OTP_NAME_LENGTH: usize = 15; + +const PWS_NAME_LENGTH: usize = 11; +const PWS_LOGIN_LENGTH: usize = 32; +const PWS_PASSWORD_LENGTH: usize = 20; + /// Set `libnitrokey`'s log level based on the execution context's verbosity. fn set_log_level(ctx: &mut Context<'_>) { let log_lvl = match ctx.config.verbosity { @@ -391,6 +397,42 @@ fn value_or_stdin<'s>(ctx: &mut Context<'_>, s: &'s str) -> anyhow::Result anyhow::Result<()> { + let mut invalid_strings = Vec::new(); + for (label, value, max_length) in data { + let length = value.as_bytes().len(); + if length > *max_length { + invalid_strings.push((label, length, max_length)); + } + } + match invalid_strings.len() { + 0 => Ok(()), + 1 => { + let (label, length, max_length) = invalid_strings[0]; + Err(anyhow::anyhow!( + "The provided {} is too long (actual length: {} bytes, maximum length: {} bytes)", + label, + length, + max_length + )) + } + _ => { + let mut msg = String::from("Multiple provided strings are too long:"); + for (label, length, max_length) in invalid_strings { + msg.push_str(&format!( + "\n {} (actual length: {} bytes, maximum length: {} bytes)", + label, length, max_length + )); + } + Err(anyhow::anyhow!(msg)) + } + } +} + /// Query and pretty print the status that is common to all Nitrokey devices. fn print_status(ctx: &mut Context<'_>, device: &nitrokey::DeviceWrapper<'_>) -> anyhow::Result<()> { let serial_number = device @@ -846,8 +888,14 @@ fn prepare_secret( /// Configure a one-time password slot on the Nitrokey device. pub fn otp_set(ctx: &mut Context<'_>, args: args::OtpSetArgs) -> anyhow::Result<()> { + // Ideally, we would also like to verify the length of the secret. But the maximum length is + // determined by the firmware version of the device and we don't want to run an additional + // command just to determine the firmware version. + ensure_string_lengths(&[("slot name", &args.name, OTP_NAME_LENGTH)])?; + let secret = value_or_stdin(ctx, &args.secret)?; let secret = prepare_secret(secret, args.format)?; + let data = nitrokey::OtpSlotData::new(args.slot, args.name, secret, args.digits.into()); let (algorithm, counter, time_window) = (args.algorithm, args.counter, args.time_window); with_device(ctx, |ctx, device| { @@ -1044,6 +1092,24 @@ pub fn pws_get( }) } +fn ensure_pws_string_lengths( + name: Option<&str>, + login: Option<&str>, + password: Option<&str>, +) -> anyhow::Result<()> { + let mut data = Vec::new(); + if let Some(name) = name { + data.push(("slot name", name, PWS_NAME_LENGTH)); + } + if let Some(login) = login { + data.push(("login", login, PWS_LOGIN_LENGTH)); + } + if let Some(password) = password { + data.push(("password", password, PWS_PASSWORD_LENGTH)); + } + ensure_string_lengths(&data) +} + /// Add a new PWS slot. pub fn pws_add( ctx: &mut Context<'_>, @@ -1053,6 +1119,7 @@ pub fn pws_add( slot_idx: Option, ) -> anyhow::Result<()> { let password = value_or_stdin(ctx, password)?; + ensure_pws_string_lengths(Some(name), Some(login), Some(&password))?; with_password_safe(ctx, |ctx, mut pws| { let slots = pws.get_slots()?; @@ -1103,6 +1170,7 @@ pub fn pws_update( } let password = password.map(|s| value_or_stdin(ctx, s)).transpose()?; + ensure_pws_string_lengths(name, login, password.as_deref())?; with_password_safe(ctx, |_ctx, mut pws| { let slot = pws.get_slot(slot_idx).context("Failed to query PWS slot")?; diff --git a/src/tests/otp.rs b/src/tests/otp.rs index 008b4145..6d407b54 100644 --- a/src/tests/otp.rs +++ b/src/tests/otp.rs @@ -34,6 +34,39 @@ fn set_invalid_slot(model: nitrokey::Model) { assert_eq!(err, "Failed to write OTP slot"); } +#[test_device] +fn set_overlong_name(model: nitrokey::Model) { + let err = Nitrocli::new() + .model(model) + .handle(&["otp", "set", "0", "1234567890123456", "1234"]) + .unwrap_err() + .to_string(); + assert_eq!( + err, + "The provided slot name is too long (actual length: 16 bytes, maximum length: 15 bytes)" + ); + + let err = Nitrocli::new() + .model(model) + .handle(&["otp", "set", "0", "รถ23456789012345", "1234"]) + .unwrap_err() + .to_string(); + assert_eq!( + err, + "The provided slot name is too long (actual length: 16 bytes, maximum length: 15 bytes)" + ); + + let err = Nitrocli::new() + .model(model) + .handle(&["otp", "set", "0", "1234567890123456789012345", "1234"]) + .unwrap_err() + .to_string(); + assert_eq!( + err, + "The provided slot name is too long (actual length: 25 bytes, maximum length: 15 bytes)" + ); +} + #[test_device] fn status(model: nitrokey::Model) -> anyhow::Result<()> { let re = regex::Regex::new( diff --git a/src/tests/pws.rs b/src/tests/pws.rs index 4f9006de..8aadc9be 100644 --- a/src/tests/pws.rs +++ b/src/tests/pws.rs @@ -47,6 +47,84 @@ fn add_invalid_slot(model: nitrokey::Model) { assert_eq!(err, "Encountered invalid slot index: 100"); } +#[test_device] +fn add_overlong_data(model: nitrokey::Model) { + let err = Nitrocli::new() + .model(model) + .handle(&[ + "pws", + "add", + "--slot", + "1", + "123456789012", + "123456789012345678901234567890123", + "123456789012345678901", + ]) + .unwrap_err() + .to_string(); + assert_eq!( + err, + "Multiple provided strings are too long: + slot name (actual length: 12 bytes, maximum length: 11 bytes) + login (actual length: 33 bytes, maximum length: 32 bytes) + password (actual length: 21 bytes, maximum length: 20 bytes)" + ); + + let err = Nitrocli::new() + .model(model) + .handle(&[ + "pws", + "add", + "--slot", + "1", + "123456789012", + "12345678901234567890123456789012", + "12345678901234567890", + ]) + .unwrap_err() + .to_string(); + assert_eq!( + err, + "The provided slot name is too long (actual length: 12 bytes, maximum length: 11 bytes)" + ); +} + +#[test_device] +fn update_overlong_data(model: nitrokey::Model) { + let err = Nitrocli::new() + .model(model) + .handle(&[ + "pws", + "update", + "1", + "--name", + "123456789012", + "--login", + "123456789012345678901234567890123", + "--password", + "123456789012345678901", + ]) + .unwrap_err() + .to_string(); + assert_eq!( + err, + "Multiple provided strings are too long: + slot name (actual length: 12 bytes, maximum length: 11 bytes) + login (actual length: 33 bytes, maximum length: 32 bytes) + password (actual length: 21 bytes, maximum length: 20 bytes)" + ); + + let err = Nitrocli::new() + .model(model) + .handle(&["pws", "update", "1", "--name", "123456789012"]) + .unwrap_err() + .to_string(); + assert_eq!( + err, + "The provided slot name is too long (actual length: 12 bytes, maximum length: 11 bytes)" + ); +} + #[test_device] fn status(model: nitrokey::Model) -> anyhow::Result<()> { let re = regex::Regex::new(