From f6915ce5df134d7cd5d0f9f0f482268b380af61a Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Sat, 7 Dec 2019 11:21:39 -0800 Subject: [PATCH] Drop YubiKey NEO support (closes #18) YubiKey NEOs are legacy YubiKey devices, most of which contain unpatchable security vulnerabilities. They have smaller buffer sizes than YK4 and YK5, which necessitates a whole bunch of conditional gating and buffer size calculations. Getting rid of them simplifies this logic and allows us to assume consistent buffer sizes everywhere. We never tested on NEOs anyway, and looking at the deleted code it seems it may have been miscalculating the NEO's buffer size! If someone *really* wants to support NEOs, it shouldn't be that hard to restore, but the codebase is definitely cleaner without it. --- README.md | 4 +-- cli/README.md | 4 +-- src/certificate.rs | 9 ++--- src/consts.rs | 22 ++++++------ src/container.rs | 3 +- src/key.rs | 3 +- src/lib.rs | 4 +-- src/metadata.rs | 9 ++--- src/mgm.rs | 5 ++- src/msroots.rs | 10 +++--- src/transaction.rs | 21 +++--------- src/yubikey.rs | 81 ++++++++------------------------------------ tests/integration.rs | 9 +++-- 13 files changed, 58 insertions(+), 126 deletions(-) diff --git a/README.md b/README.md index 09c0860b..6f02ac7c 100644 --- a/README.md +++ b/README.md @@ -40,11 +40,11 @@ endorsed by Yubico. ## Supported YubiKeys -- [YubiKey NEO] series (may be dropped in the future, see [#18]) - [YubiKey 4] series - [YubiKey 5] series -NOTE: Nano and USB-C variants of the above are also supported +NOTE: Nano and USB-C variants of the above are also supported. + Pre-YK4 [YubiKey NEO] series is **NOT** supported (see [#18]). ## Security Warning diff --git a/cli/README.md b/cli/README.md index 19f99033..1246202b 100644 --- a/cli/README.md +++ b/cli/README.md @@ -22,11 +22,11 @@ utility with general-purpose public-key encryption and signing support. ## Supported YubiKeys -- [YubiKey NEO] series (may be dropped in the future, see [#18]) - [YubiKey 4] series - [YubiKey 5] series -NOTE: Nano and USB-C variants of the above are also supported +NOTE: Nano and USB-C variants of the above are also supported. + Pre-YK4 [YubiKey NEO] series is **NOT** supported (see [#18]). ## Security Warning diff --git a/src/certificate.rs b/src/certificate.rs index ebb09df1..38709c40 100644 --- a/src/certificate.rs +++ b/src/certificate.rs @@ -145,17 +145,15 @@ impl Certificate { /// Write this certificate into the YubiKey in the given slot #[cfg(feature = "untested")] pub fn write(&self, yubikey: &mut YubiKey, slot: SlotId, certinfo: u8) -> Result<(), Error> { - let max_size = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; - write_certificate(&txn, slot, Some(&self.data), certinfo, max_size) + write_certificate(&txn, slot, Some(&self.data), certinfo) } /// Delete a certificate located at the given slot of the given YubiKey #[cfg(feature = "untested")] pub fn delete(yubikey: &mut YubiKey, slot: SlotId) -> Result<(), Error> { - let max_size = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; - write_certificate(&txn, slot, None, 0, max_size) + write_certificate(&txn, slot, None, 0) } /// Initialize a local certificate struct from the given bytebuffer @@ -244,7 +242,6 @@ pub(crate) fn write_certificate( slot: SlotId, data: Option<&[u8]>, certinfo: u8, - max_size: usize, ) -> Result<(), Error> { let mut buf = [0u8; CB_OBJ_MAX]; let mut offset = 0; @@ -261,7 +258,7 @@ pub(crate) fn write_certificate( req_len += set_length(&mut buf, data.len()); req_len += data.len(); - if req_len < data.len() || req_len > max_size { + if req_len < data.len() || req_len > CB_OBJ_MAX { return Err(Error::SizeError); } diff --git a/src/consts.rs b/src/consts.rs index 95576e6a..aa3208b6 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -39,21 +39,27 @@ pub const szLOG_SOURCE: &str = "yubikey-piv.rs"; pub const ADMIN_FLAGS_1_PUK_BLOCKED: u8 = 0x01; pub const ADMIN_FLAGS_1_PROTECTED_MGM: u8 = 0x02; +/// PIV Applet ID +pub const PIV_AID: [u8; 5] = [0xa0, 0x00, 0x00, 0x03, 0x08]; + +/// MGMT Applet ID. +/// +pub const MGMT_AID: [u8; 8] = [0xa0, 0x00, 0x00, 0x05, 0x27, 0x47, 0x11, 0x17]; + +/// YubiKey OTP Applet ID. Needed to query serial on YK4. +pub const YK_AID: [u8; 8] = [0xa0, 0x00, 0x00, 0x05, 0x27, 0x20, 0x01, 0x01]; + pub const CB_ADMIN_TIMESTAMP: usize = 0x04; pub const CB_ADMIN_SALT: usize = 16; pub const CB_ATR_MAX: usize = 33; -pub const CB_BUF_MAX_NEO: usize = 2048; -pub const CB_BUF_MAX_YK4: usize = 3072; -pub const CB_BUF_MAX: usize = CB_BUF_MAX_YK4; +pub const CB_BUF_MAX: usize = 3072; pub const CB_ECC_POINTP256: usize = 65; pub const CB_ECC_POINTP384: usize = 97; -pub const CB_OBJ_MAX_YK4: usize = CB_BUF_MAX_YK4 - 9; -pub const CB_OBJ_MAX: usize = CB_OBJ_MAX_YK4; -pub const CB_OBJ_MAX_NEO: usize = CB_BUF_MAX_NEO - 9; +pub const CB_OBJ_MAX: usize = CB_BUF_MAX - 9; pub const CB_OBJ_TAG_MIN: usize = 2; // 1 byte tag + 1 byte len pub const CB_OBJ_TAG_MAX: usize = (CB_OBJ_TAG_MIN + 2); // 1 byte tag + 3 bytes len @@ -82,9 +88,7 @@ pub const DES_LEN_3DES: usize = DES_LEN_DES * 3; // device types pub const DEVTYPE_UNKNOWN: u32 = 0x0000_0000; -pub const DEVTYPE_NEO: u32 = 0x4E45_0000; //"NE" pub const DEVTYPE_YK: u32 = 0x594B_0000; //"YK" -pub const DEVTYPE_NEOr3: u32 = (DEVTYPE_NEO | 0x0000_7233); //"r3" pub const DEVTYPE_YK4: u32 = (DEVTYPE_YK | 0x0000_0034); // "4" pub const DEVYTPE_YK5: u32 = (DEVTYPE_YK | 0x0000_0035); // "5" @@ -124,8 +128,6 @@ pub const TAG_ECC_POINT: u8 = 0x86; pub const YKPIV_ALGO_3DES: u8 = 0x03; -pub const YKPIV_ATR_NEO_R3: &[u8] = b";\xFC\x13\0\0\x811\xFE\x15YubikeyNEOr3\xE1\0"; - pub const YKPIV_CHUID_SIZE: usize = 59; pub const YKPIV_CARDID_SIZE: usize = 16; pub const YKPIV_FASCN_SIZE: usize = 25; diff --git a/src/container.rs b/src/container.rs index 56c48980..003ac2f1 100644 --- a/src/container.rs +++ b/src/container.rs @@ -107,7 +107,6 @@ impl Container { let n_containers = containers.len(); let data_len = n_containers * CONTAINER_REC_LEN; - let max_size = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; if n_containers == 0 { @@ -116,7 +115,7 @@ impl Container { let req_len = 1 + set_length(&mut buf, data_len) + data_len; - if req_len > max_size { + if req_len > CB_OBJ_MAX { return Err(Error::SizeError); } diff --git a/src/key.rs b/src/key.rs index d503eed7..e50264a7 100644 --- a/src/key.rs +++ b/src/key.rs @@ -438,8 +438,7 @@ pub fn generate( match algorithm { AlgorithmId::Rsa1024 | AlgorithmId::Rsa2048 => { - if yubikey.device_model() == DEVTYPE_YK4 - && yubikey.version.major == 4 + if yubikey.version.major == 4 && (yubikey.version.minor < 3 || yubikey.version.minor == 3 && (yubikey.version.patch < 5)) { diff --git a/src/lib.rs b/src/lib.rs index 90a48d33..65ea3ccc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,11 +18,11 @@ //! //! ## Supported YubiKeys //! -//! - [YubiKey NEO] series //! - [YubiKey 4] series //! - [YubiKey 5] series //! -//! NOTE: Nano and USB-C variants of the above are also supported +//! NOTE: Nano and USB-C variants of the above are also supported. +//! Pre-YK4 [YubiKey NEO] series is **NOT** supported. //! //! ## Supported Algorithms //! diff --git a/src/metadata.rs b/src/metadata.rs index 5cadbc54..869f068a 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -191,15 +191,10 @@ pub(crate) fn read(txn: &Transaction<'_>, tag: u8) -> Result { } /// Write metadata -pub(crate) fn write( - txn: &Transaction<'_>, - tag: u8, - data: &[u8], - max_size: usize, -) -> Result<(), Error> { +pub(crate) fn write(txn: &Transaction<'_>, tag: u8, data: &[u8]) -> Result<(), Error> { let mut buf = Zeroizing::new(vec![0u8; CB_OBJ_MAX]); - if data.len() > max_size - CB_OBJ_TAG_MAX { + if data.len() > CB_OBJ_MAX - CB_OBJ_TAG_MAX { return Err(Error::GenericError); } diff --git a/src/mgm.rs b/src/mgm.rs index c59932c9..d9e33d2c 100644 --- a/src/mgm.rs +++ b/src/mgm.rs @@ -167,7 +167,6 @@ impl MgmKey { pub fn set_protected(&self, yubikey: &mut YubiKey) -> Result<(), Error> { let mut data = Zeroizing::new(vec![0u8; CB_BUF_MAX]); - let max_size = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; txn.set_mgm_key(self, None).map_err(|e| { @@ -200,7 +199,7 @@ impl MgmKey { ) { error!("could not set protected mgm item, err = {:?}", e); } else { - metadata::write(&txn, TAG_PROTECTED, &data, max_size).map_err(|e| { + metadata::write(&txn, TAG_PROTECTED, &data).map_err(|e| { error!("could not write protected data, err = {:?}", e); e })?; @@ -247,7 +246,7 @@ impl MgmKey { &flags_1, ) { error!("could not set admin flags item, err = {}", e); - } else if let Err(e) = metadata::write(&txn, TAG_ADMIN, &data[..cb_data], max_size) { + } else if let Err(e) = metadata::write(&txn, TAG_ADMIN, &data[..cb_data]) { error!("could not write admin data, err = {}", e); } diff --git a/src/msroots.rs b/src/msroots.rs index c6911d0d..16f18462 100644 --- a/src/msroots.rs +++ b/src/msroots.rs @@ -51,11 +51,10 @@ impl MsRoots { /// Read `msroots` file from YubiKey pub fn read(yubikey: &mut YubiKey) -> Result, Error> { - let cb_data = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; // allocate first page - let mut data = Vec::with_capacity(cb_data); + let mut data = Vec::with_capacity(CB_OBJ_MAX); for object_id in YKPIV_OBJ_MSROOTS1..YKPIV_OBJ_MSROOTS5 { let buf = txn.fetch_object(object_id)?; @@ -106,7 +105,6 @@ impl MsRoots { let data = &self.0; let data_len = data.len(); let n_objs: usize; - let cb_obj_max = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; if data_len == 0 { @@ -114,7 +112,7 @@ impl MsRoots { } // Calculate number of objects required to store blob - n_objs = (data_len / (cb_obj_max - CB_OBJ_TAG_MAX)) + 1; + n_objs = (data_len / (CB_OBJ_MAX - CB_OBJ_TAG_MAX)) + 1; if n_objs > 5 { return Err(Error::SizeError); @@ -123,8 +121,8 @@ impl MsRoots { for i in 0..n_objs { offset = 0; - data_chunk = if cb_obj_max - CB_OBJ_TAG_MAX < data_len - data_offset { - cb_obj_max - CB_OBJ_TAG_MAX + data_chunk = if CB_OBJ_MAX - CB_OBJ_TAG_MAX < data_len - data_offset { + CB_OBJ_MAX - CB_OBJ_TAG_MAX } else { data_len - data_offset }; diff --git a/src/transaction.rs b/src/transaction.rs index eb0dbb3e..02a36f59 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -32,15 +32,6 @@ impl<'tx> Transaction<'tx> { }) } - /// Get an attribute of the card or card reader. - pub fn get_attribute<'buf>( - &self, - attribute: pcsc::Attribute, - buffer: &'buf mut [u8], - ) -> Result<&'buf [u8], Error> { - Ok(self.inner.get_attribute(attribute, buffer)?) - } - /// Transmit a single serialized APDU to the card this transaction is open /// with and receive a response. /// @@ -66,7 +57,7 @@ impl<'tx> Transaction<'tx> { pub fn select_application(&self) -> Result<(), Error> { let response = APDU::new(Ins::SelectApplication) .p1(0x04) - .data(&AID) + .data(&PIV_AID) .transmit(self, 0xFF) .map_err(|e| { error!("failed communicating with card: '{}'", e); @@ -102,13 +93,11 @@ impl<'tx> Transaction<'tx> { /// Get YubiKey device serial number. pub fn get_serial(&self, version: Version) -> Result { - let yk_applet = [0xa0, 0x00, 0x00, 0x05, 0x27, 0x20, 0x01, 0x01]; - let response = if version.major < 5 { - // get serial from neo/yk4 devices using the otp applet + // YK4 requires switching to the yk applet to retrieve the serial let sw = APDU::new(Ins::SelectApplication) .p1(0x04) - .data(&yk_applet) + .data(&YK_AID) .transmit(self, 0xFF)? .status_words(); @@ -130,7 +119,7 @@ impl<'tx> Transaction<'tx> { // reselect the PIV applet let sw = APDU::new(Ins::SelectApplication) .p1(0x04) - .data(&AID) + .data(&PIV_AID) .transmit(self, 0xFF)? .status_words(); @@ -141,7 +130,7 @@ impl<'tx> Transaction<'tx> { resp } else { - // get serial from yk5 and later devices using the f8 command + // YK5 implements getting the serial as a PIV applet command (0xf8) let resp = APDU::new(Ins::GetSerial).transmit(self, 0xFF)?; if !resp.is_success() { diff --git a/src/yubikey.rs b/src/yubikey.rs index fdefc194..2e619b6c 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -31,12 +31,11 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use crate::{ - consts::*, error::Error, readers::{Reader, Readers}, transaction::Transaction, }; -use log::{error, info, warn}; +use log::{error, info}; use pcsc::Card; use std::{ convert::TryFrom, @@ -46,6 +45,7 @@ use std::{ #[cfg(feature = "untested")] use crate::{ apdu::{Ins, StatusWords, APDU}, + consts::*, metadata, mgm::MgmKey, Buffer, ObjectId, @@ -60,13 +60,6 @@ use std::{ time::{SystemTime, UNIX_EPOCH}, }; -/// PIV Application ID -pub const AID: [u8; 5] = [0xa0, 0x00, 0x00, 0x03, 0x08]; - -/// MGMT Application ID. -/// -pub const MGMT_AID: [u8; 8] = [0xa0, 0x00, 0x00, 0x05, 0x27, 0x47, 0x11, 0x17]; - /// Cached YubiKey PIN pub type CachedPin = secrecy::SecretVec; @@ -116,6 +109,12 @@ impl Version { } } +impl Display for Version { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}.{}.{}", self.major, self.minor, self.patch) + } +} + /// YubiKey Device: this is the primary API for opening a session and /// performing various operations. /// @@ -126,7 +125,6 @@ impl Version { pub struct YubiKey { pub(crate) card: Card, pub(crate) pin: Option, - pub(crate) is_neo: bool, pub(crate) version: Version, pub(crate) serial: Serial, } @@ -202,18 +200,6 @@ impl YubiKey { self.serial } - /// Get YubiKey device model - // TODO(tarcieri): use an emum for this - #[cfg(feature = "untested")] - pub fn device_model(&self) -> u32 { - if self.is_neo { - DEVTYPE_NEOr3 - } else { - // TODO(tarcieri): YK5? - DEVTYPE_YK4 - } - } - /// Authenticate to the card using the provided management key (MGM). #[cfg(feature = "untested")] pub fn authenticate(&mut self, mgm_key: MgmKey) -> Result<(), Error> { @@ -368,7 +354,6 @@ impl YubiKey { #[cfg(feature = "untested")] pub fn set_pin_last_changed(yubikey: &mut YubiKey) -> Result<(), Error> { let mut data = [0u8; CB_BUF_MAX]; - let max_size = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; let buffer = metadata::read(&txn, TAG_ADMIN)?; @@ -394,7 +379,7 @@ impl YubiKey { e })?; - metadata::write(&txn, TAG_ADMIN, &data, max_size).map_err(|e| { + metadata::write(&txn, TAG_ADMIN, &data).map_err(|e| { error!("could not write admin data, err = {}", e); e })?; @@ -422,7 +407,6 @@ impl YubiKey { let mut tries_remaining: i32 = -1; let mut flags = [0]; - let max_size = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; while tries_remaining != 0 { @@ -473,7 +457,7 @@ impl YubiKey { ) .is_ok() { - if metadata::write(&txn, TAG_ADMIN, &data[..cb_data], max_size).is_err() { + if metadata::write(&txn, TAG_ADMIN, &data[..cb_data]).is_err() { error!("could not write admin metadata"); } } else { @@ -565,16 +549,6 @@ impl YubiKey { Ok(()) } - - /// Get max object size supported by this device - #[cfg(feature = "untested")] - pub(crate) fn obj_size_max(&self) -> usize { - if self.is_neo { - CB_OBJ_MAX_NEO - } else { - CB_OBJ_MAX - } - } } impl<'a> TryFrom<&'a Reader<'_>> for YubiKey { @@ -588,43 +562,18 @@ impl<'a> TryFrom<&'a Reader<'_>> for YubiKey { info!("connected to reader: {}", reader.name()); - let mut is_neo = false; - let version: Version; - let serial: Serial; - - { + let (version, serial) = { let txn = Transaction::new(&mut card)?; - let mut atr_buf = [0; CB_ATR_MAX]; - let atr = txn.get_attribute(pcsc::Attribute::AtrString, &mut atr_buf)?; - if atr == YKPIV_ATR_NEO_R3 { - is_neo = true; - } - txn.select_application()?; - // now that the PIV application is selected, retrieve the version - // and serial number. Previously the NEO/YK4 required switching - // to the yk applet to retrieve the serial, YK5 implements this - // as a PIV applet command. Unfortunately, this change requires - // that we retrieve the version number first, so that get_serial - // can determine how to get the serial number, which for the NEO/Yk4 - // will result in another selection of the PIV applet. - - version = txn.get_version().map_err(|e| { - warn!("failed to retrieve version: '{}'", e); - e - })?; - - serial = txn.get_serial(version).map_err(|e| { - warn!("failed to retrieve serial number: '{}'", e); - e - })?; - } + let v = txn.get_version()?; + let s = txn.get_serial(v)?; + (v, s) + }; let yubikey = YubiKey { card, pin: None, - is_neo, version, serial, }; diff --git a/tests/integration.rs b/tests/integration.rs index b0407b31..78b17a3f 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -4,6 +4,7 @@ #![warn(missing_docs, rust_2018_idioms, trivial_casts, unused_qualifications)] use lazy_static::lazy_static; +use log::trace; use std::{env, sync::Mutex}; use yubikey_piv::{key::Key, YubiKey}; @@ -19,7 +20,11 @@ fn init_yubikey() -> Mutex { env_logger::builder().format_timestamp(None).init(); } - Mutex::new(YubiKey::open().unwrap()) + let mut yubikey = YubiKey::open().unwrap(); + trace!("serial: {}", yubikey.serial()); + trace!("version: {}", yubikey.version()); + + Mutex::new(yubikey) } #[test] @@ -36,5 +41,5 @@ fn test_list_keys() { let mut yubikey = YUBIKEY.lock().unwrap(); let keys_result = Key::list(&mut yubikey); assert!(keys_result.is_ok()); - dbg!(keys_result.unwrap()); + trace!("keys: {:?}", keys_result.unwrap()); }