From 5361876fc576e72f979e3714f9185f5405cb5539 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Fri, 13 Dec 2024 15:16:42 -0500 Subject: [PATCH 1/6] Add caching layer for NSS keys NSS uses a particularly expensive PBKDF2 operation to derive per-attribute encryption/siagnture keys, therefore a caching layer is necessary to attain reasonable peformance. Signed-off-by: Simo Sorce --- src/error.rs | 7 + src/storage/nssdb/ci.rs | 324 +++++++++++++++++++++++++++++---------- src/storage/nssdb/mod.rs | 104 ++++++------- 3 files changed, 302 insertions(+), 133 deletions(-) diff --git a/src/error.rs b/src/error.rs index f28670c..49353cb 100644 --- a/src/error.rs +++ b/src/error.rs @@ -204,6 +204,13 @@ impl From> for Error { } } +use std::array::TryFromSliceError; +impl From for Error { + fn from(error: TryFromSliceError) -> Error { + Error::other_error(error) + } +} + #[macro_export] macro_rules! some_or_err { ($action:expr) => { diff --git a/src/storage/nssdb/ci.rs b/src/storage/nssdb/ci.rs index 2885dd0..5914d01 100644 --- a/src/storage/nssdb/ci.rs +++ b/src/storage/nssdb/ci.rs @@ -1,6 +1,9 @@ // Copyright 2024 Simo Sorce // See LICENSE.txt file for terms +use std::collections::BTreeMap; +use std::sync::{RwLock, RwLockReadGuard}; + use crate::attribute::CkAttrs; use crate::error::Result; use crate::interface::*; @@ -12,6 +15,119 @@ use crate::token::TokenFacilities; use crate::CSPRNG; use crate::{sizeof, void_ptr}; +use zeroize::Zeroize; + +const SHA256_LEN: usize = 32; + +enum KeyOp { + Encryption, + Signature, +} + +#[derive(Debug)] +struct LockedKey<'a> { + id: [u8; SHA256_LEN], + l: RwLockReadGuard<'a, BTreeMap<[u8; SHA256_LEN], Object>>, +} + +impl LockedKey<'_> { + fn get_id(&self) -> [u8; SHA256_LEN] { + self.id + } + + pub fn get_key<'a>(&'a self) -> Option<&'a Object> { + self.l.get(&self.id) + } +} + +#[derive(Debug)] +pub struct KeysWithCaching { + enckey: Option>, + cache: RwLock>, +} + +impl Default for KeysWithCaching { + fn default() -> KeysWithCaching { + KeysWithCaching { + enckey: None, + cache: RwLock::new(BTreeMap::new()), + } + } +} + +impl Drop for KeysWithCaching { + fn drop(&mut self) { + if let Some(ref mut key) = &mut self.enckey { + key.zeroize(); + } + } +} + +impl KeysWithCaching { + pub fn available(&self) -> bool { + self.enckey.is_some() + } + + fn get_key(&self) -> Result<&[u8]> { + match &self.enckey { + Some(ref key) => Ok(key.as_slice()), + None => Err(CKR_USER_NOT_LOGGED_IN)?, + } + } + + pub fn check_key(&self, check: &[u8]) -> bool { + match &self.enckey { + Some(ref key) => key.as_slice() == check, + None => false, + } + } + + pub fn set_key(&mut self, key: Vec) { + self.enckey = Some(key); + } + + pub fn unset_key(&mut self) { + self.enckey = None; + } + + fn get_cached_key(&self, id: &[u8; SHA256_LEN]) -> Option { + if self.enckey.is_none() { + /* access to the cache is available only if enckey is set. + * When unset it means the user logged off and no + * keys should be available */ + return None; + } + let read_lock = match self.cache.read() { + Ok(r) => r, + Err(_) => return None, + }; + Some(LockedKey { + id: *id, + l: read_lock, + }) + } + + fn set_cached_key(&self, id: &[u8; SHA256_LEN], key: Object) -> Result<()> { + let mut w = match self.cache.write() { + Ok(w) => w, + Err(_) => return Err(CKR_CANT_LOCK)?, + }; + let _ = w.insert(*id, key); + Ok(()) + } + + fn invalidate_cached_key(&self, lk: LockedKey) { + let id = lk.get_id(); + drop(lk); + match self.cache.write() { + Ok(mut w) => { + let _ = w.remove(&id); + } + Err(_) => (), + } + } +} + pub const NSS_MP_PBE_ITERATION_COUNT: usize = 10000; #[derive( @@ -125,9 +241,72 @@ fn aes_cbc_encrypt( Ok((iv[2..].to_vec(), encdata)) } +fn derive_key_internal<'a>( + facilities: &TokenFacilities, + keys: &'a KeysWithCaching, + params: &PBKDF2Params, + operation: KeyOp, +) -> Result> { + let keyid: [u8; 32] = params.salt.try_into()?; + + /* First check if we have this key in cache */ + match keys.get_cached_key(&keyid) { + Some(lk) => match lk.get_key() { + Some(_) => return Ok(lk), + None => (), + }, + None => (), + } + + /* if not compute it */ + let ck_class: CK_OBJECT_CLASS = CKO_SECRET_KEY; + let ck_key_type: CK_KEY_TYPE = match operation { + KeyOp::Encryption => CKK_AES, + KeyOp::Signature => CKK_GENERIC_SECRET, + }; + let ck_key_len: CK_ULONG = match params.key_length { + Some(l) => CK_ULONG::try_from(l)?, + None => return Err(CKR_MECHANISM_PARAM_INVALID)?, + }; + let ck_true: CK_BBOOL = CK_TRUE; + let mut key_template = CkAttrs::with_capacity(5); + key_template.add_ulong(CKA_CLASS, &ck_class); + key_template.add_ulong(CKA_KEY_TYPE, &ck_key_type); + key_template.add_ulong(CKA_VALUE_LEN, &ck_key_len); + match operation { + KeyOp::Encryption => { + key_template.add_bool(CKA_DECRYPT, &ck_true); + key_template.add_bool(CKA_ENCRYPT, &ck_true); + } + KeyOp::Signature => { + key_template.add_bool(CKA_SIGN, &ck_true); + key_template.add_bool(CKA_VERIFY, &ck_true); + } + } + + let key = pbkdf2_derive( + facilities, + params, + keys.get_key()?, + key_template.as_slice(), + )?; + + /* and store in cache for later use */ + keys.set_cached_key(&keyid, key)?; + match keys.get_cached_key(&keyid) { + Some(lk) => match lk.get_key() { + Some(_) => return Ok(lk), + None => (), + }, + None => (), + } + + return Err(CKR_GENERAL_ERROR)?; +} + pub fn decrypt_data( facilities: &TokenFacilities, - secret: &[u8], + keys: &KeysWithCaching, data: &[u8], ) -> Result> { let info = match asn1::parse_single::(data) { @@ -140,43 +319,36 @@ pub fn decrypt_data( _ => return Err(CKR_MECHANISM_INVALID)?, }; - let key = match &pbes2.key_derivation_func.params { + let lockedkey = match &pbes2.key_derivation_func.params { AlgorithmParameters::Pbkdf2(ref params) => { - let ck_class: CK_OBJECT_CLASS = CKO_SECRET_KEY; - let ck_key_type: CK_KEY_TYPE = CKK_AES; - let ck_key_len: CK_ULONG = match params.key_length { - Some(l) => CK_ULONG::try_from(l)?, - None => return Err(CKR_MECHANISM_PARAM_INVALID)?, - }; - let ck_true: CK_BBOOL = CK_TRUE; - let mut key_template = CkAttrs::with_capacity(5); - key_template.add_ulong(CKA_CLASS, &ck_class); - key_template.add_ulong(CKA_KEY_TYPE, &ck_key_type); - key_template.add_ulong(CKA_VALUE_LEN, &ck_key_len); - key_template.add_bool(CKA_DECRYPT, &ck_true); - key_template.add_bool(CKA_ENCRYPT, &ck_true); - - pbkdf2_derive(facilities, params, secret, key_template.as_slice())? + derive_key_internal(facilities, keys, params, KeyOp::Encryption)? } _ => return Err(CKR_MECHANISM_INVALID)?, }; - match &pbes2.encryption_scheme.params { + + let key = match lockedkey.get_key() { + Some(k) => k, + None => return Err(CKR_GENERAL_ERROR)?, + }; + + let result = match &pbes2.encryption_scheme.params { BrokenAlgorithmParameters::Aes128Cbc(ref iv) => { - aes_cbc_decrypt(facilities, &key, iv, info.enc_or_sig_data) + aes_cbc_decrypt(facilities, key, iv, info.enc_or_sig_data) } BrokenAlgorithmParameters::Aes256Cbc(ref iv) => { - aes_cbc_decrypt(facilities, &key, iv, info.enc_or_sig_data) + aes_cbc_decrypt(facilities, key, iv, info.enc_or_sig_data) } _ => Err(CKR_MECHANISM_INVALID)?, + }; + if result.is_err() { + keys.invalidate_cached_key(lockedkey) } + result } -/* SHA2-256 length */ -const SHA256_LEN: usize = 32; - -pub fn encrypt_data<'a>( +pub fn encrypt_data( facilities: &TokenFacilities, - secret: &'a [u8], + keys: &KeysWithCaching, iterations: usize, data: &[u8], ) -> Result> { @@ -191,25 +363,25 @@ pub fn encrypt_data<'a>( prf: Box::new(HMAC_SHA_256_ALG), }; - /* compute key */ - let ck_class: CK_OBJECT_CLASS = CKO_SECRET_KEY; - let ck_key_type: CK_KEY_TYPE = CKK_AES; - let ck_key_len = CK_ULONG::try_from(SHA256_LEN)?; - let ck_true: CK_BBOOL = CK_TRUE; - let mut key_template = CkAttrs::with_capacity(5); - key_template.add_ulong(CKA_CLASS, &ck_class); - key_template.add_ulong(CKA_KEY_TYPE, &ck_key_type); - key_template.add_ulong(CKA_VALUE_LEN, &ck_key_len); - key_template.add_bool(CKA_DECRYPT, &ck_true); - key_template.add_bool(CKA_ENCRYPT, &ck_true); - - let key = pbkdf2_derive( + let lockedkey = derive_key_internal( facilities, + keys, &pbkdf2_params, - secret, - key_template.as_slice(), + KeyOp::Encryption, )?; - let (iv, enc_data) = aes_cbc_encrypt(facilities, &key, data)?; + + let key = match lockedkey.get_key() { + Some(k) => k, + None => return Err(CKR_GENERAL_ERROR)?, + }; + + let (iv, enc_data) = match aes_cbc_encrypt(facilities, key, data) { + Ok(x) => x, + Err(e) => { + keys.invalidate_cached_key(lockedkey); + return Err(e); + } + }; let enc_params = BrokenAlgorithmIdentifier { oid: asn1::DefinedByMarker::marker(), @@ -263,7 +435,7 @@ fn hmac_verify( pub fn check_signature( facilities: &TokenFacilities, - secret: &[u8], + keys: &KeysWithCaching, attribute: &[u8], signature: &[u8], nssobjid: u32, @@ -279,39 +451,34 @@ pub fn check_signature( _ => return Err(CKR_MECHANISM_INVALID)?, }; - let key = match &pbmac1.key_derivation_func.params { + let lockedkey = match &pbmac1.key_derivation_func.params { AlgorithmParameters::Pbkdf2(ref params) => { - let ck_class: CK_OBJECT_CLASS = CKO_SECRET_KEY; - let ck_key_type: CK_KEY_TYPE = CKK_GENERIC_SECRET; - let ck_key_len: CK_ULONG = match params.key_length { - Some(l) => CK_ULONG::try_from(l)?, - None => return Err(CKR_MECHANISM_PARAM_INVALID)?, - }; - let ck_true: CK_BBOOL = CK_TRUE; - let mut key_template = CkAttrs::with_capacity(5); - key_template.add_ulong(CKA_CLASS, &ck_class); - key_template.add_ulong(CKA_KEY_TYPE, &ck_key_type); - key_template.add_ulong(CKA_VALUE_LEN, &ck_key_len); - key_template.add_bool(CKA_SIGN, &ck_true); - key_template.add_bool(CKA_VERIFY, &ck_true); - - pbkdf2_derive(facilities, params, secret, key_template.as_slice())? + derive_key_internal(facilities, keys, params, KeyOp::Signature)? } _ => return Err(CKR_MECHANISM_INVALID)?, }; - match &pbmac1.message_auth_scheme.params { + let key = match lockedkey.get_key() { + Some(k) => k, + None => return Err(CKR_GENERAL_ERROR)?, + }; + + let result = match &pbmac1.message_auth_scheme.params { AlgorithmParameters::HmacWithSha256(None) => hmac_verify( facilities, CKM_SHA256_HMAC_GENERAL, - &key, + key, nssobjid, sdbtype, attribute, info.enc_or_sig_data, ), _ => Err(CKR_MECHANISM_INVALID)?, + }; + if result.is_err() { + keys.invalidate_cached_key(lockedkey); } + result } pub fn enckey_derive( @@ -358,42 +525,41 @@ fn hmac_sign( pub fn make_signature( facilities: &TokenFacilities, - secret: &[u8], + keys: &KeysWithCaching, attribute: &[u8], nssobjid: u32, sdbtype: u32, - iterations: u64, + iterations: usize, ) -> Result> { let mut salt: [u8; SHA256_LEN] = [0u8; SHA256_LEN]; CSPRNG.with(|rng| rng.borrow_mut().generate_random(&mut salt))?; let pbkdf2_params = PBKDF2Params { salt: &salt, - iteration_count: iterations, + iteration_count: u64::try_from(iterations)?, key_length: Some(u64::try_from(SHA256_LEN)?), prf: Box::new(HMAC_SHA_256_ALG), }; - /* compute key */ - let ck_class: CK_OBJECT_CLASS = CKO_SECRET_KEY; - let ck_key_type: CK_KEY_TYPE = CKK_GENERIC_SECRET; - let ck_key_len = CK_ULONG::try_from(SHA256_LEN)?; - let ck_true: CK_BBOOL = CK_TRUE; - let mut key_template = CkAttrs::with_capacity(5); - key_template.add_ulong(CKA_CLASS, &ck_class); - key_template.add_ulong(CKA_KEY_TYPE, &ck_key_type); - key_template.add_ulong(CKA_VALUE_LEN, &ck_key_len); - key_template.add_bool(CKA_SIGN, &ck_true); - key_template.add_bool(CKA_VERIFY, &ck_true); - - let key = pbkdf2_derive( + let lockedkey = derive_key_internal( facilities, + keys, &pbkdf2_params, - secret, - key_template.as_slice(), + KeyOp::Signature, )?; - let sig = hmac_sign(facilities, &key, nssobjid, sdbtype, attribute)?; + let key = match lockedkey.get_key() { + Some(k) => k, + None => return Err(CKR_GENERAL_ERROR)?, + }; + + let sig = match hmac_sign(facilities, key, nssobjid, sdbtype, attribute) { + Ok(x) => x, + Err(e) => { + keys.invalidate_cached_key(lockedkey); + return Err(e); + } + }; let info = NSSEncryptedDataInfo { algorithm: Box::new(BrokenAlgorithmIdentifier { diff --git a/src/storage/nssdb/mod.rs b/src/storage/nssdb/mod.rs index 63993d9..d71dd33 100644 --- a/src/storage/nssdb/mod.rs +++ b/src/storage/nssdb/mod.rs @@ -120,15 +120,7 @@ struct NSSSearchQuery { pub struct NSSStorage { config: NSSConfig, conn: Arc>, - enckey: Option>, -} - -impl Drop for NSSStorage { - fn drop(&mut self) { - if let Some(ref mut key) = &mut self.enckey { - key.zeroize(); - } - } + keys: KeysWithCaching, } impl NSSStorage { @@ -823,7 +815,7 @@ impl Storage for NSSStorage { _facilities: &TokenFacilities, ) -> Result { self.initialize()?; - self.enckey = None; + self.keys = KeysWithCaching::default(); self.get_token_info() } @@ -864,15 +856,14 @@ impl Storage for NSSStorage { } let mut obj = self.fetch_by_nssid(&table, nssobjid, attrs.as_slice())?; - if let Some(ref enckey) = self.enckey { + if self.keys.available() { if table == NSS_PRIVATE_TABLE { for typ in NSS_SENSITIVE_ATTRIBUTES { let encval = match obj.get_attr(typ) { Some(attr) => attr.get_value(), None => continue, }; - let plain = - decrypt_data(facilities, enckey.as_slice(), encval)?; + let plain = decrypt_data(facilities, &self.keys, encval)?; obj.set_attr(Attribute::from_bytes(typ, plain))?; } } @@ -891,7 +882,7 @@ impl Storage for NSSStorage { let signature = self.fetch_signature(dbtype, nssobjid, typ)?; check_signature( facilities, - enckey.as_slice(), + &self.keys, value.as_slice(), signature.as_slice(), nssobjid, @@ -923,10 +914,9 @@ impl Storage for NSSStorage { _ => return Err(CKR_ATTRIBUTE_VALUE_INVALID)?, }; - let enckey = match self.enckey { - Some(ref e) => e, - None => return Err(CKR_USER_NOT_LOGGED_IN)?, - }; + if !self.keys.available() { + return Err(CKR_USER_NOT_LOGGED_IN)?; + } if table == NSS_PRIVATE_TABLE { for typ in NSS_SENSITIVE_ATTRIBUTES { @@ -938,7 +928,7 @@ impl Storage for NSSStorage { }; let encval = encrypt_data( facilities, - enckey.as_slice(), + &self.keys, NSS_MP_PBE_ITERATION_COUNT, plain.as_slice(), )?; @@ -967,11 +957,11 @@ impl Storage for NSSStorage { }; let sig = make_signature( facilities, - enckey.as_slice(), + &self.keys, value.as_slice(), nssobjid, u32::try_from(typ)?, - u64::try_from(NSS_MP_PBE_ITERATION_COUNT)?, + NSS_MP_PBE_ITERATION_COUNT, )?; Self::store_signature( &mut tx, @@ -1004,10 +994,9 @@ impl Storage for NSSStorage { }; let (table, nssobjid) = nss_id_parse(nssid)?; - let enckey = match self.enckey { - Some(ref e) => e, - None => return Err(CKR_USER_NOT_LOGGED_IN)?, - }; + if !self.keys.available() { + return Err(CKR_USER_NOT_LOGGED_IN)?; + } let mut attrs = CkAttrs::from(template); @@ -1020,7 +1009,7 @@ impl Storage for NSSStorage { let plain = a.to_buf()?; let encval = encrypt_data( facilities, - enckey.as_slice(), + &self.keys, NSS_MP_PBE_ITERATION_COUNT, plain.as_slice(), )?; @@ -1044,11 +1033,11 @@ impl Storage for NSSStorage { let value = a.to_buf()?; let sig = make_signature( facilities, - enckey.as_slice(), + &self.keys, value.as_slice(), nssobjid, u32::try_from(typ)?, - u64::try_from(NSS_MP_PBE_ITERATION_COUNT)?, + NSS_MP_PBE_ITERATION_COUNT, )?; Self::store_signature( &mut tx, @@ -1141,35 +1130,44 @@ impl Storage for NSSStorage { * key through pbkdf2 and then decrypting the entry according * to the chosen algorithm. The data is a structured ASN.1 * structure that includes in formation about which algorithm - * to use for the decryption. */ + * to use for the decryption. + * NOTE: to allow key caching we set the key unchecked, and + * then remove it on failure or if only a check was requested */ let (salt, data) = self.fetch_password()?; - let mut enckey = enckey_derive(facilities, pin, salt.as_slice())?; - let plain = - decrypt_data(facilities, enckey.as_slice(), data.as_slice())?; - if plain.as_slice() != NSS_PASS_CHECK { - return Err(CKR_PIN_INCORRECT)?; + let enckey = enckey_derive(facilities, pin, salt.as_slice())?; + let originally_set = self.keys.available(); + if originally_set && self.keys.check_key(enckey.as_slice()) { + return Ok(()); + } + self.keys.set_key(enckey); + let check = match decrypt_data(facilities, &self.keys, data.as_slice()) + { + Ok(plain) => { + if plain.as_slice() == NSS_PASS_CHECK { + Ok(()) + } else { + Err(CKR_PIN_INCORRECT)? + } + } + Err(e) => Err(e), + }; + if check.is_err() { + /* unconditionally remove the key on failure */ + self.keys.unset_key(); + return check; } /* NSS does not support any error counter for authentication attempts */ *flag = 0; - if !check_only { - self.enckey = Some(enckey); - } else { - enckey.zeroize(); + if check_only && !originally_set { + self.keys.unset_key(); } Ok(()) } fn unauth_user(&mut self, _user_type: CK_USER_TYPE) -> Result<()> { - match self.enckey { - Some(ref mut key) => { - key.zeroize(); - self.enckey = None; - } - None => (), - } - Ok(()) + Ok(self.keys.unset_key()) } fn set_user_pin( @@ -1188,7 +1186,9 @@ impl Storage for NSSStorage { let mut salt: [u8; NSS_PIN_SALT_LEN] = [0u8; NSS_PIN_SALT_LEN]; CSPRNG.with(|rng| rng.borrow_mut().generate_random(&mut salt))?; - let newenckey = enckey_derive(facilities, pin, &salt)?; + let enckey = enckey_derive(facilities, pin, &salt)?; + let mut tmpkeys = KeysWithCaching::default(); + tmpkeys.set_key(enckey); let iterations = match pin.len() { 0 => 1, @@ -1197,12 +1197,8 @@ impl Storage for NSSStorage { NSS_MP_PBE_ITERATION_COUNT } }; - let mut encdata = encrypt_data( - facilities, - newenckey.as_slice(), - iterations, - NSS_PASS_CHECK, - )?; + let mut encdata = + encrypt_data(facilities, &tmpkeys, iterations, NSS_PASS_CHECK)?; /* FIXME: need to re-encode all encrypted/integrity protected attributes */ @@ -1259,7 +1255,7 @@ impl StorageDBInfo for NSSDBInfo { Ok(Box::new(NSSStorage { config: config, conn: conn, - enckey: None, + keys: KeysWithCaching::default(), })) } From 7b2cedab8c4ec596f78536e96af9f2ad5bb52801 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Fri, 13 Dec 2024 16:05:13 -0500 Subject: [PATCH 2/6] Add login test that exercises the caching layer Signed-off-by: Simo Sorce --- src/tests/nssdb.rs | 53 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/tests/nssdb.rs b/src/tests/nssdb.rs index 9e880e5..8400967 100644 --- a/src/tests/nssdb.rs +++ b/src/tests/nssdb.rs @@ -290,3 +290,56 @@ fn test_nssdb_init_token_params() { testtokn.finalize(); } + +#[test] +#[parallel] +fn test_nssdb_key_cache() { + let name = String::from("test_nssdb_key_cache"); + let datadir = "testdata/nssdbdir"; + let destdir = format!("{}/{}", TESTDIR, name); + + let dbargs = format!("configDir={}", destdir); + let dbtype = "nssdb"; + + /* allocates a unique slotid to use in the tests */ + let mut testtokn = + TestToken::new_type(String::from(dbtype), String::from(""), name); + + /* Do this after TestToken::new() otherwise the data + * is wiped away by the initialization code */ + std::fs::create_dir_all(destdir.clone()).unwrap(); + assert!(std::fs::copy( + format!("{}/cert9.db", datadir), + format!("{}/cert9.db", destdir), + ) + .is_ok()); + assert!(std::fs::copy( + format!("{}/key4.db", datadir), + format!("{}/key4.db", destdir), + ) + .is_ok()); + assert!(std::fs::copy( + format!("{}/pkcs11.txt", datadir), + format!("{}/pkcs11.txt", destdir), + ) + .is_ok()); + + /* pre-populate conf so we get the correct slot number assigned */ + let mut slot = config::Slot::with_db(dbtype, Some(dbargs.clone())); + slot.slot = u32::try_from(testtokn.get_slot()).unwrap(); + let ret = add_slot(slot); + assert_eq!(ret, CKR_OK); + + let mut args = TestToken::make_init_args(Some(dbargs.clone())); + let args_ptr = &mut args as *mut CK_C_INITIALIZE_ARGS; + let ret = fn_initialize(args_ptr as *mut std::ffi::c_void); + assert_eq!(ret, CKR_OK); + + let _ = testtokn.get_session(false); + for _ in 0..1000 { + testtokn.login(); + testtokn.logout(); + } + + testtokn.finalize(); +} From f7b0457866350a96e6597bb827ab1b5f9e46450b Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Fri, 13 Dec 2024 17:25:12 -0500 Subject: [PATCH 3/6] Set a ceiling to the number of keys in cache This is a very simple way to avoid unbounded memory pressure, however it has no smarts to keep in cache the most used keys. It simply kicks out the last key whch is the key that happens to have the "higher" salt. It can cause performance degradation if a token has very many keys and keeps trying to access a roration of those with a "higher" salt. Signed-off-by: Simo Sorce --- src/storage/nssdb/ci.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/storage/nssdb/ci.rs b/src/storage/nssdb/ci.rs index 5914d01..8db345c 100644 --- a/src/storage/nssdb/ci.rs +++ b/src/storage/nssdb/ci.rs @@ -18,6 +18,7 @@ use crate::{sizeof, void_ptr}; use zeroize::Zeroize; const SHA256_LEN: usize = 32; +const MAX_KEY_CACHE_SIZE: usize = 128; enum KeyOp { Encryption, @@ -108,11 +109,15 @@ impl KeysWithCaching { } fn set_cached_key(&self, id: &[u8; SHA256_LEN], key: Object) -> Result<()> { - let mut w = match self.cache.write() { - Ok(w) => w, + match self.cache.write() { + Ok(mut w) => { + if w.len() > MAX_KEY_CACHE_SIZE { + let _ = w.pop_last(); + } + let _ = w.insert(*id, key); + } Err(_) => return Err(CKR_CANT_LOCK)?, }; - let _ = w.insert(*id, key); Ok(()) } From 8d23be322cc538d101a97d9fd52055fa991e2bf6 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Fri, 13 Dec 2024 17:41:29 -0500 Subject: [PATCH 4/6] Reset the key cache on pin changes Signed-off-by: Simo Sorce --- src/storage/nssdb/mod.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/storage/nssdb/mod.rs b/src/storage/nssdb/mod.rs index d71dd33..ea2e8d8 100644 --- a/src/storage/nssdb/mod.rs +++ b/src/storage/nssdb/mod.rs @@ -1187,8 +1187,8 @@ impl Storage for NSSStorage { CSPRNG.with(|rng| rng.borrow_mut().generate_random(&mut salt))?; let enckey = enckey_derive(facilities, pin, &salt)?; - let mut tmpkeys = KeysWithCaching::default(); - tmpkeys.set_key(enckey); + let mut newkeys = KeysWithCaching::default(); + newkeys.set_key(enckey); let iterations = match pin.len() { 0 => 1, @@ -1198,10 +1198,15 @@ impl Storage for NSSStorage { } }; let mut encdata = - encrypt_data(facilities, &tmpkeys, iterations, NSS_PASS_CHECK)?; + encrypt_data(facilities, &newkeys, iterations, NSS_PASS_CHECK)?; /* FIXME: need to re-encode all encrypted/integrity protected attributes */ + /* now that the pin has changed all cached keys are invalid, replace the lot */ + self.keys = newkeys; + /* changing the pin does not leave the token logged in */ + self.keys.unset_key(); + let result = self.save_password(&salt, encdata.as_slice()); encdata.zeroize(); result From d263818c6093489100341bc22753d47994ffb221 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Mon, 16 Dec 2024 10:00:00 -0500 Subject: [PATCH 5/6] Ensure keys are zerized on replacement. Signed-off-by: Simo Sorce --- src/storage/nssdb/ci.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/storage/nssdb/ci.rs b/src/storage/nssdb/ci.rs index 8db345c..02930dd 100644 --- a/src/storage/nssdb/ci.rs +++ b/src/storage/nssdb/ci.rs @@ -84,11 +84,17 @@ impl KeysWithCaching { } pub fn set_key(&mut self, key: Vec) { + if let Some(ref mut oldkey) = &mut self.enckey { + oldkey.zeroize(); + } self.enckey = Some(key); } pub fn unset_key(&mut self) { - self.enckey = None; + if let Some(ref mut key) = &mut self.enckey { + key.zeroize(); + self.enckey = None; + } } fn get_cached_key(&self, id: &[u8; SHA256_LEN]) -> Option { From 7ef98941ad1c1c00f8174f5f90d00e75a2db14b5 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Mon, 16 Dec 2024 10:29:39 -0500 Subject: [PATCH 6/6] Update github action artifacts to v4 v3 is deprecated and will be soon non-functional Signed-off-by: Simo Sorce --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 3ea508c..b570aa2 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -101,7 +101,7 @@ jobs: cargo build -vv $OPTS --features "$FEATURES" cargo test -vv $OPTS --features "$FEATURES" - - uses: actions/upload-artifact@v3 + - uses: actions/upload-artifact@v4 if: failure() with: name: Build logs ${{ matrix.name }}