From 48305cc967799b8fb8524c5a3936f987a1083b04 Mon Sep 17 00:00:00 2001 From: Greg Bowyer Date: Mon, 5 Aug 2024 12:53:13 -0700 Subject: [PATCH 1/5] PIV: Support AES management keys This commit adds support for setting and getting the AES management keys, these are available in firmwars 5.4 and later, and are now the default in firmwares 5.7. The key is handled via being generic on a limit number of allowed alogrithms, using implementations of those from rust-crypto crates. Right now support in PIV MGM keys is for: * TripleDes (`0x03`) - The key type originally used * AES128 (`0x08`) - The new key type using a 128 bit key * AES192 (`0x0A`) - The new key type using a 192 bit key, this also doubles as the algorithm for firmwares 5.7 and later, where the default key is the same as the original TripleDes key. * AES256 (`0x0C`) - The new key type using a 256 bit key Suitable type aliases are provided for each of these key types. The rationale here for exposing the key as a generic type parameter is to largely use the original logic, but avoid scattered enums and provide the end user with some degree of control over the key types at compile time (it should, for instance be relatively easy make 3Des keys uncompileable). See: https://docs.yubico.com/yesdk/users-manual/application-piv/apdu/auth-mgmt.html --- Cargo.lock | 16 +++ Cargo.toml | 3 + src/lib.rs | 2 +- src/mgm.rs | 326 ++++++++++++++++++++++++++++--------------- src/piv.rs | 14 +- src/transaction.rs | 14 +- src/yubikey.rs | 47 +++---- tests/integration.rs | 86 ++++++++---- 8 files changed, 334 insertions(+), 174 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f1d542e2..7e582342 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,18 @@ # It is not intended for manual editing. version = 3 +[[package]] +name = "aes" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b169f7a6d4742236a0a00c541b845991d0ac43e546831af1249753ab4c3aa3a0" +dependencies = [ + "cfg-if", + "cipher", + "cpufeatures", + "zeroize", +] + [[package]] name = "aho-corasick" version = "1.1.2" @@ -200,6 +212,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" dependencies = [ "generic-array", + "rand_core", "typenum", ] @@ -1050,7 +1063,10 @@ dependencies = [ name = "yubikey" version = "0.8.0" dependencies = [ + "aes", "base16ct", + "cipher", + "crypto-common", "der", "des", "ecdsa", diff --git a/Cargo.toml b/Cargo.toml index ec5d52b5..13050be5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ x509-cert = { version = "0.2.5", features = [ "builder", "hazmat" ] } [dependencies] der = "0.7.1" des = "0.8" +aes = { version = "0.8.4", features = ["zeroize"] } elliptic-curve = "0.13" hex = { package = "base16ct", version = "0.2", features = ["alloc"] } hmac = "0.12" @@ -48,6 +49,8 @@ subtle = "2" uuid = { version = "1.2", features = ["v4"] } x509-cert.workspace = true zeroize = "1" +cipher = "0.4.4" +crypto-common = { version = "0.1.6", features = ["rand_core"] } [dev-dependencies] env_logger = "0.10" diff --git a/src/lib.rs b/src/lib.rs index 74cdf8cf..2f4cff1d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,7 +71,7 @@ pub use crate::{ chuid::ChuId, config::Config, error::{Error, Result}, - mgm::{MgmKey, MgmType}, + mgm::{MgmKey, MgmType, MgmKeyAlgorithm, MgmKey3Des, MgmKeyAes128, MgmKeyAes192, MgmKeyAes256}, piv::Key, policy::{PinPolicy, TouchPolicy}, reader::Context, diff --git a/src/mgm.rs b/src/mgm.rs index c8f41918..40c73ad2 100644 --- a/src/mgm.rs +++ b/src/mgm.rs @@ -32,7 +32,7 @@ use crate::{Error, Result}; use log::error; -use rand_core::{OsRng, RngCore}; +use rand_core::OsRng; use zeroize::{Zeroize, Zeroizing}; #[cfg(feature = "untested")] @@ -41,10 +41,12 @@ use crate::{ metadata::{AdminData, ProtectedData}, yubikey::YubiKey, }; -use des::{ - cipher::{generic_array::GenericArray, BlockDecrypt, BlockEncrypt, KeyInit}, - TdesEde3, +use cipher::{ + crypto_common::{Key, KeyInit}, + generic_array::GenericArray, + BlockCipher, BlockDecrypt, BlockEncrypt, }; + #[cfg(feature = "untested")] use {pbkdf2::pbkdf2_hmac, sha1::Sha1}; @@ -66,8 +68,10 @@ const CB_ADMIN_SALT: usize = 16; /// Size of a DES key const DES_LEN_DES: usize = 8; -/// Size of a 3DES key -pub(crate) const DES_LEN_3DES: usize = DES_LEN_DES * 3; +/// The default MGM key loaded for both Triple-DES and AES keys +const DEFAULT_MGM_KEY: [u8; 24] = [ + 1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8, +]; /// Number of PBKDF2 iterations to use when deriving from a password #[cfg(feature = "untested")] @@ -86,44 +90,167 @@ pub enum MgmType { Protected = 2, } +/// The algorithm used for the MGM key. +pub trait MgmKeyAlgorithm: + BlockCipher + BlockDecrypt + BlockEncrypt + KeyInit + private::Seal +{ + /// The KeySized used for this algorithm + const KEY_SIZE: u8; + + /// The algorithm ID used in APDU packets + const ALGORITHM_ID: u8; + + /// Implemented by specializations to check if the key is weak. + /// + /// Returns an error if the key is weak. + fn check_weak_key(_key: &Key) -> Result<()> { + Ok(()) + } +} + +impl MgmKeyAlgorithm for des::TdesEee3 { + const KEY_SIZE: u8 = 24; + const ALGORITHM_ID: u8 = 0x03; + + /// Is this 3DES key weak? + /// + /// This check is performed automatically when the key is instantiated to + /// ensure no such keys are used. + fn check_weak_key(key: &Key) -> Result<()> { + /// Weak and semi weak DES keys as taken from: + /// %A D.W. Davies + /// %A W.L. Price + /// %T Security for Computer Networks + /// %I John Wiley & Sons + /// %D 1984 + const WEAK_DES_KEYS: &[[u8; DES_LEN_DES]] = &[ + // weak keys + [0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01], + [0xFE, 0xFE, 0xFE, 0xFE, 0xFE, 0xFE, 0xFE, 0xFE], + [0x1F, 0x1F, 0x1F, 0x1F, 0x0E, 0x0E, 0x0E, 0x0E], + [0xE0, 0xE0, 0xE0, 0xE0, 0xF1, 0xF1, 0xF1, 0xF1], + // semi-weak keys + [0x01, 0xFE, 0x01, 0xFE, 0x01, 0xFE, 0x01, 0xFE], + [0xFE, 0x01, 0xFE, 0x01, 0xFE, 0x01, 0xFE, 0x01], + [0x1F, 0xE0, 0x1F, 0xE0, 0x0E, 0xF1, 0x0E, 0xF1], + [0xE0, 0x1F, 0xE0, 0x1F, 0xF1, 0x0E, 0xF1, 0x0E], + [0x01, 0xE0, 0x01, 0xE0, 0x01, 0xF1, 0x01, 0xF1], + [0xE0, 0x01, 0xE0, 0x01, 0xF1, 0x01, 0xF1, 0x01], + [0x1F, 0xFE, 0x1F, 0xFE, 0x0E, 0xFE, 0x0E, 0xFE], + [0xFE, 0x1F, 0xFE, 0x1F, 0xFE, 0x0E, 0xFE, 0x0E], + [0x01, 0x1F, 0x01, 0x1F, 0x01, 0x0E, 0x01, 0x0E], + [0x1F, 0x01, 0x1F, 0x01, 0x0E, 0x01, 0x0E, 0x01], + [0xE0, 0xFE, 0xE0, 0xFE, 0xF1, 0xFE, 0xF1, 0xFE], + [0xFE, 0xE0, 0xFE, 0xE0, 0xFE, 0xF1, 0xFE, 0xF1], + ]; + + let key_bytes = key.as_slice(); + + // set odd parity of key + let mut tmp = Zeroizing::new([0u8; Self::KEY_SIZE as usize]); + + for i in 0..(Self::KEY_SIZE as usize) { + // count number of set bits in byte, excluding the low-order bit - SWAR method + let mut c = key[i] & 0xFE; + + c = (c & 0x55) + ((c >> 1) & 0x55); + c = (c & 0x33) + ((c >> 2) & 0x33); + c = (c & 0x0F) + ((c >> 4) & 0x0F); + + // if count is even, set low key bit to 1, otherwise 0 + tmp[i] = (key[i] & 0xFE) | u8::from(c & 0x01 != 0x01); + } + + // check odd parity key against table by DES key block + for weak_key in WEAK_DES_KEYS.iter() { + if weak_key == &tmp[0..DES_LEN_DES] + || weak_key == &tmp[DES_LEN_DES..2 * DES_LEN_DES] + || weak_key == &tmp[2 * DES_LEN_DES..3 * DES_LEN_DES] + { + error!( + "blacklisting key '{:?}' since it's weak (with odd parity)", + &key_bytes + ); + + return Err(Error::KeyError) + } + } + + Ok(()) + } +} + +impl MgmKeyAlgorithm for aes::Aes128 { + const KEY_SIZE: u8 = 16; + const ALGORITHM_ID: u8 = 0x08; +} + +impl MgmKeyAlgorithm for aes::Aes192 { + const KEY_SIZE: u8 = 24; + const ALGORITHM_ID: u8 = 0x0A; +} + +impl MgmKeyAlgorithm for aes::Aes256 { + const KEY_SIZE: u8 = 32; + const ALGORITHM_ID: u8 = 0x0C; +} + /// Management Key (MGM). /// /// This key is used to authenticate to the management applet running on /// a YubiKey in order to perform administrative functions. /// -/// The only supported algorithm for MGM keys is 3DES. +/// The only supported algorithm for MGM keys are 3DES and AES. #[derive(Clone)] -pub struct MgmKey([u8; DES_LEN_3DES]); +pub struct MgmKey { + key: Key, + _cipher: std::marker::PhantomData, +} + +/// A Management Key (MGM) using Triple-DES +pub type MgmKey3Des = MgmKey; + +/// A Management Key (MGM) using AES-128 +pub type MgmKeyAes128 = MgmKey; -impl MgmKey { +/// A Management Key (MGM) using AES-192 +pub type MgmKeyAes192 = MgmKey; + +/// A Management Key (MGM) using AES-256 +pub type MgmKeyAes256 = MgmKey; + +impl MgmKey { /// Generate a random MGM key pub fn generate() -> Self { - let mut key_bytes = [0u8; DES_LEN_3DES]; - OsRng.fill_bytes(&mut key_bytes); - Self(key_bytes) + let key = C::generate_key(&mut OsRng); + Self { + key, + _cipher: std::marker::PhantomData, + } } /// Create an MGM key from byte slice. /// /// Returns an error if the slice is the wrong size or the key is weak. pub fn from_bytes(bytes: impl AsRef<[u8]>) -> Result { - bytes.as_ref().try_into() + let bytes = bytes.as_ref(); + if bytes.len() != C::key_size() { + return Err(Error::SizeError); + } + + let key = Key::::from_slice(bytes); + Self::new(key.clone()) } - /// Create an MGM key from the given byte array. + /// Create an MGM key from the given key. /// /// Returns an error if the key is weak. - pub fn new(key_bytes: [u8; DES_LEN_3DES]) -> Result { - if is_weak_key(&key_bytes) { - error!( - "blacklisting key '{:?}' since it's weak (with odd parity)", - &key_bytes - ); - - return Err(Error::KeyError); - } - - Ok(Self(key_bytes)) + pub fn new(key: Key) -> Result { + C::check_weak_key(&key)?; + Ok(Self { + key, + _cipher: std::marker::PhantomData, + }) } /// Get derived management key (MGM) @@ -145,7 +272,7 @@ impl MgmKey { return Err(Error::GenericError); } - let mut mgm = [0u8; DES_LEN_3DES]; + let mut mgm = vec![0u8; C::KEY_SIZE as usize]; pbkdf2_hmac::(pin, salt, ITER_MGM_PBKDF2, &mut mgm); MgmKey::from_bytes(mgm) } @@ -165,11 +292,11 @@ impl MgmKey { e })?; - if item.len() != DES_LEN_3DES { + if item.len() != C::KEY_SIZE as usize { error!( "protected data contains MGM, but is the wrong size: {} (expected {})", item.len(), - DES_LEN_3DES + C::KEY_SIZE ); return Err(Error::AuthenticationError); @@ -183,7 +310,7 @@ impl MgmKey { /// This will wipe any metadata related to derived and PIN-protected management keys. #[cfg(feature = "untested")] pub fn set_default(yubikey: &mut YubiKey) -> Result<()> { - MgmKey::default().set_manual(yubikey, false) + Self::default().set_manual(yubikey, false) } /// Configures the given YubiKey to use this management key. @@ -319,111 +446,86 @@ impl MgmKey { Ok(()) } - /// Encrypt with 3DES key - pub(crate) fn encrypt(&self, input: &[u8; DES_LEN_DES]) -> [u8; DES_LEN_DES] { - let mut output = input.to_owned(); - TdesEde3::new(GenericArray::from_slice(&self.0)) - .encrypt_block(GenericArray::from_mut_slice(&mut output)); - output + /// Return the size of the key used in management operations for a given algorithm + pub const fn key_size(&self) -> u8 { + C::KEY_SIZE } - /// Decrypt with 3DES key - pub(crate) fn decrypt(&self, input: &[u8; DES_LEN_DES]) -> [u8; DES_LEN_DES] { - let mut output = input.to_owned(); - TdesEde3::new(GenericArray::from_slice(&self.0)) + /// Given a challenge from a card, decrypt it and return the value + pub(crate) fn card_challenge(&self, challenge: &[u8]) -> Result> { + if challenge.len() != C::block_size() { + return Err(Error::SizeError); + } + + let mut output = challenge.to_owned(); + + C::new(&self.key) .decrypt_block(GenericArray::from_mut_slice(&mut output)); - output + + Ok(output) + } + + /// Checks the authentication matches the challenge and auth data + pub(crate) fn check_challenge(&self, challenge: &[u8], auth_data: &[u8]) -> Result<()> { + let mut response = challenge.to_owned(); + + if challenge.len() != C::block_size() { + return Err(Error::AuthenticationError); + } + + C::new(&self.key) + .encrypt_block(GenericArray::from_mut_slice(&mut response)); + + use subtle::ConstantTimeEq; + if response.ct_eq(auth_data).unwrap_u8() != 1 { + return Err(Error::AuthenticationError); + } + + Ok(()) + } + + /// Return the ID used to identify the key algorithm with APDU packets + pub(crate) fn algorithm_id(&self) -> u8 { + C::ALGORITHM_ID } } -impl AsRef<[u8; DES_LEN_3DES]> for MgmKey { - fn as_ref(&self) -> &[u8; DES_LEN_3DES] { - &self.0 +impl AsRef<[u8]> for MgmKey { + fn as_ref(&self) -> &[u8] { + self.key.as_ref() } } /// Default MGM key configured on all YubiKeys -impl Default for MgmKey { +impl Default for MgmKey { fn default() -> Self { - MgmKey([ - 1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8, - ]) + let key = Key::::from_slice(&DEFAULT_MGM_KEY).clone(); + Self { + key, + _cipher: std::marker::PhantomData, + } } } -impl Drop for MgmKey { +impl Drop for MgmKey { fn drop(&mut self) { - self.0.zeroize(); + self.key.zeroize(); } } -impl<'a> TryFrom<&'a [u8]> for MgmKey { +impl<'a, C: MgmKeyAlgorithm> TryFrom<&'a [u8]> for MgmKey { type Error = Error; fn try_from(key_bytes: &'a [u8]) -> Result { - Self::new(key_bytes.try_into().map_err(|_| Error::SizeError)?) + Self::from_bytes(key_bytes) } } -/// Weak and semi weak DES keys as taken from: -/// %A D.W. Davies -/// %A W.L. Price -/// %T Security for Computer Networks -/// %I John Wiley & Sons -/// %D 1984 -const WEAK_DES_KEYS: &[[u8; DES_LEN_DES]] = &[ - // weak keys - [0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01], - [0xFE, 0xFE, 0xFE, 0xFE, 0xFE, 0xFE, 0xFE, 0xFE], - [0x1F, 0x1F, 0x1F, 0x1F, 0x0E, 0x0E, 0x0E, 0x0E], - [0xE0, 0xE0, 0xE0, 0xE0, 0xF1, 0xF1, 0xF1, 0xF1], - // semi-weak keys - [0x01, 0xFE, 0x01, 0xFE, 0x01, 0xFE, 0x01, 0xFE], - [0xFE, 0x01, 0xFE, 0x01, 0xFE, 0x01, 0xFE, 0x01], - [0x1F, 0xE0, 0x1F, 0xE0, 0x0E, 0xF1, 0x0E, 0xF1], - [0xE0, 0x1F, 0xE0, 0x1F, 0xF1, 0x0E, 0xF1, 0x0E], - [0x01, 0xE0, 0x01, 0xE0, 0x01, 0xF1, 0x01, 0xF1], - [0xE0, 0x01, 0xE0, 0x01, 0xF1, 0x01, 0xF1, 0x01], - [0x1F, 0xFE, 0x1F, 0xFE, 0x0E, 0xFE, 0x0E, 0xFE], - [0xFE, 0x1F, 0xFE, 0x1F, 0xFE, 0x0E, 0xFE, 0x0E], - [0x01, 0x1F, 0x01, 0x1F, 0x01, 0x0E, 0x01, 0x0E], - [0x1F, 0x01, 0x1F, 0x01, 0x0E, 0x01, 0x0E, 0x01], - [0xE0, 0xFE, 0xE0, 0xFE, 0xF1, 0xFE, 0xF1, 0xFE], - [0xFE, 0xE0, 0xFE, 0xE0, 0xFE, 0xF1, 0xFE, 0xF1], -]; - -/// Is this 3DES key weak? -/// -/// This check is performed automatically when the key is instantiated to -/// ensure no such keys are used. -fn is_weak_key(key: &[u8; DES_LEN_3DES]) -> bool { - // set odd parity of key - let mut tmp = Zeroizing::new([0u8; DES_LEN_3DES]); - - for i in 0..DES_LEN_3DES { - // count number of set bits in byte, excluding the low-order bit - SWAR method - let mut c = key[i] & 0xFE; - - c = (c & 0x55) + ((c >> 1) & 0x55); - c = (c & 0x33) + ((c >> 2) & 0x33); - c = (c & 0x0F) + ((c >> 4) & 0x0F); - - // if count is even, set low key bit to 1, otherwise 0 - tmp[i] = (key[i] & 0xFE) | u8::from(c & 0x01 != 0x01); - } - - // check odd parity key against table by DES key block - let mut is_weak = false; - - for weak_key in WEAK_DES_KEYS.iter() { - if weak_key == &tmp[0..DES_LEN_DES] - || weak_key == &tmp[DES_LEN_DES..2 * DES_LEN_DES] - || weak_key == &tmp[2 * DES_LEN_DES..3 * DES_LEN_DES] - { - is_weak = true; - break; - } - } - - is_weak +// Seal the MgmKeyAlgorithm trait +mod private { + pub trait Seal {} + impl Seal for des::TdesEee3 {} + impl Seal for aes::Aes128 {} + impl Seal for aes::Aes192 {} + impl Seal for aes::Aes256 {} } diff --git a/src/piv.rs b/src/piv.rs index 26ae42d6..83edc9fc 100644 --- a/src/piv.rs +++ b/src/piv.rs @@ -1218,8 +1218,14 @@ fn read_public_key( pub enum ManagementAlgorithmId { /// Used on PIN and PUK slots. PinPuk, - /// Used on the key management slot. + /// Used on the key management slot to indicate the management key is TripleDes. ThreeDes, + /// Used on the key management slot to indicate the management key is AES-128. + Aes128, + /// Used on the key management slot to indicate the management key is AES-192. + Aes192, + /// Used on the key management slot to indicate the management key is AES-256. + Aes256, /// Used on all other slots. Asymmetric(AlgorithmId), } @@ -1231,6 +1237,9 @@ impl TryFrom for ManagementAlgorithmId { match value { 0xff => Ok(ManagementAlgorithmId::PinPuk), 0x03 => Ok(ManagementAlgorithmId::ThreeDes), + 0x08 => Ok(ManagementAlgorithmId::Aes128), + 0x0a => Ok(ManagementAlgorithmId::Aes192), + 0x0c => Ok(ManagementAlgorithmId::Aes256), oth => AlgorithmId::try_from(oth).map(ManagementAlgorithmId::Asymmetric), } } @@ -1241,6 +1250,9 @@ impl From for u8 { match id { ManagementAlgorithmId::PinPuk => 0xff, ManagementAlgorithmId::ThreeDes => 0x03, + ManagementAlgorithmId::Aes128 => 0x08, + ManagementAlgorithmId::Aes192 => 0x0a, + ManagementAlgorithmId::Aes256 => 0x0c, ManagementAlgorithmId::Asymmetric(oth) => oth.into(), } } diff --git a/src/transaction.rs b/src/transaction.rs index 65140ad7..b714173a 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -15,7 +15,7 @@ use log::{error, trace}; use zeroize::Zeroizing; #[cfg(feature = "untested")] -use crate::mgm::{MgmKey, DES_LEN_3DES}; +use crate::mgm::{MgmKey, MgmKeyAlgorithm}; const CB_PIN_MAX: usize = 8; @@ -244,14 +244,14 @@ impl<'tx> Transaction<'tx> { /// Set the management key (MGM). #[cfg(feature = "untested")] - pub fn set_mgm_key(&self, new_key: &MgmKey, require_touch: bool) -> Result<()> { + pub fn set_mgm_key(&self, new_key: &MgmKey, require_touch: bool) -> Result<()> { let p2 = if require_touch { 0xfe } else { 0xff }; - let mut data = [0u8; DES_LEN_3DES + 3]; - data[0] = ALGO_3DES; - data[1] = KEY_CARDMGM; - data[2] = DES_LEN_3DES as u8; - data[3..3 + DES_LEN_3DES].copy_from_slice(new_key.as_ref()); + let mut data = Vec::with_capacity(new_key.key_size() as usize + 3); + data.push(new_key.algorithm_id()); + data.push(KEY_CARDMGM); + data.push(new_key.key_size()); + data.extend_from_slice(new_key.as_ref()); let status_words = Apdu::new(Ins::SetMgmKey) .params(0xff, p2) diff --git a/src/yubikey.rs b/src/yubikey.rs index fb62bc4a..1c563705 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -36,7 +36,7 @@ use crate::{ chuid::ChuId, config::Config, error::{Error, Result}, - mgm::MgmKey, + mgm::{MgmKey, MgmKeyAlgorithm}, piv, reader::{Context, Reader}, transaction::Transaction, @@ -357,37 +357,39 @@ impl YubiKey { } /// Authenticate to the card using the provided management key (MGM). - pub fn authenticate(&mut self, mgm_key: MgmKey) -> Result<()> { + pub fn authenticate(&mut self, mgm_key: MgmKey) -> Result<()> { let txn = self.begin_transaction()?; // get a challenge from the card - let challenge = Apdu::new(Ins::Authenticate) - .params(ALGO_3DES, KEY_CARDMGM) + let card_response = Apdu::new(Ins::Authenticate) + .params(mgm_key.algorithm_id(), KEY_CARDMGM) .data([TAG_DYN_AUTH, 0x02, 0x80, 0x00]) .transmit(&txn, 261)?; - if !challenge.is_success() || challenge.data().len() < 12 { + if !card_response.is_success() { return Err(Error::AuthenticationError); } // send a response to the cards challenge and a challenge of our own. - let response = mgm_key.decrypt(challenge.data()[4..12].try_into()?); + let card_challenge = mgm_key.card_challenge(&card_response.data()[4..])?; + let challenge_len = card_challenge.len(); - let mut data = [0u8; 22]; - data[0] = TAG_DYN_AUTH; - data[1] = 20; // 2 + 8 + 2 +8 - data[2] = 0x80; - data[3] = 8; - data[4..12].copy_from_slice(&response); - data[12] = 0x81; - data[13] = 8; - OsRng.fill_bytes(&mut data[14..22]); + let mut data = Vec::with_capacity(4 + challenge_len + 2 + challenge_len); + data.push(TAG_DYN_AUTH); + data.push((2 + challenge_len + 2 + challenge_len).try_into().unwrap()); + data.push(0x80); + data.push(challenge_len as u8); + data.extend_from_slice(&card_challenge); + data.push(0x81); + data.push(challenge_len as u8); + + let mut host_challenge = vec![0u8; challenge_len]; + OsRng.fill_bytes(&mut host_challenge); - let mut challenge = [0u8; 8]; - challenge.copy_from_slice(&data[14..22]); + data.extend_from_slice(&host_challenge); let authentication = Apdu::new(Ins::Authenticate) - .params(ALGO_3DES, KEY_CARDMGM) + .params(mgm_key.algorithm_id(), KEY_CARDMGM) .data(data) .transmit(&txn, 261)?; @@ -396,14 +398,7 @@ impl YubiKey { } // compare the response from the card with our challenge - let response = mgm_key.encrypt(&challenge); - - use subtle::ConstantTimeEq; - if response.ct_eq(&authentication.data()[4..12]).unwrap_u8() != 1 { - return Err(Error::AuthenticationError); - } - - Ok(()) + mgm_key.check_challenge(&host_challenge, &authentication.data()[4..]) } /// Get the PIV keys contained in this YubiKey. diff --git a/tests/integration.rs b/tests/integration.rs index e75d0f0f..9746f9e0 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -16,7 +16,8 @@ use yubikey::{ certificate::yubikey_signer, certificate::Certificate, piv::{self, AlgorithmId, Key, ManagementSlotId, RetiredSlotId, SlotId}, - Error, MgmKey, PinPolicy, Serial, TouchPolicy, YubiKey, + Error, MgmKey, MgmKey3Des, MgmKeyAes128, MgmKeyAes192, MgmKeyAes256, MgmKeyAlgorithm, + PinPolicy, Serial, TouchPolicy, YubiKey, }; static YUBIKEY: Lazy> = Lazy::new(|| { @@ -108,6 +109,26 @@ fn test_verify_pin() { assert!(yubikey.verify_pin(b"123456").is_ok()); } +fn get_mgm_key_meta(yubikey: &mut YubiKey) -> piv::SlotMetadata { + piv::metadata(yubikey, SlotId::Management(ManagementSlotId::Management)).unwrap() +} + +/// Given a default YubiKey, authenticate with the default management key. +/// +/// This is slightly complicated by newer firmwares using AES192 as the MGM default key, +/// over 3DES. +fn auth_default_mgm(yubikey: &mut YubiKey) { + match get_mgm_key_meta(yubikey).algorithm { + piv::ManagementAlgorithmId::ThreeDes => { + assert!(yubikey.authenticate(MgmKey3Des::default()).is_ok()) + } + piv::ManagementAlgorithmId::Aes192 => { + assert!(yubikey.authenticate(MgmKeyAes192::default()).is_ok()) + } + other => panic!("unexpected management key algorithm: {:?}", other), + } +} + // // Management key support // @@ -119,29 +140,41 @@ fn test_set_mgmkey() { let mut yubikey = YUBIKEY.lock().unwrap(); assert!(yubikey.verify_pin(b"123456").is_ok()); - assert!(MgmKey::get_protected(&mut yubikey).is_err()); - assert!(yubikey.authenticate(MgmKey::default()).is_ok()); - - // Set a protected management key. - assert!(MgmKey::generate().set_protected(&mut yubikey).is_ok()); - let protected = MgmKey::get_protected(&mut yubikey).unwrap(); - assert!(yubikey.authenticate(MgmKey::default()).is_err()); - assert!(yubikey.authenticate(protected.clone()).is_ok()); - - // Set a manual management key. - let manual = MgmKey::generate(); - assert!(manual.set_manual(&mut yubikey, false).is_ok()); - assert!(MgmKey::get_protected(&mut yubikey).is_err()); - assert!(yubikey.authenticate(MgmKey::default()).is_err()); - assert!(yubikey.authenticate(protected.clone()).is_err()); - assert!(yubikey.authenticate(manual.clone()).is_ok()); - - // Set back to the default management key. - assert!(MgmKey::set_default(&mut yubikey).is_ok()); - assert!(MgmKey::get_protected(&mut yubikey).is_err()); - assert!(yubikey.authenticate(protected).is_err()); - assert!(yubikey.authenticate(manual).is_err()); - assert!(yubikey.authenticate(MgmKey::default()).is_ok()); + + fn test_mgm(yubikey: &mut YubiKey) { + assert!(yubikey.authenticate::(MgmKey::default::()).is_ok()); + assert!(MgmKey::::get_protected(yubikey).is_err()); + + // Set a protected management key. + assert!(MgmKey::generate::().set_protected(yubikey).is_ok()); + let protected = MgmKey::get_protected::(yubikey).unwrap(); + assert!(yubikey.authenticate::(MgmKey::default::()).is_err()); + assert!(yubikey.authenticate(protected.clone()).is_ok()); + + // Set a manual management key. + let manual = MgmKey::::generate(); + assert!(manual.set_manual(&mut yubikey, false).is_ok()); + assert!(MgmKey::::get_protected(&mut yubikey).is_err()); + assert!(yubikey.authenticate(MgmKey::::default()).is_err()); + assert!(yubikey.authenticate(protected.clone()).is_err()); + assert!(yubikey.authenticate(manual.clone()).is_ok()); + + // Set back to the default management key. + assert!(MgmKey::::set_default(&mut yubikey).is_ok()); + assert!(MgmKey::::get_protected(&mut yubikey).is_err()); + assert!(yubikey.authenticate(protected).is_err()); + assert!(yubikey.authenticate(manual).is_err()); + assert!(yubikey.authenticate(MgmKey::::default()).is_ok()); + } + + match get_mgm_admin(&mut yubikey).algorithm { + ManagementAlgorithmId::ThreeDes => test_mgm::(&mut yubikey), + ManagementAlgorithmId::Aes192 => test_mgm::(&mut yubikey), + ManagementAlgorithmId::Aes128 | ManagementAlgorithmId::Aes192 => { + panic!("AES128 or AES256 should not be a default key") + } + other => panic!("unexpected management key algorithm: {:?}", other), + } } // @@ -152,11 +185,10 @@ fn generate_self_signed_cert() -> Certificate { let mut yubikey = YUBIKEY.lock().unwrap(); assert!(yubikey.verify_pin(b"123456").is_ok()); - assert!(yubikey.authenticate(MgmKey::default()).is_ok()); + auth_default_mgm(&mut yubikey); let slot = SlotId::Retired(RetiredSlotId::R1); - // Generate a new key in the selected slot. let generated = piv::generate( &mut yubikey, slot, @@ -289,7 +321,7 @@ fn test_read_metadata() { let mut yubikey = YUBIKEY.lock().unwrap(); assert!(yubikey.verify_pin(b"123456").is_ok()); - assert!(yubikey.authenticate(MgmKey::default()).is_ok()); + auth_default_mgm(&mut yubikey); let slot = SlotId::Retired(RetiredSlotId::R1); From 434d2244cd018dafd3f64195faf1ff635c89901f Mon Sep 17 00:00:00 2001 From: Greg Bowyer Date: Mon, 5 Aug 2024 21:06:26 -0700 Subject: [PATCH 2/5] PIV: remove additional PIV MGM methods `Yubikey` hosts methods to do authentication with the MGM key in a one shot method, and via broken out methods (`get_auth_challenge` and `verify_auth_response`). These methods are a little hard to make work with AES or 3DES keys and currently have no integration tests. Rather than having duplicate logic (and subsequently duplicating error tests), these methods are being removed. --- src/yubikey.rs | 50 -------------------------------------------------- 1 file changed, 50 deletions(-) diff --git a/src/yubikey.rs b/src/yubikey.rs index 1c563705..c3ed08bc 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -66,9 +66,6 @@ use { /// Flag for PUK blocked pub(crate) const ADMIN_FLAGS_1_PUK_BLOCKED: u8 = 0x01; -/// 3DES authentication -pub(crate) const ALGO_3DES: u8 = 0x03; - /// Card management key pub(crate) const KEY_CARDMGM: u8 = 0x9b; @@ -628,53 +625,6 @@ impl YubiKey { txn.save_object(object_id, indata) } - /// Get an auth challenge. - #[cfg(feature = "untested")] - pub fn get_auth_challenge(&mut self) -> Result<[u8; 8]> { - let txn = self.begin_transaction()?; - - let response = Apdu::new(Ins::Authenticate) - .params(ALGO_3DES, KEY_CARDMGM) - .data([0x7c, 0x02, 0x81, 0x00]) - .transmit(&txn, 261)?; - - if !response.is_success() { - return Err(Error::AuthenticationError); - } - - Ok(response - .data() - .get(4..12) - .ok_or(Error::SizeError)? - .try_into()?) - } - - /// Verify an auth response. - #[cfg(feature = "untested")] - pub fn verify_auth_response(&mut self, response: [u8; 8]) -> Result<()> { - let mut data = [0u8; 12]; - data[0] = 0x7c; - data[1] = 0x0a; - data[2] = 0x82; - data[3] = 0x08; - data[4..12].copy_from_slice(&response); - - let txn = self.begin_transaction()?; - - // send the response to the card and a challenge of our own. - let status_words = Apdu::new(Ins::Authenticate) - .params(ALGO_3DES, KEY_CARDMGM) - .data(data) - .transmit(&txn, 261)? - .status_words(); - - if !status_words.is_success() { - return Err(Error::AuthenticationError); - } - - Ok(()) - } - /// Reset YubiKey. /// /// WARNING: this is a destructive operation which will destroy all keys! From 80a3956c8baf18687ad3d4063f8f514216aee40c Mon Sep 17 00:00:00 2001 From: "Riad S. Wahby" <716593+kwantam@users.noreply.github.com> Date: Wed, 14 Aug 2024 16:57:20 -0400 Subject: [PATCH 3/5] Fix: integration tests with `untested` feature, clippy (#1) PIV: formatting and lint improvements --- src/error.rs | 4 ++-- src/lib.rs | 2 +- src/mgm.rs | 10 ++++------ src/transaction.rs | 6 +++++- src/yubikey.rs | 16 ++++++++++------ tests/integration.rs | 37 ++++++++++++++++++------------------- 6 files changed, 40 insertions(+), 35 deletions(-) diff --git a/src/error.rs b/src/error.rs index 7774b002..2f0d35e8 100644 --- a/src/error.rs +++ b/src/error.rs @@ -192,8 +192,8 @@ impl std::error::Error for Error { } } -impl From for Error { - fn from(_err: x509_cert::der::Error) -> Error { +impl From for Error { + fn from(_err: der::Error) -> Error { Error::ParseError } } diff --git a/src/lib.rs b/src/lib.rs index 2f4cff1d..3fc43b91 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,7 +71,7 @@ pub use crate::{ chuid::ChuId, config::Config, error::{Error, Result}, - mgm::{MgmKey, MgmType, MgmKeyAlgorithm, MgmKey3Des, MgmKeyAes128, MgmKeyAes192, MgmKeyAes256}, + mgm::{MgmKey, MgmKey3Des, MgmKeyAes128, MgmKeyAes192, MgmKeyAes256, MgmKeyAlgorithm, MgmType}, piv::Key, policy::{PinPolicy, TouchPolicy}, reader::Context, diff --git a/src/mgm.rs b/src/mgm.rs index 40c73ad2..0bb2da71 100644 --- a/src/mgm.rs +++ b/src/mgm.rs @@ -92,7 +92,7 @@ pub enum MgmType { /// The algorithm used for the MGM key. pub trait MgmKeyAlgorithm: - BlockCipher + BlockDecrypt + BlockEncrypt + KeyInit + private::Seal + BlockCipher + BlockDecrypt + BlockEncrypt + Clone + KeyInit + private::Seal { /// The KeySized used for this algorithm const KEY_SIZE: u8; @@ -172,7 +172,7 @@ impl MgmKeyAlgorithm for des::TdesEee3 { &key_bytes ); - return Err(Error::KeyError) + return Err(Error::KeyError); } } @@ -459,8 +459,7 @@ impl MgmKey { let mut output = challenge.to_owned(); - C::new(&self.key) - .decrypt_block(GenericArray::from_mut_slice(&mut output)); + C::new(&self.key).decrypt_block(GenericArray::from_mut_slice(&mut output)); Ok(output) } @@ -473,8 +472,7 @@ impl MgmKey { return Err(Error::AuthenticationError); } - C::new(&self.key) - .encrypt_block(GenericArray::from_mut_slice(&mut response)); + C::new(&self.key).encrypt_block(GenericArray::from_mut_slice(&mut response)); use subtle::ConstantTimeEq; if response.ct_eq(auth_data).unwrap_u8() != 1 { diff --git a/src/transaction.rs b/src/transaction.rs index b714173a..82dadbef 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -244,7 +244,11 @@ impl<'tx> Transaction<'tx> { /// Set the management key (MGM). #[cfg(feature = "untested")] - pub fn set_mgm_key(&self, new_key: &MgmKey, require_touch: bool) -> Result<()> { + pub fn set_mgm_key( + &self, + new_key: &MgmKey, + require_touch: bool, + ) -> Result<()> { let p2 = if require_touch { 0xfe } else { 0xff }; let mut data = Vec::with_capacity(new_key.key_size() as usize + 3); diff --git a/src/yubikey.rs b/src/yubikey.rs index c3ed08bc..8956d426 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -195,8 +195,8 @@ impl YubiKey { if let Some(yk_stored) = yubikey { // We found two YubiKeys, so we won't use either. // Don't reset them. - let _ = yk_stored.disconnect(pcsc::Disposition::LeaveCard); - let _ = yk_found.disconnect(pcsc::Disposition::LeaveCard); + let _ = yk_stored.disconnect(Disposition::LeaveCard); + let _ = yk_found.disconnect(Disposition::LeaveCard); error!("multiple YubiKeys detected!"); return Err(Error::PcscError { inner: None }); @@ -243,7 +243,7 @@ impl YubiKey { return Ok(yubikey); } else { // We didn't want this YubiKey; don't reset it. - let _ = yubikey.disconnect(pcsc::Disposition::LeaveCard); + let _ = yubikey.disconnect(Disposition::LeaveCard); } } @@ -263,7 +263,7 @@ impl YubiKey { self.card.reconnect( pcsc::ShareMode::Shared, pcsc::Protocols::T1, - pcsc::Disposition::ResetCard, + Disposition::ResetCard, )?; let pin = self @@ -373,7 +373,11 @@ impl YubiKey { let mut data = Vec::with_capacity(4 + challenge_len + 2 + challenge_len); data.push(TAG_DYN_AUTH); - data.push((2 + challenge_len + 2 + challenge_len).try_into().unwrap()); + data.push( + (2 + challenge_len + 2 + challenge_len) + .try_into() + .expect("value fits in u8"), + ); data.push(0x80); data.push(challenge_len as u8); data.extend_from_slice(&card_challenge); @@ -672,7 +676,7 @@ impl<'a> TryFrom<&'a Reader<'_>> for YubiKey { // a side-effect of determining this. Avoid disrupting its internal state // any further (e.g. preserve the PIN cache of whatever applet is selected // currently). - if let Err((_, e)) = card.disconnect(pcsc::Disposition::LeaveCard) { + if let Err((_, e)) = card.disconnect(Disposition::LeaveCard) { error!("Failed to disconnect gracefully from card: {}", e); } diff --git a/tests/integration.rs b/tests/integration.rs index 9746f9e0..c545c4ec 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -12,13 +12,13 @@ use signature::hazmat::PrehashVerifier; use std::{env, str::FromStr, sync::Mutex, time::Duration}; use x509_cert::{der::Encode, name::Name, serial_number::SerialNumber, time::Validity}; use yubikey::{ - certificate, certificate::yubikey_signer, certificate::Certificate, - piv::{self, AlgorithmId, Key, ManagementSlotId, RetiredSlotId, SlotId}, - Error, MgmKey, MgmKey3Des, MgmKeyAes128, MgmKeyAes192, MgmKeyAes256, MgmKeyAlgorithm, - PinPolicy, Serial, TouchPolicy, YubiKey, + piv::{self, AlgorithmId, Key, ManagementAlgorithmId, ManagementSlotId, RetiredSlotId, SlotId}, + Error, MgmKey3Des, MgmKeyAes192, PinPolicy, Serial, TouchPolicy, YubiKey, }; +#[cfg(feature = "untested")] +use yubikey::{MgmKey, MgmKeyAlgorithm}; static YUBIKEY: Lazy> = Lazy::new(|| { // Only show logs if `RUST_LOG` is set @@ -114,15 +114,15 @@ fn get_mgm_key_meta(yubikey: &mut YubiKey) -> piv::SlotMetadata { } /// Given a default YubiKey, authenticate with the default management key. -/// +/// /// This is slightly complicated by newer firmwares using AES192 as the MGM default key, /// over 3DES. fn auth_default_mgm(yubikey: &mut YubiKey) { match get_mgm_key_meta(yubikey).algorithm { - piv::ManagementAlgorithmId::ThreeDes => { + ManagementAlgorithmId::ThreeDes => { assert!(yubikey.authenticate(MgmKey3Des::default()).is_ok()) } - piv::ManagementAlgorithmId::Aes192 => { + ManagementAlgorithmId::Aes192 => { assert!(yubikey.authenticate(MgmKeyAes192::default()).is_ok()) } other => panic!("unexpected management key algorithm: {:?}", other), @@ -142,35 +142,35 @@ fn test_set_mgmkey() { assert!(yubikey.verify_pin(b"123456").is_ok()); fn test_mgm(yubikey: &mut YubiKey) { - assert!(yubikey.authenticate::(MgmKey::default::()).is_ok()); + assert!(yubikey.authenticate::(MgmKey::default()).is_ok()); assert!(MgmKey::::get_protected(yubikey).is_err()); // Set a protected management key. - assert!(MgmKey::generate::().set_protected(yubikey).is_ok()); - let protected = MgmKey::get_protected::(yubikey).unwrap(); - assert!(yubikey.authenticate::(MgmKey::default::()).is_err()); + assert!(MgmKey::::generate().set_protected(yubikey).is_ok()); + let protected = MgmKey::::get_protected(yubikey).unwrap(); + assert!(yubikey.authenticate::(MgmKey::default()).is_err()); assert!(yubikey.authenticate(protected.clone()).is_ok()); // Set a manual management key. let manual = MgmKey::::generate(); - assert!(manual.set_manual(&mut yubikey, false).is_ok()); - assert!(MgmKey::::get_protected(&mut yubikey).is_err()); + assert!(manual.set_manual(yubikey, false).is_ok()); + assert!(MgmKey::::get_protected(yubikey).is_err()); assert!(yubikey.authenticate(MgmKey::::default()).is_err()); assert!(yubikey.authenticate(protected.clone()).is_err()); assert!(yubikey.authenticate(manual.clone()).is_ok()); // Set back to the default management key. - assert!(MgmKey::::set_default(&mut yubikey).is_ok()); - assert!(MgmKey::::get_protected(&mut yubikey).is_err()); + assert!(MgmKey::::set_default(yubikey).is_ok()); + assert!(MgmKey::::get_protected(yubikey).is_err()); assert!(yubikey.authenticate(protected).is_err()); assert!(yubikey.authenticate(manual).is_err()); assert!(yubikey.authenticate(MgmKey::::default()).is_ok()); } - match get_mgm_admin(&mut yubikey).algorithm { + match get_mgm_key_meta(&mut yubikey).algorithm { ManagementAlgorithmId::ThreeDes => test_mgm::(&mut yubikey), ManagementAlgorithmId::Aes192 => test_mgm::(&mut yubikey), - ManagementAlgorithmId::Aes128 | ManagementAlgorithmId::Aes192 => { + ManagementAlgorithmId::Aes128 | ManagementAlgorithmId::Aes256 => { panic!("AES128 or AES256 should not be a default key") } other => panic!("unexpected management key algorithm: {:?}", other), @@ -349,8 +349,7 @@ fn test_read_metadata() { #[ignore] fn test_parse_cert_from_der() { let bob_der = std::fs::read("tests/assets/Bob.der").expect(".der file not found"); - let cert = - certificate::Certificate::from_bytes(bob_der).expect("Failed to parse valid certificate"); + let cert = Certificate::from_bytes(bob_der).expect("Failed to parse valid certificate"); assert_eq!( cert.subject(), "CN=Bob", From b83cbd2ceb061ad6a8d2ed27cd754d2d7677980c Mon Sep 17 00:00:00 2001 From: kwantam Date: Wed, 14 Aug 2024 17:58:30 -0400 Subject: [PATCH 4/5] 3des should use enc-dec-enc, not enc-enc-enc --- src/mgm.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mgm.rs b/src/mgm.rs index 0bb2da71..f2a9a4fd 100644 --- a/src/mgm.rs +++ b/src/mgm.rs @@ -108,7 +108,7 @@ pub trait MgmKeyAlgorithm: } } -impl MgmKeyAlgorithm for des::TdesEee3 { +impl MgmKeyAlgorithm for des::TdesEde3 { const KEY_SIZE: u8 = 24; const ALGORITHM_ID: u8 = 0x03; @@ -208,7 +208,7 @@ pub struct MgmKey { } /// A Management Key (MGM) using Triple-DES -pub type MgmKey3Des = MgmKey; +pub type MgmKey3Des = MgmKey; /// A Management Key (MGM) using AES-128 pub type MgmKeyAes128 = MgmKey; @@ -522,7 +522,7 @@ impl<'a, C: MgmKeyAlgorithm> TryFrom<&'a [u8]> for MgmKey { // Seal the MgmKeyAlgorithm trait mod private { pub trait Seal {} - impl Seal for des::TdesEee3 {} + impl Seal for des::TdesEde3 {} impl Seal for aes::Aes128 {} impl Seal for aes::Aes192 {} impl Seal for aes::Aes256 {} From 13ee1a8567ba36e7e89fd6ae94db924aeaf20643 Mon Sep 17 00:00:00 2001 From: kwantam Date: Wed, 14 Aug 2024 18:18:01 -0400 Subject: [PATCH 5/5] ensure no oob read of card response --- src/yubikey.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/yubikey.rs b/src/yubikey.rs index 8956d426..27f62dc3 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -363,7 +363,7 @@ impl YubiKey { .data([TAG_DYN_AUTH, 0x02, 0x80, 0x00]) .transmit(&txn, 261)?; - if !card_response.is_success() { + if !card_response.is_success() || card_response.data().len() < 5 { return Err(Error::AuthenticationError); }