From 44b8c57a6f8701c50b179e482deca79a9e4e7acb Mon Sep 17 00:00:00 2001 From: Daniel Mueller Date: Fri, 11 Jan 2019 19:45:59 -0800 Subject: Isolate cached PINs for multiple devices from each other The application supports multiple devices both plugged in at the same time as well as when used after the other. However, the GPG cache ID we use for storing and retrieving the respective PIN is effectively a constant. This constraint can cause problems when devices have different PINs, as the PIN of a previously plugged in device may be reused for an operation on a different one. To resolve this problem this change adds the respective device's model and serial number to the cache ID. As each serial number is supposed to be different, this will ensure that the correct PIN is used for each device. With this change we also show the model and serial number of the currently used device in the pinentry dialog. Note that because we do not store the serial numbers of all previously plugged in devices, the pin clear command will only clear the PIN for the currently plugged in device. If a user wants to make sure that a cached PIN is cleared, the pin clear command should be invoked before unplugging the device. --- nitrocli/CHANGELOG.md | 2 + nitrocli/doc/nitrocli.1 | 6 ++- nitrocli/doc/nitrocli.1.pdf | Bin 14794 -> 15579 bytes nitrocli/src/args.rs | 2 +- nitrocli/src/commands.rs | 88 ++++++++++++++++++++++++++------------------ nitrocli/src/pinentry.rs | 86 +++++++++++++++++++++++++++++-------------- 6 files changed, 118 insertions(+), 66 deletions(-) diff --git a/nitrocli/CHANGELOG.md b/nitrocli/CHANGELOG.md index 6236a77..fcb126e 100644 --- a/nitrocli/CHANGELOG.md +++ b/nitrocli/CHANGELOG.md @@ -1,5 +1,7 @@ Unreleased ---------- +- Store cached PINs on a per-device basis to better support multi-device + scenarios - Further decreased binary size by using system allocator diff --git a/nitrocli/doc/nitrocli.1 b/nitrocli/doc/nitrocli.1 index 9537439..dbbaf2b 100644 --- a/nitrocli/doc/nitrocli.1 +++ b/nitrocli/doc/nitrocli.1 @@ -1,4 +1,4 @@ -.TH NITROCLI 1 2019-01-10 +.TH NITROCLI 1 2019-01-11 .SH NAME nitrocli \- access Nitrokey devices .SH SYNOPSIS @@ -201,7 +201,9 @@ reset using \fBgpg\fR(1). Use the \fBstatus\fR command to check the retry counters. .TP .B nitrocli pin clear -Clear the PINs cached by the other commands. +Clear the PINs cached by the other commands. Note that cached PINs are +associated with the device they belong to and the \fBclear\fR command will only +clear the PIN for the currently used device, not all others. .TP \fBnitrocli pin set \fItype\fR Change a PIN. diff --git a/nitrocli/doc/nitrocli.1.pdf b/nitrocli/doc/nitrocli.1.pdf index f93e3b7..a823db0 100644 Binary files a/nitrocli/doc/nitrocli.1.pdf and b/nitrocli/doc/nitrocli.1.pdf differ diff --git a/nitrocli/src/args.rs b/nitrocli/src/args.rs index f4e0758..b15ca65 100644 --- a/nitrocli/src/args.rs +++ b/nitrocli/src/args.rs @@ -631,7 +631,7 @@ fn pin_clear(ctx: &mut ExecCtx<'_>, args: Vec) -> Result<()> { parser.set_description("Clears the cached PINs"); parse(ctx, &parser, args)?; - commands::pin_clear() + commands::pin_clear(ctx) } /// Change a PIN. diff --git a/nitrocli/src/commands.rs b/nitrocli/src/commands.rs index 209fb8f..00a594f 100644 --- a/nitrocli/src/commands.rs +++ b/nitrocli/src/commands.rs @@ -81,13 +81,18 @@ fn get_storage_device(ctx: &mut args::ExecCtx<'_>) -> Result } /// Open the password safe on the given device. -fn get_password_safe<'dev>( +fn get_password_safe<'dev, D>( ctx: &mut args::ExecCtx<'_>, - device: &'dev dyn Device, -) -> Result> { + device: &'dev D, +) -> Result> +where + D: Device, +{ + let pin_entry = pinentry::PinEntry::from(pinentry::PinType::User, device)?; + try_with_pin_and_data( ctx, - pinentry::PinType::User, + &pin_entry, "Could not access the password safe", (), |_, pin| device.get_password_safe(pin).map_err(|err| ((), err)), @@ -108,7 +113,9 @@ where D: Device, F: Fn(D, &str) -> result::Result, { - try_with_pin_and_data(ctx, pin_type, msg, device, op) + let pin_entry = pinentry::PinEntry::from(pin_type, &device)?; + + try_with_pin_and_data(ctx, &pin_entry, msg, device, op) } /// Authenticate the given device with the user PIN. @@ -167,7 +174,7 @@ fn get_volume_status(status: &nitrokey::VolumeStatus) -> &'static str { /// second or third try, it will call `op` with the data returned by the /// previous call to `op`. fn try_with_pin_and_data_with_pinentry( - pin_type: pinentry::PinType, + pin_entry: &pinentry::PinEntry, msg: &'static str, data: D, op: F, @@ -179,12 +186,12 @@ where let mut retry = 3; let mut error_msg = None; loop { - let pin = pinentry::inquire_pin(pin_type, pinentry::Mode::Query, error_msg)?; + let pin = pinentry::inquire_pin(pin_entry, pinentry::Mode::Query, error_msg)?; match op(data, &pin) { Ok(result) => return Ok(result), Err((new_data, err)) => match err { nitrokey::CommandError::WrongPassword => { - pinentry::clear_pin(pin_type)?; + pinentry::clear_pin(pin_entry)?; retry -= 1; if retry > 0 { @@ -203,7 +210,7 @@ where /// Try to execute the given function with a PIN. fn try_with_pin_and_data( ctx: &mut args::ExecCtx<'_>, - pin_type: pinentry::PinType, + pin_entry: &pinentry::PinEntry, msg: &'static str, data: D, op: F, @@ -211,7 +218,7 @@ fn try_with_pin_and_data( where F: Fn(D, &str) -> result::Result, { - let pin = match pin_type { + let pin = match pin_entry.pin_type() { pinentry::PinType::Admin => &ctx.admin_pin, pinentry::PinType::User => &ctx.user_pin, }; @@ -225,7 +232,7 @@ where })?; op(data, &pin).map_err(|(_, err)| get_error(msg, err)) } else { - try_with_pin_and_data_with_pinentry(pin_type, msg, data, op) + try_with_pin_and_data_with_pinentry(pin_entry, msg, data, op) } } @@ -235,14 +242,14 @@ where /// it refrains from passing any data to it. fn try_with_pin( ctx: &mut args::ExecCtx<'_>, - pin_type: pinentry::PinType, + pin_entry: &pinentry::PinEntry, msg: &'static str, op: F, ) -> Result<()> where F: Fn(&str) -> result::Result<(), nitrokey::CommandError>, { - try_with_pin_and_data(ctx, pin_type, msg, (), |data, pin| { + try_with_pin_and_data(ctx, pin_entry, msg, (), |data, pin| { op(pin).map_err(|err| (data, err)) }) } @@ -287,12 +294,11 @@ pub fn status(ctx: &mut args::ExecCtx<'_>) -> Result<()> { /// Open the encrypted volume on the nitrokey. pub fn storage_open(ctx: &mut args::ExecCtx<'_>) -> Result<()> { let device = get_storage_device(ctx)?; - try_with_pin( - ctx, - pinentry::PinType::User, - "Opening encrypted volume failed", - |pin| device.enable_encrypted_volume(&pin), - ) + let pin_entry = pinentry::PinEntry::from(pinentry::PinType::User, &device)?; + + try_with_pin(ctx, &pin_entry, "Opening encrypted volume failed", |pin| { + device.enable_encrypted_volume(&pin) + }) } /// Close the previously opened encrypted volume. @@ -573,9 +579,14 @@ pub fn otp_status(ctx: &mut args::ExecCtx<'_>, all: bool) -> Result<()> { } /// Clear the PIN stored by various operations. -pub fn pin_clear() -> Result<()> { - pinentry::clear_pin(pinentry::PinType::Admin)?; - pinentry::clear_pin(pinentry::PinType::User)?; +pub fn pin_clear(ctx: &mut args::ExecCtx<'_>) -> Result<()> { + let device = get_device(ctx)?; + + pinentry::clear_pin(&pinentry::PinEntry::from( + pinentry::PinType::Admin, + &device, + )?)?; + pinentry::clear_pin(&pinentry::PinEntry::from(pinentry::PinType::User, &device)?)?; Ok(()) } @@ -594,14 +605,14 @@ fn check_pin(pin_type: pinentry::PinType, pin: &str) -> Result<()> { } } -fn choose_pin_with_pinentry(pin_type: pinentry::PinType) -> Result { - pinentry::clear_pin(pin_type)?; - let new_pin = pinentry::inquire_pin(pin_type, pinentry::Mode::Choose, None)?; - pinentry::clear_pin(pin_type)?; - check_pin(pin_type, &new_pin)?; +fn choose_pin_with_pinentry(pin_entry: &pinentry::PinEntry) -> Result { + pinentry::clear_pin(pin_entry)?; + let new_pin = pinentry::inquire_pin(pin_entry, pinentry::Mode::Choose, None)?; + pinentry::clear_pin(pin_entry)?; + check_pin(pin_entry.pin_type(), &new_pin)?; - let confirm_pin = pinentry::inquire_pin(pin_type, pinentry::Mode::Confirm, None)?; - pinentry::clear_pin(pin_type)?; + let confirm_pin = pinentry::inquire_pin(pin_entry, pinentry::Mode::Confirm, None)?; + pinentry::clear_pin(pin_entry)?; if new_pin != confirm_pin { Err(Error::from("Entered PINs do not match")) @@ -616,10 +627,10 @@ fn choose_pin_with_pinentry(pin_type: pinentry::PinType) -> Result { /// given PIN type, it will be used. fn choose_pin( ctx: &mut args::ExecCtx<'_>, - pin_type: pinentry::PinType, + pin_entry: &pinentry::PinEntry, new: bool, ) -> Result { - let new_pin = match pin_type { + let new_pin = match pin_entry.pin_type() { pinentry::PinType::Admin => { if new { &ctx.new_admin_pin @@ -642,17 +653,19 @@ fn choose_pin( .ok_or_else(|| Error::from("Failed to read PIN: invalid Unicode data found")) .map(ToOwned::to_owned) } else { - choose_pin_with_pinentry(pin_type) + choose_pin_with_pinentry(pin_entry) } } /// Change a PIN. pub fn pin_set(ctx: &mut args::ExecCtx<'_>, pin_type: pinentry::PinType) -> Result<()> { let device = get_device(ctx)?; - let new_pin = choose_pin(ctx, pin_type, true)?; + let pin_entry = pinentry::PinEntry::from(pin_type, &device)?; + let new_pin = choose_pin(ctx, &pin_entry, true)?; + try_with_pin( ctx, - pin_type, + &pin_entry, "Could not change the PIN", |current_pin| match pin_type { pinentry::PinType::Admin => device.change_admin_pin(¤t_pin, &new_pin), @@ -664,10 +677,13 @@ pub fn pin_set(ctx: &mut args::ExecCtx<'_>, pin_type: pinentry::PinType) -> Resu /// Unblock and reset the user PIN. pub fn pin_unblock(ctx: &mut args::ExecCtx<'_>) -> Result<()> { let device = get_device(ctx)?; - let user_pin = choose_pin(ctx, pinentry::PinType::User, false)?; + let pin_entry = pinentry::PinEntry::from(pinentry::PinType::User, &device)?; + let user_pin = choose_pin(ctx, &pin_entry, false)?; + let pin_entry = pinentry::PinEntry::from(pinentry::PinType::Admin, &device)?; + try_with_pin( ctx, - pinentry::PinType::Admin, + &pin_entry, "Could not unblock the user PIN", |admin_pin| device.unlock_user_pin(&admin_pin, &user_pin), ) diff --git a/nitrocli/src/pinentry.rs b/nitrocli/src/pinentry.rs index d54a182..1eecdd0 100644 --- a/nitrocli/src/pinentry.rs +++ b/nitrocli/src/pinentry.rs @@ -46,34 +46,66 @@ impl str::FromStr for PinType { } } -impl PinType { - fn cache_id(self) -> &'static str { - match self { - PinType::Admin => "nitrocli:admin", - PinType::User => "nitrocli:user", +#[derive(Debug)] +pub struct PinEntry { + pin_type: PinType, + model: nitrokey::Model, + serial: String, +} + +impl PinEntry { + pub fn from(pin_type: PinType, device: &D) -> crate::Result + where + D: nitrokey::Device, + { + let model = device.get_model(); + let serial = device.get_serial_number()?; + Ok(Self { + pin_type, + model, + serial, + }) + } + + fn cache_id(&self) -> String { + let model = self.model.to_string().to_lowercase(); + let suffix = format!("{}:{}", model, self.serial); + + match self.pin_type { + PinType::Admin => format!("nitrocli:admin:{}", suffix), + PinType::User => format!("nitrocli:user:{}", suffix), } } - fn prompt(self) -> &'static str { - match self { + fn prompt(&self) -> &'static str { + match self.pin_type { PinType::Admin => "Admin PIN", PinType::User => "User PIN", } } - fn description(self, mode: Mode) -> &'static str { - match self { - PinType::Admin => match mode { - Mode::Choose => "Please enter a new admin PIN", - Mode::Confirm => "Please confirm the new admin PIN", - Mode::Query => "Please enter the admin PIN", + fn description(&self, mode: Mode) -> String { + format!( + "{} for\rNitrokey {} {}", + match self.pin_type { + PinType::Admin => match mode { + Mode::Choose => "Please enter a new admin PIN", + Mode::Confirm => "Please confirm the new admin PIN", + Mode::Query => "Please enter the admin PIN", + }, + PinType::User => match mode { + Mode::Choose => "Please enter a new user PIN", + Mode::Confirm => "Please confirm the new user PIN", + Mode::Query => "Please enter the user PIN", + }, }, - PinType::User => match mode { - Mode::Choose => "Please enter a new user PIN", - Mode::Confirm => "Please confirm the new user PIN", - Mode::Query => "Please enter the user PIN", - }, - } + self.model, + self.serial, + ) + } + + pub fn pin_type(&self) -> PinType { + self.pin_type } } @@ -134,18 +166,18 @@ where /// description and to decide whether a quality bar is shown in the /// dialog. pub fn inquire_pin( - pin_type: PinType, + pin_entry: &PinEntry, mode: Mode, error_msg: Option<&str>, ) -> Result { - let cache_id = pin_type.cache_id(); + let cache_id = pin_entry.cache_id(); let error_msg = error_msg .map(|msg| msg.replace(" ", "+")) .unwrap_or_else(|| String::from("+")); - let prompt = pin_type.prompt().replace(" ", "+"); - let description = pin_type.description(mode).replace(" ", "+"); + let prompt = pin_entry.prompt().replace(" ", "+"); + let description = pin_entry.description(mode).replace(" ", "+"); - let args = vec![cache_id, &error_msg, &prompt, &description].join(" "); + let args = vec![cache_id, error_msg, prompt, description].join(" "); let mut command = "GET_PASSPHRASE --data ".to_string(); if mode.show_quality_bar() { command += "--qualitybar "; @@ -178,9 +210,9 @@ where Err(Error::Error(format!("Unexpected response: {}", string))) } -/// Clear the cached pin of the given type. -pub fn clear_pin(pin_type: PinType) -> Result<(), Error> { - let command = "CLEAR_PASSPHRASE ".to_string() + pin_type.cache_id(); +/// Clear the cached pin represented by the given entry. +pub fn clear_pin(pin_entry: &PinEntry) -> Result<(), Error> { + let command = format!("CLEAR_PASSPHRASE {}", pin_entry.cache_id()); let output = process::Command::new("gpg-connect-agent") .arg(command) .arg("/bye") -- cgit v1.2.3