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

@@ -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

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;

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,

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<u8> for MgmAlgorithmId {
+    type Error = Error;
+
+    fn try_from(value: u8) -> Result<Self> {
+        match value {
+            0x03 => Ok(MgmAlgorithmId::ThreeDes),
+            _ => Err(Error::AlgorithmError),
+        }
+    }
+}
+
+impl From<MgmAlgorithmId> 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<Self> {
+        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<Self> {
-        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<Self> {
         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<Self> {
         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
-}

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
+}

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<SlotMetadata> {
     let txn = yubikey.begin_transaction()?;
-    let templ = [0, Ins::GetMetadata.code(), 0, slot.into()];
 
-    let response = txn.transfer_data(&templ, &[], CB_OBJ_MAX)?;
+    txn.get_metadata(slot)
-
-    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)
 }
 
 /// 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<Buffer> 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<Buffer> for SlotMetadata {
             |acc: Result<SlotMetadata>, 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<Buffer> 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<u8> for ManagementAlgorithmId {
+impl TryFrom<u8> for SlotAlgorithmId {
     type Error = Error;
 
     fn try_from(value: u8) -> Result<Self> {
         match value {
-            0xff => Ok(ManagementAlgorithmId::PinPuk),
+            0xff => Ok(SlotAlgorithmId::PinPuk),
-            0x03 => Ok(ManagementAlgorithmId::ThreeDes),
+            oth => MgmAlgorithmId::try_from(oth)
-            oth => AlgorithmId::try_from(oth).map(ManagementAlgorithmId::Asymmetric),
+                .map(SlotAlgorithmId::Management)
+                .or_else(|_| AlgorithmId::try_from(oth).map(SlotAlgorithmId::Asymmetric)),
         }
     }
 }
 
-impl From<ManagementAlgorithmId> for u8 {
+impl From<SlotAlgorithmId> for u8 {
-    fn from(id: ManagementAlgorithmId) -> u8 {
+    fn from(id: SlotAlgorithmId) -> u8 {
         match id {
-            ManagementAlgorithmId::PinPuk => 0xff,
+            SlotAlgorithmId::PinPuk => 0xff,
-            ManagementAlgorithmId::ThreeDes => 0x03,
+            SlotAlgorithmId::Management(oth) => oth.into(),
-            ManagementAlgorithmId::Asymmetric(oth) => oth.into(),
+            SlotAlgorithmId::Asymmetric(oth) => oth.into(),
         }
     }
 }

src/transaction.rs

@@ -168,6 +168,25 @@ impl<'tx> Transaction<'tx> {
         }
     }
 
+    /// Read metadata
+    pub(crate) fn get_metadata(&self, slot: SlotId) -> Result<piv::SlotMetadata> {
+        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 {