From 74d3fd334c19ae27083286cc8aff413a1cfb2d17 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Sun, 25 Apr 2021 18:05:35 +0200 Subject: [PATCH] Improve error messages for overlong input data Previously, we didn't check the length of the input data in the otp set, pws add and pws update commands. While libnitrokey does check the input and return an error if a string is too long, it cannot inform us which of the input strings is the problematic one. With this patch, we manually validate the input string lengths for these commands to improve the error messages, clearly stating which strings are problematic, how long they are and how long they can be. Fixes #161 --- CHANGELOG.md | 2 ++ src/commands.rs | 68 +++++++++++++++++++++++++++++++++++++++++ src/tests/otp.rs | 33 ++++++++++++++++++++ src/tests/pws.rs | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 181 insertions(+) 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(