From bd485eb91298b87c3d5585f2f5f06d9f8a2a5695 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Thu, 21 Nov 2019 09:05:32 -0800 Subject: [PATCH] Clean up APDU construction with builder API Changes the `APDU` struct into a builder for serialized APDU messages. This makes APDU construction safer and more idiomatic, and also caught a few bugs in the process (missing templ from the C translation). --- src/apdu.rs | 85 +++++++++++-- src/util.rs | 3 +- src/yubikey.rs | 323 ++++++++++++++++++++++--------------------------- 3 files changed, 223 insertions(+), 188 deletions(-) diff --git a/src/apdu.rs b/src/apdu.rs index 52065fa8..bdf36014 100644 --- a/src/apdu.rs +++ b/src/apdu.rs @@ -1,7 +1,13 @@ //! Application Protocol Data Unit (APDU) use std::fmt::{self, Debug}; -use zeroize::Zeroize; +use zeroize::{Zeroize, Zeroizing}; + +/// Size of a serialized APDU (5 byte header + 255 bytes data) +pub const APDU_SIZE: usize = 260; + +/// Buffer type (self-zeroizing byte vector) +pub(crate) type Buffer = Zeroizing>; /// Application Protocol Data Unit (APDU). /// @@ -10,30 +16,83 @@ use zeroize::Zeroize; #[derive(Clone)] pub struct APDU { /// Instruction class - indicates the type of command, e.g. interindustry or proprietary - pub cla: u8, + cla: u8, /// Instruction code - indicates the specific command, e.g. "write data" - pub ins: u8, + ins: u8, /// Instruction parameter 1 for the command, e.g. offset into file at which to write the data - pub p1: u8, + p1: u8, /// Instruction parameter 2 for the command - pub p2: u8, + p2: u8, /// Length of command - encodes the number of bytes of command data to follow - pub lc: u8, + lc: u8, /// Command data - pub data: [u8; 255], + data: [u8; 255], } impl APDU { - /// Get a mut pointer to this APDU - // TODO(tarcieri): eliminate pointers and use all safe references - pub(crate) fn as_mut_ptr(&mut self) -> *mut APDU { + /// Create a new APDU with the given instruction code + pub fn new(ins: u8) -> Self { + let mut apdu = Self::default(); + apdu.ins = ins; + apdu + } + + /// Set this APDU's class + pub fn cla(&mut self, value: u8) -> &mut Self { + self.cla = value; + self + } + + /// Set this APDU's first parameter only + pub fn p1(&mut self, value: u8) -> &mut Self { + self.p1 = value; + self + } + + /// Set both parameters for this APDU + pub fn params(&mut self, p1: u8, p2: u8) -> &mut Self { + self.p1 = p1; + self.p2 = p2; self } + + /// Set the command data for this APDU. + /// + /// Panics if the byte slice is more than 255 bytes! + pub fn data(&mut self, bytes: impl AsRef<[u8]>) -> &mut Self { + assert_eq!(self.lc, 0, "APDU command already set!"); + + let bytes = bytes.as_ref(); + + assert!( + bytes.len() <= self.data.len(), + "APDU command data too large: {}-bytes", + bytes.len() + ); + + self.lc = bytes.len() as u8; + self.data[..bytes.len()].copy_from_slice(bytes); + + self + } + + /// Consume this APDU and return a self-zeroizing buffer + pub fn to_bytes(&self) -> Buffer { + let mut bytes = Vec::with_capacity(APDU_SIZE); + bytes.push(self.cla); + bytes.push(self.ins); + bytes.push(self.p1); + bytes.push(self.p2); + bytes.push(self.lc); + bytes.extend_from_slice(self.data.as_ref()); + + Zeroizing::new(bytes) + } } impl Debug for APDU { @@ -64,6 +123,12 @@ impl Default for APDU { } } +impl Drop for APDU { + fn drop(&mut self) { + self.zeroize(); + } +} + impl Zeroize for APDU { fn zeroize(&mut self) { self.cla.zeroize(); diff --git a/src/util.rs b/src/util.rs index 80a4bf76..3613bb7d 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1607,12 +1607,13 @@ pub unsafe fn ykpiv_util_set_protected_mgm( if ykrc.is_err() { // if set_mgmkey fails with KeyError, it means the generated key is weak // otherwise, log a warning, since the device mgm key is corrupt or we're in - // a yubikey where we can't set the mgm key + // a state where we can't set the mgm key if Err(Error::KeyError) != ykrc { error!( "could not set new derived mgm key, err = {}", ykrc.as_ref().unwrap_err() ); + let _ = yubikey._ykpiv_end_transaction(); return ykrc; } diff --git a/src/yubikey.rs b/src/yubikey.rs index 8a650fde..b1cd0897 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -42,7 +42,7 @@ use crate::{ use getrandom::getrandom; use libc::{c_char, free, malloc, memcmp, memcpy, memmove, memset, strlen, strncasecmp}; use log::{error, info, trace, warn}; -use std::{convert::TryInto, ffi::CStr, mem, os::raw::c_void, ptr, slice}; +use std::{ffi::CStr, os::raw::c_void, ptr, slice}; use zeroize::Zeroize; extern "C" { @@ -90,7 +90,7 @@ extern "C" { fn SCardTransmit( hCard: i32, pioSendPci: *const c_void, - pbSendBuffer: *mut c_char, + pbSendBuffer: *const c_char, cbSendLength: u32, pioRecvPci: *const c_void, pbRecvBuffer: *mut u8, @@ -197,18 +197,13 @@ impl YubiKey { let mut recv_len = data.len() as u32; let mut sw = 0i32; - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_SELECT_APPLICATION; - apdu.p1 = 0x04; - apdu.lc = AID.len() as u8; + let apdu = APDU::new(YKPIV_INS_SELECT_APPLICATION) + .p1(0x04) + .data(&AID) + .to_bytes(); - memcpy( - apdu.data.as_mut_ptr() as *mut c_void, - AID.as_ptr() as *const c_void, - AID.len(), - ); - - if let Err(e) = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw) { + if let Err(e) = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw) + { error!("failed communicating with card: \'{}\'", e); return Err(e); } @@ -537,37 +532,33 @@ impl YubiKey { let mut data = [0u8; 261]; recv_len = data.len() as u32; - let mut apdu = APDU::default(); - apdu.cla = *templ; - apdu.ins = *templ.offset(1); - apdu.p1 = *templ.offset(2); - apdu.p2 = *templ.offset(3); - - if in_ptr.offset(0xff) < in_data.offset(in_len) { - apdu.cla = 0x10; + let cla = if in_ptr.offset(0xff) < in_data.offset(in_len) { + 0x10 } else { this_size = in_data.offset(in_len) as usize - in_ptr as usize; - } + *templ + }; trace!("going to send {} bytes in this go", this_size); - apdu.lc = this_size.try_into().unwrap(); - memcpy( - apdu.data.as_mut_ptr() as *mut c_void, - in_ptr as *const c_void, - this_size, - ); + let apdu = APDU::new(*templ.offset(1)) + .cla(cla) + .params(*templ.offset(2), *templ.offset(3)) + .data(slice::from_raw_parts(in_ptr, this_size)) + .to_bytes(); - res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, sw); + res = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, sw); if res.is_err() { _currentBlock = 24; break; } + if *sw != SW_SUCCESS && (*sw >> 8 != 0x61) { _currentBlock = 24; break; } + if (*out_len) .wrapping_add(recv_len as (usize)) .wrapping_sub(2usize) @@ -610,9 +601,9 @@ impl YubiKey { *sw & 0xff ); - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_GET_RESPONSE_APDU; - res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, sw); + let apdu = APDU::new(YKPIV_INS_GET_RESPONSE_APDU).to_bytes(); + + res = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, sw); if res.is_err() { _currentBlock = 24; @@ -686,20 +677,21 @@ impl YubiKey { /// Send data pub(crate) unsafe fn _send_data( &mut self, - apdu: &mut APDU, + apdu: impl AsRef<[u8]>, data: *mut u8, recv_len: *mut u32, sw: *mut i32, ) -> Result<(), Error> { - let send_len = apdu.lc as u32 + 5; + let send_len = apdu.as_ref().len() as u32; + let apdu_ptr = apdu.as_ref().as_ptr(); let mut tmp_len = *recv_len; - trace!("> {:?}", apdu); + trace!("> {:?}", apdu.as_ref()); let rc = SCardTransmit( self.card, SCARD_PCI_T1, - apdu.as_mut_ptr() as *mut i8, + apdu_ptr as *const i8, send_len, ptr::null(), data, @@ -729,9 +721,6 @@ impl YubiKey { &mut self, key: Option<&[u8; DES_LEN_3DES]>, ) -> Result<(), Error> { - let mut data = [0u8; 261]; - let mut recv_len = data.len() as u32; - let mut sw: i32 = 0; let mut res = Ok(()); self._ykpiv_begin_transaction()?; @@ -741,16 +730,16 @@ impl YubiKey { let mgm_key = DesKey::from_bytes(*key.unwrap_or(DEFAULT_AUTH_KEY)); // get a challenge from the card - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_AUTHENTICATE; - apdu.p1 = YKPIV_ALGO_3DES; // triple des - apdu.p2 = YKPIV_KEY_CARDMGM; // management key - apdu.lc = 0x04; - apdu.data[0] = 0x7c; - apdu.data[1] = 0x02; - apdu.data[2] = 0x80; + let apdu = APDU::new(YKPIV_INS_AUTHENTICATE) + .params(YKPIV_ALGO_3DES, YKPIV_KEY_CARDMGM) + .data(&[0x7c, 0x02, 0x80, 0x00]) + .to_bytes(); - res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw); + let mut data = [0u8; 261]; + let mut recv_len = data.len() as u32; + let mut sw: i32 = 0; + + res = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw); if res.is_err() { let _ = self._ykpiv_end_transaction(); @@ -768,28 +757,30 @@ impl YubiKey { des_decrypt(&mgm_key, &challenge, &mut response); recv_len = data.len() as u32; - apdu = APDU::default(); - apdu.ins = YKPIV_INS_AUTHENTICATE; - apdu.p1 = YKPIV_ALGO_3DES; // triple des - apdu.p2 = YKPIV_KEY_CARDMGM; // management key - apdu.data[0] = 0x7c; - apdu.data[1] = 20; // 2 + 8 + 2 +8 - apdu.data[2] = 0x80; - apdu.data[3] = 8; - apdu.data[4..12].copy_from_slice(&response); - apdu.data[12] = 0x81; - apdu.data[13] = 8; + + let mut data = [0u8; 22]; + data[0] = 0x7c; + 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; if getrandom(&mut data[14..22]).is_err() { error!("failed getting randomness for authentication."); let _ = self._ykpiv_end_transaction(); return Err(Error::RandomnessError); } + challenge.copy_from_slice(&data[14..22]); - apdu.lc = 22; + let apdu = APDU::new(YKPIV_INS_AUTHENTICATE) + .params(YKPIV_ALGO_3DES, YKPIV_KEY_CARDMGM) + .data(&data) + .to_bytes(); - res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw); + res = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw); if res.is_err() { let _ = self._ykpiv_end_transaction(); @@ -825,11 +816,7 @@ impl YubiKey { new_key: &[u8; DES_LEN_3DES], touch: u8, ) -> Result<(), Error> { - let mut data = [0u8; 261]; - let mut recv_len = data.len() as u32; - let mut sw: i32 = 0; let mut res = Ok(()); - let mut apdu = APDU::default(); self._ykpiv_begin_transaction()?; @@ -839,36 +826,41 @@ impl YubiKey { "won't set new key '{:?}' since it's weak (with odd parity)", new_key ); - res = Err(Error::KeyError); - } else { - apdu.ins = YKPIV_INS_SET_MGMKEY; - apdu.p1 = 0xff; - - apdu.p2 = match touch { - 0 => 0xff, - 1 => 0xfe, - _ => { - let _ = self._ykpiv_end_transaction(); - return Err(Error::GenericError); - } - }; - apdu.lc = DES_LEN_3DES as u8 + 3; - apdu.data[0] = YKPIV_ALGO_3DES; - apdu.data[1] = YKPIV_KEY_CARDMGM; - apdu.data[2] = DES_LEN_3DES as u8; - apdu.data[3..3 + DES_LEN_3DES].copy_from_slice(new_key); - - res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw); + let _ = self._ykpiv_end_transaction(); + return Err(Error::KeyError); + } - if res.is_ok() && sw != SW_SUCCESS { - res = Err(Error::GenericError); + let p2 = match touch { + 0 => 0xff, + 1 => 0xfe, + _ => { + let _ = self._ykpiv_end_transaction(); + return Err(Error::GenericError); } + }; + + let mut data = [0u8; DES_LEN_3DES + 3]; + data[0] = YKPIV_ALGO_3DES; + data[1] = YKPIV_KEY_CARDMGM; + data[2] = DES_LEN_3DES as u8; + data[3..3 + DES_LEN_3DES].copy_from_slice(new_key); + + let apdu = APDU::new(YKPIV_INS_SET_MGMKEY) + .params(0xff, p2) + .data(&data) + .to_bytes(); + + let mut data = [0u8; 261]; + let mut recv_len = data.len() as u32; + let mut sw: i32 = 0; + res = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw); + + if res.is_ok() && sw != SW_SUCCESS { + res = Err(Error::GenericError); } } - apdu.zeroize(); - let _ = self._ykpiv_end_transaction(); res } @@ -1046,10 +1038,9 @@ impl YubiKey { } // get version from device - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_GET_VERSION; + let apdu = APDU::new(YKPIV_INS_GET_VERSION).to_bytes(); - self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw)?; + self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw)?; if sw != SW_SUCCESS { return Err(Error::GenericError); @@ -1084,7 +1075,7 @@ impl YubiKey { /// /// NOTE: caller must make sure that this is wrapped in a transaction for synchronized operation pub(crate) unsafe fn _ykpiv_get_serial(&mut self, f_force: bool) -> Result { - let yk_applet: *const u8 = ptr::null(); + let yk_applet = [0xa0, 0x00, 0x00, 0x05, 0x27, 0x20, 0x01, 0x01]; let mut data = [0u8; 255]; let mut recv_len = data.len() as u32; let mut sw: i32 = 0; @@ -1099,18 +1090,14 @@ impl YubiKey { let mut temp = [0u8; 255]; recv_len = temp.len() as u32; - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_SELECT_APPLICATION; - apdu.p1 = 0x04; - apdu.lc = mem::size_of_val(&yk_applet) as u8; + let apdu = APDU::new(YKPIV_INS_SELECT_APPLICATION) + .p1(0x04) + .data(&yk_applet) + .to_bytes(); - memcpy( - apdu.data.as_mut_ptr() as *mut c_void, - yk_applet as *const c_void, - mem::size_of_val(&yk_applet), - ); - - if let Err(e) = self._send_data(&mut apdu, temp.as_mut_ptr(), &mut recv_len, &mut sw) { + if let Err(e) = + self._send_data(apdu.as_slice(), temp.as_mut_ptr(), &mut recv_len, &mut sw) + { error!("failed communicating with card: '{}'", e); return Err(e); } @@ -1121,12 +1108,11 @@ impl YubiKey { } recv_len = temp.len() as u32; - apdu = APDU::default(); - apdu.ins = 0x01; - apdu.p1 = 0x10; - apdu.lc = 0x00; + let apdu = APDU::new(0x01).p1(0x10).to_bytes(); - if let Err(e) = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw) { + if let Err(e) = + self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw) + { error!("failed communicating with card: '{}'", e); return Err(e); } @@ -1137,18 +1123,14 @@ impl YubiKey { } recv_len = temp.len() as u32; - apdu = APDU::default(); - apdu.ins = YKPIV_INS_SELECT_APPLICATION; - apdu.p1 = 0x04; - apdu.lc = mem::size_of_val(&AID) as u8; - - memcpy( - apdu.data.as_mut_ptr() as *mut c_void, - AID.as_ptr() as *const c_void, - mem::size_of_val(&AID), - ); + let apdu = APDU::new(YKPIV_INS_SELECT_APPLICATION) + .p1(0x04) + .data(&AID) + .to_bytes(); - if let Err(e) = self._send_data(&mut apdu, temp.as_mut_ptr(), &mut recv_len, &mut sw) { + if let Err(e) = + self._send_data(apdu.as_slice(), temp.as_mut_ptr(), &mut recv_len, &mut sw) + { error!("failed communicating with card: '{}'", e); return Err(e); } @@ -1159,10 +1141,11 @@ impl YubiKey { } } else { // get serial from yk5 and later devices using the f8 command - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_GET_SERIAL; + let apdu = APDU::new(YKPIV_INS_GET_SERIAL).to_bytes(); - if let Err(e) = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw) { + if let Err(e) = + self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw) + { error!("failed communicating with card: '{}'", e); return Err(e); } @@ -1264,31 +1247,27 @@ impl YubiKey { return Err(Error::SizeError); } - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_VERIFY; - apdu.p1 = 0x00; - apdu.p2 = 0x80; - apdu.lc = if pin.is_null() { 0 } else { 0x08 }; + let mut apdu = APDU::new(YKPIV_INS_VERIFY); + apdu.params(0x00, 0x80); if !pin.is_null() { + let mut data = [0xFF; CB_PIN_MAX]; + memcpy( - apdu.data.as_mut_ptr() as *mut c_void, + data.as_mut_ptr() as *mut c_void, pin as *const c_void, pin_len, ); - if pin_len < CB_PIN_MAX { - memset( - apdu.data.as_mut_ptr().add(pin_len) as *mut c_void, - 0xff, - CB_PIN_MAX - pin_len, - ); - } + apdu.data(data); } - let res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw); - - apdu.zeroize(); + let res = self._send_data( + apdu.to_bytes().as_slice(), + data.as_mut_ptr(), + &mut recv_len, + &mut sw, + ); if let Err(e) = res { return Err(e); @@ -1943,16 +1922,14 @@ impl YubiKey { self._ykpiv_begin_transaction()?; if self._ykpiv_ensure_application_selected().is_ok() { - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_AUTHENTICATE; - apdu.p1 = YKPIV_ALGO_3DES; // triple des - apdu.p2 = YKPIV_KEY_CARDMGM; // management key - apdu.lc = 0x04; - apdu.data[0] = 0x7c; - apdu.data[1] = 0x02; - apdu.data[2] = 0x81; //0x80; - - if let Err(e) = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw) { + let apdu = APDU::new(YKPIV_INS_AUTHENTICATE) + .params(YKPIV_ALGO_3DES, YKPIV_KEY_CARDMGM) + .data(&[0x7c, 0x02, 0x81, 0x00]) + .to_bytes(); + + if let Err(e) = + self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw) + { res = Err(e) } else if sw != SW_SUCCESS { res = Err(Error::AuthenticationError); @@ -1969,31 +1946,29 @@ impl YubiKey { /// Verify an auth response pub unsafe fn ykpiv_auth_verifyresponse(&mut self, response: [u8; 8]) -> Result<(), Error> { - let mut data = [0u8; 261]; - let mut recv_len = data.len() as u32; - let mut sw: i32 = 0; - self._ykpiv_begin_transaction()?; + let mut data = [ + 0x7c, 0x0a, 0x82, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ]; + + data[4..12].copy_from_slice(&response); + // send the response to the card and a challenge of our own. - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_AUTHENTICATE; - apdu.p1 = YKPIV_ALGO_3DES; // triple des - apdu.p2 = YKPIV_KEY_CARDMGM; // management key - apdu.data[0] = 0x7c; - apdu.data[1] = 0x0a; // 2 + 8 - apdu.data[2] = 0x82; - apdu.data[3] = 8; - apdu.data[4..12].copy_from_slice(&response); - apdu.lc = 12; - - let mut res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw); + let apdu = APDU::new(YKPIV_INS_AUTHENTICATE) + .params(YKPIV_ALGO_3DES, YKPIV_KEY_CARDMGM) + .data(&data) + .to_bytes(); + + let mut data = [0u8; 261]; + let mut recv_len = data.len() as u32; + let mut sw: i32 = 0; + let mut res = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw); if res.is_ok() && sw != SW_SUCCESS { res = Err(Error::AuthenticationError); } - apdu.zeroize(); let _ = self._ykpiv_end_transaction(); res } @@ -2006,18 +1981,12 @@ impl YubiKey { self._ykpiv_begin_transaction()?; - let mut apdu = APDU::default(); - apdu.ins = YKPIV_INS_SELECT_APPLICATION; - apdu.p1 = 0x04; - apdu.lc = mem::size_of::<*const u8>() as u8; - - memcpy( - apdu.data.as_mut_ptr() as *mut c_void, - MGMT_AID.as_ptr() as *const c_void, - MGMT_AID.len(), - ); + let apdu = APDU::new(YKPIV_INS_SELECT_APPLICATION) + .p1(0x04) + .data(MGMT_AID) + .to_bytes(); - let mut res = self._send_data(&mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw); + let mut res = self._send_data(apdu.as_slice(), data.as_mut_ptr(), &mut recv_len, &mut sw); if let Err(e) = &res { error!("failed communicating with card: \'{}\'", e);