From 86fde50c2dacf57369cf857cfc734f0298ee8d5f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 21 Nov 2019 00:19:34 +0000 Subject: [PATCH 1/3] Use des crate for 3DES operations --- Cargo.toml | 1 + src/internal.rs | 257 +++++++++++++++++------------------------------- src/util.rs | 2 +- src/yubikey.rs | 110 +++++---------------- 4 files changed, 121 insertions(+), 249 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 3f314e3..c163961 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,6 +17,7 @@ categories = ["api-bindings", "cryptography", "hardware-support"] keywords = ["ccid", "ecdsa", "rsa", "piv", "yubikey"] [dependencies] +des = "0.3" getrandom = "0.1" hmac = "0.7" libc = "0.2" diff --git a/src/internal.rs b/src/internal.rs index e773ea2..1df9429 100644 --- a/src/internal.rs +++ b/src/internal.rs @@ -39,189 +39,116 @@ #![allow(clippy::missing_safety_doc)] use crate::consts::*; -use libc::{ - c_char, c_int, fclose, feof, fgets, fopen, free, getenv, malloc, memcpy, memset, sscanf, - strcasecmp, strcmp, -}; -use std::{ - ffi::{CStr, CString}, - mem, - os::raw::c_void, +use des::{ + block_cipher_trait::{generic_array::GenericArray, BlockCipher}, + TdesEde3, }; +use libc::{c_char, c_int, fclose, feof, fgets, fopen, getenv, sscanf, strcasecmp, strcmp}; +use std::ffi::{CStr, CString}; +use zeroize::Zeroize; -extern "C" { - fn DES_ecb3_encrypt( - input: *mut [u8; 8], - output: *mut [u8; 8], - ks1: *mut DesSubKey, - ks2: *mut DesSubKey, - ks3: *mut DesSubKey, - enc: i32, - ); - fn DES_is_weak_key(key: *mut [u8; 8]) -> i32; - fn DES_set_key_unchecked(key: *mut [u8; 8], schedule: *mut DesSubKey); +/// 3DES keys. The three subkeys are concatenated. +pub struct DesKey([u8; DES_LEN_3DES]); + +impl DesKey { + pub fn from_bytes(bytes: [u8; DES_LEN_3DES]) -> Self { + DesKey(bytes) + } + + pub fn write(&self, out: &mut [u8]) { + out.copy_from_slice(&self.0); + } } -/// DES-related errors -#[allow(non_camel_case_types)] -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -#[repr(i32)] -pub enum DesErrorKind { - /// Ok - Ok = 0, - - /// Invalid parameter - InvalidParameter = -1, - - /// Buffer too small - BufferTooSmall = -2, - - /// Memory error - MemoryError = -3, - - /// General error - GeneralError = -4, +impl AsRef<[u8; 24]> for DesKey { + fn as_ref(&self) -> &[u8; 24] { + &self.0 + } } -/// 3DES subkeys -#[derive(Copy, Clone)] -#[repr(C)] -pub struct DesSubKey([u8; 16]); - -/// 3DES keys -#[derive(Copy, Clone)] -pub struct DesKey { - /// subkey 1 - pub ks1: DesSubKey, - - /// subkey 2 - pub ks2: DesSubKey, - - /// subkey 3 - pub ks3: DesSubKey, +impl Zeroize for DesKey { + fn zeroize(&mut self) { + self.0.zeroize(); + } } -/// Import DES key -pub unsafe fn des_import_key( - key_type: i32, - keyraw: *const u8, - keyrawlen: usize, - key: *mut *mut DesKey, -) -> DesErrorKind { - let mut key_tmp = [0u8; 8]; - let cb_expectedkey: usize; - let cb_keysize: usize; - - if key_type != DES_TYPE_3DES as i32 { - return DesErrorKind::InvalidParameter; +impl Drop for DesKey { + fn drop(&mut self) { + self.zeroize(); } - - cb_expectedkey = (8i32 * 3i32) as (usize); - cb_keysize = 8usize; - - if cb_keysize > 8 { - return DesErrorKind::MemoryError; - } - - if key.is_null() || keyraw.is_null() || keyrawlen != cb_expectedkey { - return DesErrorKind::InvalidParameter; - } - - *key = malloc(mem::size_of::()) as (*mut DesKey); - - if (*key).is_null() { - return DesErrorKind::MemoryError; - } - - memset(*key as (*mut c_void), 0i32, mem::size_of::()); - - memcpy( - key_tmp.as_mut_ptr() as (*mut c_void), - keyraw as (*const c_void), - cb_keysize, - ); - - DES_set_key_unchecked(&mut key_tmp, &mut (**key).ks1); - - memcpy( - key_tmp.as_mut_ptr() as (*mut c_void), - keyraw.add(cb_keysize) as (*const c_void), - cb_keysize, - ); - - DES_set_key_unchecked(&mut key_tmp, &mut (**key).ks2); - - memcpy( - key_tmp.as_mut_ptr() as (*mut c_void), - keyraw.add(2usize.wrapping_mul(cb_keysize)) as (*const c_void), - cb_keysize, - ); - - DES_set_key_unchecked(&mut key_tmp, &mut (**key).ks3); - - DesErrorKind::Ok -} - -/// Destroy DES key -pub unsafe fn des_destroy_key(key: *mut DesKey) -> DesErrorKind { - if !key.is_null() { - free(key as (*mut c_void)); - } - - DesErrorKind::Ok } /// Encrypt with DES key -pub unsafe fn des_encrypt( - key: *mut DesKey, - input: *const u8, - inputlen: usize, - out: *mut u8, - outlen: *mut usize, -) -> DesErrorKind { - if key.is_null() || outlen.is_null() || *outlen < inputlen || input.is_null() || out.is_null() { - return DesErrorKind::InvalidParameter; - } - - DES_ecb3_encrypt( - input as *mut [u8; 8], - out as *mut [u8; 8], - &mut (*key).ks1, - &mut (*key).ks2, - &mut (*key).ks3, - 1, - ); - - DesErrorKind::Ok +pub fn des_encrypt(key: &DesKey, input: &[u8; DES_LEN_DES], output: &mut [u8; DES_LEN_DES]) { + output.copy_from_slice(input); + TdesEde3::new(GenericArray::from_slice(&key.0)) + .encrypt_block(GenericArray::from_mut_slice(output)); } /// Decrypt with DES key -pub unsafe fn des_decrypt( - key: *mut DesKey, - in_: *const u8, - inlen: usize, - out: *mut u8, - outlen: *mut usize, -) -> DesErrorKind { - if key.is_null() || outlen.is_null() || *outlen < inlen || in_.is_null() || out.is_null() { - return DesErrorKind::InvalidParameter; - } - - DES_ecb3_encrypt( - in_ as *mut [u8; 8], - out as *mut [u8; 8], - &mut (*key).ks1, - &mut (*key).ks2, - &mut (*key).ks3, - 0, - ); - - DesErrorKind::Ok +pub fn des_decrypt(key: &DesKey, input: &[u8; DES_LEN_DES], output: &mut [u8; DES_LEN_DES]) { + output.copy_from_slice(input); + TdesEde3::new(GenericArray::from_slice(&key.0)) + .encrypt_block(GenericArray::from_mut_slice(output)); } /// Is the given DES key weak? -pub unsafe fn yk_des_is_weak_key(key: *const u8, _cb_key: usize) -> bool { - DES_is_weak_key(key as (*mut [u8; 8])) != 0 +pub fn yk_des_is_weak_key(key: &[u8; DES_LEN_3DES]) -> bool { + /// Weak and semi weak 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_keys: [[u8; DES_LEN_DES]; 16] = [ + // 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], + ]; + + // set odd parity of key + let mut tmp = [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) | (if c & 0x01 == 0x01 { 0x00 } else { 0x01 }); + } + + // check odd parity key against table by DES key block + let mut rv = false; + for weak_key in weak_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] + { + rv = true; + break; + } + } + + tmp.zeroize(); + rv } /// PKCS#5 error types diff --git a/src/util.rs b/src/util.rs index 7f6ee88..2d45ec7 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1622,7 +1622,7 @@ pub unsafe fn ykpiv_util_set_protected_mgm( } } - let ykrc = ykpiv_set_mgmkey(state, mgm_key.as_mut_ptr()); + let ykrc = ykpiv_set_mgmkey(state, &mgm_key); if ykrc.is_err() { // if set_mgmkey fails with KeyError, it means the generated key is weak diff --git a/src/yubikey.rs b/src/yubikey.rs index 18c9a8e..6aa238d 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -37,10 +37,7 @@ use crate::{ apdu::APDU, consts::*, error::ErrorKind, - internal::{ - des_decrypt, des_destroy_key, des_encrypt, des_import_key, yk_des_is_weak_key, - DesErrorKind, DesKey, - }, + internal::{des_decrypt, des_encrypt, yk_des_is_weak_key, DesKey}, }; use getrandom::getrandom; use libc::{c_char, free, malloc, memcmp, memcpy, memmove, memset, strlen, strncasecmp}; @@ -766,37 +763,23 @@ pub(crate) unsafe fn _send_data( } /// Default authentication key -pub const DEFAULT_AUTH_KEY: &[u8] = b"\x01\x02\x03\x04\x05\x06\x07\x08\x01\x02\x03\x04\x05\x06\x07\x08\x01\x02\x03\x04\x05\x06\x07\x08\0"; +pub const DEFAULT_AUTH_KEY: &[u8; DES_LEN_3DES] = b"\x01\x02\x03\x04\x05\x06\x07\x08\x01\x02\x03\x04\x05\x06\x07\x08\x01\x02\x03\x04\x05\x06\x07\x08"; /// Authenticate to the card -pub unsafe fn ykpiv_authenticate(state: &mut YubiKey, mut key: *const u8) -> Result<(), ErrorKind> { +pub unsafe fn ykpiv_authenticate( + state: &mut YubiKey, + key: Option<&[u8; DES_LEN_3DES]>, +) -> Result<(), ErrorKind> { let mut data = [0u8; 261]; - let mut challenge = [0u8; 8]; let mut recv_len = data.len() as u32; let mut sw: i32 = 0; - let mut drc: DesErrorKind; - let mut mgm_key: *mut DesKey = ptr::null_mut(); - let mut out_len: usize; let mut res = Ok(()); _ykpiv_begin_transaction(state)?; if _ykpiv_ensure_application_selected(state).is_ok() { - if key.is_null() { - key = DEFAULT_AUTH_KEY.as_ptr(); - } - - // set up our key - drc = des_import_key(1i32, key, (8i32 * 3i32) as (usize), &mut mgm_key); - - if drc != DesErrorKind::Ok { - assert!( - mgm_key.is_null(), - "didn't expect mgm key to be set by failing op!" - ); - let _ = _ykpiv_end_transaction(state); - return Err(ErrorKind::AlgorithmError); - } + // use the provided mgm key to authenticate; if it hasn't been provided, use default + let mgm_key = DesKey::from_bytes(*key.unwrap_or(DEFAULT_AUTH_KEY)); // get a challenge from the card let mut apdu = APDU::default(); @@ -813,35 +796,17 @@ pub unsafe fn ykpiv_authenticate(state: &mut YubiKey, mut key: *const u8) -> Res if res.is_err() { let _ = _ykpiv_end_transaction(state); return res; - } - - if sw != SW_SUCCESS { + } else if sw != SW_SUCCESS { let _ = _ykpiv_end_transaction(state); return Err(ErrorKind::AuthenticationError); } - memcpy( - challenge.as_mut_ptr() as *mut c_void, - data.as_ptr().offset(4) as *const c_void, - 8, - ); + let mut challenge = [0u8; 8]; + challenge.copy_from_slice(&data[4..12]); // send a response to the cards challenge and a challenge of our own. let mut response = [0u8; 8]; - out_len = response.len(); - - drc = des_decrypt( - mgm_key, - challenge.as_ptr(), - challenge.len(), - response.as_mut_ptr(), - &mut out_len, - ); - - if drc != DesErrorKind::Ok { - let _ = _ykpiv_end_transaction(state); - return Err(ErrorKind::AuthenticationError); - } + des_decrypt(&mgm_key, &challenge, &mut response); recv_len = data.len() as u32; apdu = APDU::default(); @@ -870,55 +835,38 @@ pub unsafe fn ykpiv_authenticate(state: &mut YubiKey, mut key: *const u8) -> Res if res.is_err() { let _ = _ykpiv_end_transaction(state); return res; - } - - if sw != SW_SUCCESS { + } else if sw != SW_SUCCESS { let _ = _ykpiv_end_transaction(state); return Err(ErrorKind::AuthenticationError); } // compare the response from the card with our challenge - let mut response = [0u8; 8]; - out_len = response.len(); - drc = des_encrypt( - mgm_key, - challenge.as_ptr(), - mem::size_of::<[u8; 8]>(), - response.as_mut_ptr(), - &mut out_len, - ); + des_encrypt(&mgm_key, &challenge, &mut response); - if drc == DesErrorKind::Ok - // TODO(tarcieri): constant time comparison! - && memcmp( - response.as_mut_ptr() as *const c_void, - data.as_mut_ptr().offset(4) as *const c_void, - 8, - ) == 0 - { + // TODO(tarcieri): constant time comparison! + if response == &data[4..12] { res = Ok(()); } else { res = Err(ErrorKind::AuthenticationError); } } - if !mgm_key.is_null() { - des_destroy_key(mgm_key); - } - let _ = _ykpiv_end_transaction(state); res } /// Set the management key (MGM) -pub unsafe fn ykpiv_set_mgmkey(state: &mut YubiKey, new_key: *const u8) -> Result<(), ErrorKind> { +pub unsafe fn ykpiv_set_mgmkey( + state: &mut YubiKey, + new_key: &[u8; DES_LEN_3DES], +) -> Result<(), ErrorKind> { ykpiv_set_mgmkey2(state, new_key, 0) } /// Set the management key (MGM) pub(crate) unsafe fn ykpiv_set_mgmkey2( state: &mut YubiKey, - new_key: *const u8, + new_key: &[u8; DES_LEN_3DES], touch: u8, ) -> Result<(), ErrorKind> { let mut data = [0u8; 261]; @@ -930,12 +878,13 @@ pub(crate) unsafe fn ykpiv_set_mgmkey2( _ykpiv_begin_transaction(state)?; if _ykpiv_ensure_application_selected(state).is_ok() { - if yk_des_is_weak_key(new_key, (8i32 * 3i32) as (usize)) { + if yk_des_is_weak_key(new_key) { error!( "won't set new key '{:?}' since it's weak (with odd parity)", - slice::from_raw_parts(new_key, DES_LEN_3DES) + new_key ); res = Err(ErrorKind::KeyError); + } else { apdu.ins = YKPIV_INS_SET_MGMKEY; apdu.p1 = 0xff; @@ -952,16 +901,11 @@ pub(crate) unsafe fn ykpiv_set_mgmkey2( apdu.data[0] = YKPIV_ALGO_3DES; apdu.data[1] = YKPIV_KEY_CARDMGM; apdu.data[2] = DES_LEN_3DES as u8; - memcpy( - apdu.data.as_mut_ptr().offset(3) as *mut c_void, - new_key as *const c_void, - DES_LEN_3DES, - ); - } else { + apdu.data[3..3 + DES_LEN_3DES].copy_from_slice(new_key); + res = _send_data(state, &mut apdu, data.as_mut_ptr(), &mut recv_len, &mut sw); - // TODO(str4d): Shouldn't this be res.is_ok()? - if res.is_err() && sw != SW_SUCCESS { + if res.is_ok() && sw != SW_SUCCESS { res = Err(ErrorKind::GenericError); } } From 35cc1bbf726c0e33e9b63eedbedb8399abc0f51e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 21 Nov 2019 00:44:49 +0000 Subject: [PATCH 2/3] Address clippy lints --- src/internal.rs | 2 ++ src/yubikey.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/internal.rs b/src/internal.rs index 1df9429..44fac1d 100644 --- a/src/internal.rs +++ b/src/internal.rs @@ -79,6 +79,7 @@ impl Drop for DesKey { } /// Encrypt with DES key +#[allow(clippy::trivially_copy_pass_by_ref)] pub fn des_encrypt(key: &DesKey, input: &[u8; DES_LEN_DES], output: &mut [u8; DES_LEN_DES]) { output.copy_from_slice(input); TdesEde3::new(GenericArray::from_slice(&key.0)) @@ -86,6 +87,7 @@ pub fn des_encrypt(key: &DesKey, input: &[u8; DES_LEN_DES], output: &mut [u8; DE } /// Decrypt with DES key +#[allow(clippy::trivially_copy_pass_by_ref)] pub fn des_decrypt(key: &DesKey, input: &[u8; DES_LEN_DES], output: &mut [u8; DES_LEN_DES]) { output.copy_from_slice(input); TdesEde3::new(GenericArray::from_slice(&key.0)) diff --git a/src/yubikey.rs b/src/yubikey.rs index 6aa238d..9f2e146 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -844,7 +844,7 @@ pub unsafe fn ykpiv_authenticate( des_encrypt(&mgm_key, &challenge, &mut response); // TODO(tarcieri): constant time comparison! - if response == &data[4..12] { + if response == data[4..12] { res = Ok(()); } else { res = Err(ErrorKind::AuthenticationError); From a71389a8208f57c000fb2c22e49bafecf1675d73 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 21 Nov 2019 00:48:48 +0000 Subject: [PATCH 3/3] Remove completed TODO --- src/internal.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/internal.rs b/src/internal.rs index 44fac1d..ea65c5b 100644 --- a/src/internal.rs +++ b/src/internal.rs @@ -1,9 +1,5 @@ //! Internal functions (mostly 3DES) -// TODO(tarcieri): replace OpenSSL extern "C" invocations with `des` crate -// - crate: https://crates.io/crates/des -// - docs: https://docs.rs/des/0 - // Adapted from yubico-piv-tool: // //