From 235eb6215e616d1905c624cee59218e0aa9d534f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Feb 2025 19:19:53 +0000 Subject: [PATCH] Clean up some of the management key code (#584) * mgm: Move TDES weak key checking code into a submodule * piv: Extract management key algorithm into a separate enum * mgm: Check management key algorithm when fetching from Yubikey --- CHANGELOG.md | 5 ++ src/apdu.rs | 6 ++ src/lib.rs | 2 +- src/mgm.rs | 138 ++++++++++++++++++++++----------------------- src/mgm/tdes.rs | 70 +++++++++++++++++++++++ src/piv.rs | 51 +++++++---------- src/transaction.rs | 19 +++++++ 7 files changed, 189 insertions(+), 102 deletions(-) create mode 100644 src/mgm/tdes.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ae9d4ed..f41d73e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `yubikey::certificate::SelfSigned` - `yubikey::Error::CertificateBuilder` +- `yubikey::MgmAlgorithmId` ### Changed - MSRV is now 1.81. @@ -19,6 +20,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `rsa 0.10.0-pre.3` - `sha2 0.11.0-pre.4` - `x509-cert 0.3.0-pre.0` +- `yubikey::piv`: + - `ManagementAlgorithmId` has been renamed to `SlotAlgorithmId`, and its + `ThreeDes` variant has been replaced by `SlotAlgorithmId::Management` + containing a `yubikey::MgmAlgorithmId`. ## 0.8.0 (2023-08-15) ### Added diff --git a/src/apdu.rs b/src/apdu.rs index d8942a3..890638b 100644 --- a/src/apdu.rs +++ b/src/apdu.rs @@ -82,6 +82,12 @@ impl Apdu { self } + /// Set this APDU's second parameter only + pub(crate) fn p2(&mut self, value: u8) -> &mut Self { + self.p2 = value; + self + } + /// Set both parameters for this APDU pub fn params(&mut self, p1: u8, p2: u8) -> &mut Self { self.p1 = p1; diff --git a/src/lib.rs b/src/lib.rs index 74cdf8c..89e56da 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::{MgmAlgorithmId, MgmKey, MgmType}, piv::Key, policy::{PinPolicy, TouchPolicy}, reader::Context, diff --git a/src/mgm.rs b/src/mgm.rs index 59503d8..960f49a 100644 --- a/src/mgm.rs +++ b/src/mgm.rs @@ -33,12 +33,14 @@ use crate::{Error, Result}; use log::error; use rand_core::{OsRng, RngCore}; -use zeroize::{Zeroize, Zeroizing}; +use zeroize::Zeroize; #[cfg(feature = "untested")] use crate::{ consts::{TAG_ADMIN_FLAGS_1, TAG_ADMIN_SALT, TAG_PROTECTED_MGM}, metadata::{AdminData, ProtectedData}, + piv::{ManagementSlotId, SlotAlgorithmId}, + transaction::Transaction, yubikey::YubiKey, }; use des::{ @@ -58,17 +60,15 @@ pub(crate) const APPLET_NAME: &str = "YubiKey MGMT"; #[cfg(feature = "untested")] pub(crate) const APPLET_ID: &[u8] = &[0xa0, 0x00, 0x00, 0x05, 0x27, 0x47, 0x11, 0x17]; +mod tdes; +pub(crate) use tdes::DES_LEN_3DES; +use tdes::DES_LEN_DES; + pub(crate) const ADMIN_FLAGS_1_PROTECTED_MGM: u8 = 0x02; #[cfg(feature = "untested")] 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; - /// Number of PBKDF2 iterations to use when deriving from a password #[cfg(feature = "untested")] const ITER_MGM_PBKDF2: u32 = 10000; @@ -86,6 +86,53 @@ pub enum MgmType { Protected = 2, } +/// Management key algorithm identifiers +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum MgmAlgorithmId { + /// Triple DES (3DES) in EDE mode + ThreeDes, +} + +impl TryFrom for MgmAlgorithmId { + type Error = Error; + + fn try_from(value: u8) -> Result { + match value { + 0x03 => Ok(MgmAlgorithmId::ThreeDes), + _ => Err(Error::AlgorithmError), + } + } +} + +impl From for u8 { + fn from(id: MgmAlgorithmId) -> u8 { + match id { + MgmAlgorithmId::ThreeDes => 0x03, + } + } +} + +impl MgmAlgorithmId { + /// Looks up the algorithm for the given Yubikey's current management key. + #[cfg(feature = "untested")] + fn query(txn: &Transaction<'_>) -> Result { + match txn.get_metadata(crate::piv::SlotId::Management(ManagementSlotId::Management)) { + Ok(metadata) => match metadata.algorithm { + SlotAlgorithmId::Management(alg) => Ok(alg), + // We specifically queried the management key slot; getting a known + // non-management algorithm back from the Yubikey is invalid. + _ => Err(Error::InvalidObject), + }, + // Firmware versions without `GET METADATA` only support 3DES. + Err(Error::NotSupported) => Ok(MgmAlgorithmId::ThreeDes), + // `Error::AlgorithmError` only occurs when a new algorithm is encountered. + Err(Error::AlgorithmError) => Err(Error::NotSupported), + // Raise other errors as-is. + Err(e) => Err(e), + } + } +} + /// Management Key (MGM). /// /// This key is used to authenticate to the management applet running on @@ -114,7 +161,7 @@ impl MgmKey { /// /// Returns an error if the key is weak. pub fn new(key_bytes: [u8; DES_LEN_3DES]) -> Result { - if is_weak_key(&key_bytes) { + if tdes::is_weak_key(&key_bytes) { error!( "blacklisting key '{:?}' since it's weak (with odd parity)", &key_bytes @@ -131,6 +178,12 @@ impl MgmKey { pub fn get_derived(yubikey: &mut YubiKey, pin: &[u8]) -> Result { let txn = yubikey.begin_transaction()?; + // Check the key algorithm. + let alg = MgmAlgorithmId::query(&txn)?; + if alg != MgmAlgorithmId::ThreeDes { + return Err(Error::NotSupported); + } + // recover management key let admin_data = AdminData::read(&txn)?; let salt = admin_data.get_item(TAG_ADMIN_SALT)?; @@ -155,6 +208,12 @@ impl MgmKey { pub fn get_protected(yubikey: &mut YubiKey) -> Result { let txn = yubikey.begin_transaction()?; + // Check the key algorithm. + let alg = MgmAlgorithmId::query(&txn)?; + if alg != MgmAlgorithmId::ThreeDes { + return Err(Error::NotSupported); + } + let protected_data = ProtectedData::read(&txn) .inspect_err(|e| error!("could not read protected data (err: {:?})", e))?; @@ -354,66 +413,3 @@ impl<'a> TryFrom<&'a [u8]> for MgmKey { Self::new(key_bytes.try_into().map_err(|_| Error::SizeError)?) } } - -/// 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 -} diff --git a/src/mgm/tdes.rs b/src/mgm/tdes.rs new file mode 100644 index 0000000..ba1105b --- /dev/null +++ b/src/mgm/tdes.rs @@ -0,0 +1,70 @@ +use zeroize::Zeroizing; + +/// Size of a DES key +pub(super) const DES_LEN_DES: usize = 8; + +/// Size of a 3DES key +pub(crate) const DES_LEN_3DES: usize = DES_LEN_DES * 3; + +/// 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. +pub(super) 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 +} diff --git a/src/piv.rs b/src/piv.rs index 26ae42d..e59e40c 100644 --- a/src/piv.rs +++ b/src/piv.rs @@ -45,8 +45,8 @@ use crate::{ apdu::{Ins, StatusWords}, certificate::{self, Certificate}, - consts::CB_OBJ_MAX, error::{Error, Result}, + mgm::MgmAlgorithmId, policy::{PinPolicy, TouchPolicy}, serialization::*, setting, @@ -74,6 +74,9 @@ use { #[cfg(feature = "untested")] use zeroize::Zeroizing; +#[cfg(feature = "untested")] +use crate::consts::CB_OBJ_MAX; + /// PIV Applet Name pub(crate) const APPLET_NAME: &str = "PIV"; @@ -924,28 +927,15 @@ pub fn decrypt_data( /// Read metadata pub fn metadata(yubikey: &mut YubiKey, slot: SlotId) -> Result { let txn = yubikey.begin_transaction()?; - let templ = [0, Ins::GetMetadata.code(), 0, slot.into()]; - let response = txn.transfer_data(&templ, &[], CB_OBJ_MAX)?; - - if !response.is_success() { - if response.status_words() == StatusWords::NotSupportedError { - return Err(Error::NotSupported); // Requires firmware 5.2.3 - } else { - return Err(Error::GenericError); - } - } - - let buf = Buffer::new(response.data().into()); - - SlotMetadata::try_from(buf) + txn.get_metadata(slot) } /// Metadata from a slot #[derive(Debug)] pub struct SlotMetadata { /// Algorithm / Type of key - pub algorithm: ManagementAlgorithmId, + pub algorithm: SlotAlgorithmId, /// PIN and touch policy pub policy: Option<(PinPolicy, TouchPolicy)>, /// Imported or generated key @@ -972,7 +962,7 @@ impl TryFrom for SlotMetadata { |input| Tlv::parse(input).map_err(|_| nom::Err::Error(())), || { Ok(SlotMetadata { - algorithm: ManagementAlgorithmId::PinPuk, + algorithm: SlotAlgorithmId::PinPuk, policy: None, origin: None, public: None, @@ -983,7 +973,7 @@ impl TryFrom for SlotMetadata { |acc: Result, tlv| match acc { Ok(mut metadata) => match tlv.tag { 1 => { - metadata.algorithm = ManagementAlgorithmId::try_from(tlv.value[0])?; + metadata.algorithm = SlotAlgorithmId::try_from(tlv.value[0])?; Ok(metadata) } 2 => { @@ -1015,7 +1005,7 @@ impl TryFrom for SlotMetadata { } 4 => { match metadata.algorithm { - ManagementAlgorithmId::Asymmetric(alg) => { + SlotAlgorithmId::Asymmetric(alg) => { metadata.public = Some(read_public_key(alg, tlv.value, false)?); } _ => Err(Error::ParseError)?, @@ -1215,33 +1205,34 @@ fn read_public_key( #[derive(Clone, Copy, Debug, PartialEq, Eq)] /// Algorithms as reported by the metadata command. -pub enum ManagementAlgorithmId { +pub enum SlotAlgorithmId { /// Used on PIN and PUK slots. PinPuk, /// Used on the key management slot. - ThreeDes, + Management(MgmAlgorithmId), /// Used on all other slots. Asymmetric(AlgorithmId), } -impl TryFrom for ManagementAlgorithmId { +impl TryFrom for SlotAlgorithmId { type Error = Error; fn try_from(value: u8) -> Result { match value { - 0xff => Ok(ManagementAlgorithmId::PinPuk), - 0x03 => Ok(ManagementAlgorithmId::ThreeDes), - oth => AlgorithmId::try_from(oth).map(ManagementAlgorithmId::Asymmetric), + 0xff => Ok(SlotAlgorithmId::PinPuk), + oth => MgmAlgorithmId::try_from(oth) + .map(SlotAlgorithmId::Management) + .or_else(|_| AlgorithmId::try_from(oth).map(SlotAlgorithmId::Asymmetric)), } } } -impl From for u8 { - fn from(id: ManagementAlgorithmId) -> u8 { +impl From for u8 { + fn from(id: SlotAlgorithmId) -> u8 { match id { - ManagementAlgorithmId::PinPuk => 0xff, - ManagementAlgorithmId::ThreeDes => 0x03, - ManagementAlgorithmId::Asymmetric(oth) => oth.into(), + SlotAlgorithmId::PinPuk => 0xff, + SlotAlgorithmId::Management(oth) => oth.into(), + SlotAlgorithmId::Asymmetric(oth) => oth.into(), } } } diff --git a/src/transaction.rs b/src/transaction.rs index 227bf43..727f23d 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -168,6 +168,25 @@ impl<'tx> Transaction<'tx> { } } + /// Read metadata + pub(crate) fn get_metadata(&self, slot: SlotId) -> Result { + let response = Apdu::new(Ins::GetMetadata) + .p2(slot.into()) + .transmit(self, CB_OBJ_MAX)?; + + if !response.is_success() { + if response.status_words() == StatusWords::NotSupportedError { + return Err(Error::NotSupported); // Requires firmware 5.2.3 + } else { + return Err(Error::GenericError); + } + } + + let buf = Buffer::new(response.data().into()); + + piv::SlotMetadata::try_from(buf) + } + /// Verify device PIN. pub fn verify_pin(&self, pin: &[u8]) -> Result<()> { if pin.len() > CB_PIN_MAX {