diff --git a/Cargo.lock b/Cargo.lock index 31b802cfc..f6127a681 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -48,6 +48,15 @@ dependencies = [ "zeroize", ] +[[package]] +name = "aes-kw" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "69fa2b352dcefb5f7f3a5fb840e02665d311d878955380515e4fd50095dd3d8c" +dependencies = [ + "aes", +] + [[package]] name = "aes-test" version = "0.1.0" @@ -2864,6 +2873,7 @@ version = "0.1.0" dependencies = [ "aes", "aes-gcm-siv", + "aes-kw", "blowfish 0.8.0", "byteorder", "cipher 0.4.3", diff --git a/services/pddb/src/backend/hw.rs b/services/pddb/src/backend/hw.rs index 3fdfcdfe3..fd4867962 100644 --- a/services/pddb/src/backend/hw.rs +++ b/services/pddb/src/backend/hw.rs @@ -572,6 +572,7 @@ impl PddbOs { self.rootkeys.clear_password(AesRootkeyType::User0); } pub (crate) fn try_login(&mut self) -> PasswordState { + use root_keys::api::KeywrapError; if self.system_basis_key.is_none() || self.cipher_ecb.is_none() { let scd = self.static_crypto_data_get(); if scd.version == 0xFFFF_FFFF { // system is in the blank state @@ -581,56 +582,93 @@ impl PddbOs { log::error!("Version mismatch for keystore, declaring database as uninitialized"); return PasswordState::Uninit; } - match self.rootkeys.unwrap_key(&scd.system_key_pt, AES_KEYSIZE) { - Ok(mut syskey_pt) => { - match self.rootkeys.unwrap_key(&scd.system_key, AES_KEYSIZE) { - Ok(mut syskey) => { - let cipher = Aes256::new(GenericArray::from_slice(&syskey_pt)); - self.cipher_ecb = Some(cipher); - let mut system_key_pt: [u8; AES_KEYSIZE] = [0; AES_KEYSIZE]; - // copy over the pt key only after we've unwrapped and decrypted the data key - for (&src, dst) in syskey_pt.iter().zip(system_key_pt.iter_mut()) { - *dst = src; - } - // copy over the data key - let mut system_key: [u8; AES_KEYSIZE] = [0; AES_KEYSIZE]; - for (&src, dst) in syskey.iter().zip(system_key.iter_mut()) { - *dst = src; - } + // prep a migration structure, in case it is required. Bit of history: + // + // Turns out that the keywrap implementation we found does not work. It does not generate + // results that match the NIST test vectors at + // https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/mac/kwtestvectors.zip + // + // The `aes_kw` functions in the RustCrypto library do create matching results. + // However, back when the PDDB was written, this crate didn't exist (the first commit + // to `aes_kw` was Jan 2022, the PDDB was started back in October 2021, and around + // the time I searched for an aes-kw implementation, the `aes_kw` code was just pushed + // at 0.1.0, and not registering in Google). + // + // Ah well. Better we caught it now than later! This structure here prepares for + // a potential migration in case we are told we need to do that. The main regression + // we now suffer is that the key return structure is not zeroized on drop automatically, + // because we are using a fancy compound enum and our version of zeroize is ancient + // due to interpedencies with other crypto crates. + let mut new_crypto_keys = StaticCryptoData::default(); + new_crypto_keys.version = scd.version; + new_crypto_keys.system_key_pt.copy_from_slice(&scd.system_key_pt); + new_crypto_keys.system_key.copy_from_slice(&scd.system_key); + new_crypto_keys.salt_base.copy_from_slice(&scd.salt_base); - self.system_basis_key = Some( - BasisKeys { - data: system_key, - pt: system_key_pt, - } - ); - // erase the old vector completely - let nuke = syskey.as_mut_ptr(); - for i in 0..syskey.len() { - unsafe{nuke.add(i).write_volatile(0)}; - } - let nuke = syskey_pt.as_mut_ptr(); - for i in 0..syskey_pt.len() { - unsafe{nuke.add(i).write_volatile(0)}; - } - PasswordState::Correct - } - Err(e) => { - // the PT key is still lingering on this arm, make sure we get rid of that - let nuke = syskey_pt.as_mut_ptr(); - for i in 0..syskey_pt.len() { - unsafe{nuke.add(i).write_volatile(0)}; - } - log::error!("Couldn't unwrap our system key: {:?}", e); - PasswordState::Incorrect - } + // now try to populate our keys, and prep a migration if necessary + let mut syskey_pt = [0u8; 32]; + let mut syskey = [0u8; 32]; + let mut keys_updated = false; + match self.rootkeys.unwrap_key(&scd.system_key_pt, AES_KEYSIZE) { + Ok(skpt) => syskey_pt.copy_from_slice(&skpt), + Err(e) => match e { + KeywrapError::UpgradeToNew((key, upgrade)) => { + log::warn!("pt key migration required"); + syskey_pt.copy_from_slice(&key); + new_crypto_keys.system_key_pt.copy_from_slice(&upgrade); + keys_updated = true; + } + _ => { + log::error!("Couldn't unwrap our system key: {:?}", e); + return PasswordState::Incorrect; } } - Err(e) => { - log::error!("Couldn't unwrap our system key: {:?}", e); - PasswordState::Incorrect + } + match self.rootkeys.unwrap_key(&scd.system_key, AES_KEYSIZE) { + Ok(sk) => syskey.copy_from_slice(&sk), + Err(e) => match e { + KeywrapError::UpgradeToNew((key, upgrade)) => { + log::warn!("data key migration required"); + syskey.copy_from_slice(&key); + new_crypto_keys.system_key.copy_from_slice(&upgrade); + keys_updated = true; + } + _ => { + log::error!("Couldn't unwrap our system key: {:?}", e); + return PasswordState::Incorrect; + } } } + if keys_updated { + log::warn!("Migration event from incorrectly wrapped key"); + self.patch_keys(new_crypto_keys.deref(), 0); + } + + let cipher = Aes256::new(GenericArray::from_slice(&syskey_pt)); + self.cipher_ecb = Some(cipher); + let mut system_key_pt: [u8; AES_KEYSIZE] = [0; AES_KEYSIZE]; + // copy over the pt key only after we've unwrapped and decrypted the data key + system_key_pt.copy_from_slice(&syskey_pt); + // copy over the data key + let mut system_key: [u8; AES_KEYSIZE] = [0; AES_KEYSIZE]; + system_key.copy_from_slice(&syskey); + + self.system_basis_key = Some( + BasisKeys { + data: system_key, + pt: system_key_pt, + } + ); + // erase the plaintext keys + let nuke = syskey.as_mut_ptr(); + for i in 0..syskey.len() { + unsafe{nuke.add(i).write_volatile(0)}; + } + let nuke = syskey_pt.as_mut_ptr(); + for i in 0..syskey_pt.len() { + unsafe{nuke.add(i).write_volatile(0)}; + } + PasswordState::Correct } else { PasswordState::Correct } diff --git a/services/root-keys/Cargo.toml b/services/root-keys/Cargo.toml index f37b2efe2..7324d6894 100644 --- a/services/root-keys/Cargo.toml +++ b/services/root-keys/Cargo.toml @@ -25,6 +25,8 @@ num-traits = {version = "0.2.14", default-features = false} rkyv = {version = "0.4.3", default-features = false, features = ["const_generics"]} zeroize = "1.3.0" +aes-kw = {version = "0.2.1", features = ["alloc"]} + # bcrypt cipher = "0.4.3" blowfish = { version = "0.8.0", features = ["bcrypt"] } diff --git a/services/root-keys/src/api.rs b/services/root-keys/src/api.rs index 2b21176b0..81c35a058 100644 --- a/services/root-keys/src/api.rs +++ b/services/root-keys/src/api.rs @@ -192,20 +192,18 @@ impl AesOp { } } -#[derive(Debug, rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Zeroize, Eq, PartialEq, Copy, Clone)] +#[derive(Debug, rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Eq, PartialEq, Copy, Clone)] pub enum KeywrapError { - /// Input is too big. - TooBig = 0, - /// Input is too small. - TooSmall = 1, - /// Ciphertext has invalid padding. - Unpadded = 2, - /// The ciphertext is not valid for the expected length. - InvalidExpectedLen = 3, - /// The ciphertext couldn't be authenticated. - AuthenticationFailed = 4, + InvalidDataSize, + InvalidKekSize, + InvalidOutputSize, + IntegrityCheckFailed, + /// this is a bodge to return an error code that upgrades from a faulty early version of AES-KWP + /// only works for 256-bit keys, but that is also all we used. + /// The return tuple is: (unwrapped key, correctly wrapped key) + UpgradeToNew(([u8; 32], [u8; 40])), } -#[derive(Debug, rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Zeroize, Eq, PartialEq)] +#[derive(Debug, rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Eq, PartialEq)] pub enum KeyWrapOp { Wrap = 0, Unwrap = 1, @@ -219,18 +217,21 @@ use std::fmt; impl fmt::Display for KeywrapError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { match self { - KeywrapError::TooBig => f.write_str("Input too big"), - KeywrapError::TooSmall => f.write_str("Input too small"), - KeywrapError::Unpadded => f.write_str("Padding error"), - KeywrapError::InvalidExpectedLen => f.write_str("Invalid expected lengthr"), - KeywrapError::AuthenticationFailed => f.write_str("Authentication failed"), + KeywrapError::InvalidDataSize => f.write_str("Invalid data size"), + KeywrapError::InvalidKekSize => f.write_str("Invalid key size"), + KeywrapError::InvalidOutputSize => f.write_str("Invalid output size"), + KeywrapError::IntegrityCheckFailed => f.write_str("Authentication failed"), + KeywrapError::UpgradeToNew((_k, _wk)) => f.write_str("Legacy migration detected! New wrapped key transmitted to caller"), } } } pub(crate) const MAX_WRAP_DATA: usize = 2048; -#[derive(Debug, rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Zeroize)] -#[zeroize(drop)] +/// Note regression in v0.9.9: we had to return an array type in the KeywrapError enum that +/// has a signature for an array that is 40 bytes long, which is bigger than Rust's devire +/// can deal with. So, unfortunately, the result of this does *not* get zeroized on drop :( +#[derive(Debug, rkyv::Archive, rkyv::Serialize, rkyv::Deserialize)] +// #[zeroize(drop)] pub (crate) struct KeyWrapper { pub data: [u8; MAX_WRAP_DATA + 8], // used to specify the length of the data used in the fixed-length array above diff --git a/services/root-keys/src/implementation.rs b/services/root-keys/src/implementation.rs index ea94018df..e30364ed8 100644 --- a/services/root-keys/src/implementation.rs +++ b/services/root-keys/src/implementation.rs @@ -421,10 +421,12 @@ impl<'a> RootKeys { self.compute_key_rollback(&mut key); #[cfg(feature = "hazardous-debug")] log::debug!("root user key (anti-rollback): {:x?}", key); - let keywrapper = Aes256KeyWrap::new(&key); + use aes_kw::Kek; + use aes_kw::KekAes256; + let keywrapper: KekAes256 = Kek::from(key); match kwp.op { KeyWrapOp::Wrap => { - match keywrapper.encapsulate(&kwp.data[..kwp.len as usize]) { + match keywrapper.wrap_with_padding_vec(&kwp.data[..kwp.len as usize]) { Ok(wrapped) => { for (&src, dst) in wrapped.iter().zip(kwp.data.iter_mut()) { *dst = src; @@ -435,21 +437,70 @@ impl<'a> RootKeys { kwp.expected_len = wrapped.len() as u32; } Err(e) => { - kwp.result = Some(e); + kwp.result = Some(match e { + aes_kw::Error::IntegrityCheckFailed => KeywrapError::IntegrityCheckFailed, + aes_kw::Error::InvalidDataSize => KeywrapError::InvalidDataSize, + aes_kw::Error::InvalidKekSize { size } => { + log::info!("invalid size {}", size); // weird. can't name this _size + KeywrapError::InvalidKekSize + }, + aes_kw::Error::InvalidOutputSize { expected } => { + log::info!("invalid output size {}", expected); + KeywrapError::InvalidOutputSize + }, + }); } } } KeyWrapOp::Unwrap => { - match keywrapper.decapsulate(&kwp.data[..kwp.len as usize], kwp.expected_len as usize) { - Ok(unwrapped) => { - for (&src, dst) in unwrapped.iter().zip(kwp.data.iter_mut()) { + match keywrapper.unwrap_with_padding_vec(&kwp.data[..kwp.len as usize]) { + Ok(wrapped) => { + for (&src, dst) in wrapped.iter().zip(kwp.data.iter_mut()) { *dst = src; } - kwp.len = unwrapped.len() as u32; + kwp.len = wrapped.len() as u32; kwp.result = None; + // this is an un-used field but...why not? + kwp.expected_len = wrapped.len() as u32; } Err(e) => { - kwp.result = Some(e); + kwp.result = Some(match e { + aes_kw::Error::IntegrityCheckFailed => { + // try the legacy version, if it unwraps, send back the key + an error that indicates the caller needs to update their version + let legacy_kw = Aes256KeyWrap::new(&key); + match legacy_kw.decapsulate(&kwp.data[..kwp.len as usize], kwp.expected_len as usize) { + Ok(unwrapped) => { + for (&src, dst) in unwrapped.iter().zip(kwp.data.iter_mut()) { + *dst = src; + } + kwp.len = unwrapped.len() as u32; + // hand the caller back a new version of the wrapped key that meets NIST specs. + let corrected = keywrapper.wrap_with_padding_vec(&unwrapped).expect("couldn't convert to correct version of AES keywrapping"); + let mut upgrade = [0u8; 40]; + assert!(corrected.len() == 40, "Correctly wrapped key has a different length than the legacy wrapped key"); + upgrade.copy_from_slice(&corrected); + + let mut unwrapped_key = [0u8; 32]; + assert!(kwp.len == 32, "Unwrapped key from legacy version of algorithm has the wrong length"); + unwrapped_key.copy_from_slice(&unwrapped); + log::warn!("Keywrap from incorrect version of algorithm; sending message to correct the problem"); + KeywrapError::UpgradeToNew((unwrapped_key, upgrade)) + } + Err(e) => { + e + } + } + }, + aes_kw::Error::InvalidDataSize => KeywrapError::InvalidDataSize, + aes_kw::Error::InvalidKekSize { size } => { + log::info!("invalid size {}", size); // weird. can't name this _size + KeywrapError::InvalidKekSize + }, + aes_kw::Error::InvalidOutputSize { expected } => { + log::info!("invalid output size {}", expected); + KeywrapError::InvalidOutputSize + }, + }); } } } diff --git a/services/root-keys/src/implementation/keywrap.rs b/services/root-keys/src/implementation/keywrap.rs index 97859530e..998583e5d 100644 --- a/services/root-keys/src/implementation/keywrap.rs +++ b/services/root-keys/src/implementation/keywrap.rs @@ -58,11 +58,12 @@ impl Aes256KeyWrap { } } + #[allow(dead_code)] pub fn encapsulate(&self, input: &[u8]) -> Result, KeywrapError> { if input.len() > std::u32::MAX as usize || input.len() as u64 >= std::u64::MAX / FEISTEL_ROUNDS as u64 { - return Err(KeywrapError::TooBig); + return Err(KeywrapError::InvalidDataSize); } let mut aiv: [u8; 8] = [0xa6u8, 0x59, 0x59, 0xa6, 0, 0, 0, 0]; BigEndian::write_u32(&mut aiv[4..8], input.len() as u32); @@ -101,19 +102,19 @@ impl Aes256KeyWrap { pub fn decapsulate(&self, input: &[u8], expected_len: usize) -> Result, KeywrapError> { if input.len() % 8 != 0 { - return Err(KeywrapError::Unpadded); + return Err(KeywrapError::InvalidDataSize); } let output_len = input .len() .checked_sub(Self::MAC_BYTES) - .ok_or(KeywrapError::TooSmall)?; + .ok_or(KeywrapError::InvalidOutputSize)?; if output_len > std::u32::MAX as usize || output_len as u64 >= std::u64::MAX / FEISTEL_ROUNDS as u64 { - return Err(KeywrapError::TooBig); + return Err(KeywrapError::InvalidDataSize); } if expected_len > output_len || (expected_len & !7) > output_len { - return Err(KeywrapError::InvalidExpectedLen); + return Err(KeywrapError::InvalidDataSize); } let mut output = vec![0u8; output_len]; let mut aiv: [u8; 8] = [0xa6u8, 0x59, 0x59, 0xa6, 0, 0, 0, 0]; @@ -130,7 +131,7 @@ impl Aes256KeyWrap { .zip(aiv.iter()) .fold(0, |acc, (a, b)| acc | (a ^ b)); if c != 0 { - return Err(KeywrapError::AuthenticationFailed); + return Err(KeywrapError::IntegrityCheckFailed); } output[0..8].copy_from_slice(&block[8..16]); return Ok(output); @@ -160,7 +161,7 @@ impl Aes256KeyWrap { .zip(aiv.iter()) .fold(0, |acc, (a, b)| acc | (a ^ b)); if c != 0 { - return Err(KeywrapError::AuthenticationFailed); + return Err(KeywrapError::IntegrityCheckFailed); } Ok(output) } diff --git a/services/root-keys/src/lib.rs b/services/root-keys/src/lib.rs index 3e32d1d80..3c9aeea86 100644 --- a/services/root-keys/src/lib.rs +++ b/services/root-keys/src/lib.rs @@ -335,25 +335,25 @@ impl RootKeys { // but the intention of this API is to wrap crypto keys -- not bulk data. So for simplicity // we're going to limit the size of wrapped data to 2kiB (typically it's envisioned you're // wrapping data on the order of 32-512 bytes) - return Err(KeywrapError::TooBig) + return Err(KeywrapError::InvalidDataSize) } if !self.ensure_aes_password() { - return Err(KeywrapError::AuthenticationFailed); + return Err(KeywrapError::IntegrityCheckFailed); } let mut alloc = KeyWrapper { data: [0u8; MAX_WRAP_DATA + 8], len: input.len() as u32, key_index: self.key_index.to_u8().unwrap(), op: KeyWrapOp::Wrap, - result: Some(KeywrapError::AuthenticationFailed), // initialize to a default value that throws an error if it wasn't modified by the recipient + result: Some(KeywrapError::IntegrityCheckFailed), // initialize to a default value that throws an error if it wasn't modified by the recipient // this field is ignored on the Wrap side expected_len: 0, }; for (&src, dst) in input.iter().zip(alloc.data.iter_mut()) { *dst = src; } - let mut buf = Buffer::into_buf(alloc).or(Err(KeywrapError::AuthenticationFailed))?; - buf.lend_mut(self.conn, Opcode::AesKwp.to_u32().unwrap()).or(Err(KeywrapError::AuthenticationFailed))?; + let mut buf = Buffer::into_buf(alloc).or(Err(KeywrapError::IntegrityCheckFailed))?; + buf.lend_mut(self.conn, Opcode::AesKwp.to_u32().unwrap()).or(Err(KeywrapError::IntegrityCheckFailed))?; let ret = buf.to_original::().unwrap(); match ret.result { None => { // no error, proceed @@ -366,32 +366,34 @@ impl RootKeys { } pub fn unwrap_key(&self, wrapped: &[u8], expected_len: usize) -> Result, KeywrapError> { if wrapped.len() > api::MAX_WRAP_DATA + 8 { - return Err(KeywrapError::TooBig) + return Err(KeywrapError::InvalidDataSize) } if !self.ensure_aes_password() { - return Err(KeywrapError::AuthenticationFailed); + return Err(KeywrapError::IntegrityCheckFailed); } let mut alloc = KeyWrapper { data: [0u8; MAX_WRAP_DATA + 8], len: wrapped.len() as u32, key_index: self.key_index.to_u8().unwrap(), op: KeyWrapOp::Unwrap, - result: Some(KeywrapError::AuthenticationFailed), // initialize to a default value that throws an error if it wasn't modified by the recipient + result: Some(KeywrapError::IntegrityCheckFailed), // initialize to a default value that throws an error if it wasn't modified by the recipient expected_len: expected_len as u32, }; for (&src, dst) in wrapped.iter().zip(alloc.data.iter_mut()) { *dst = src; } - let mut buf = Buffer::into_buf(alloc).or(Err(KeywrapError::AuthenticationFailed))?; - buf.lend_mut(self.conn, Opcode::AesKwp.to_u32().unwrap()).or(Err(KeywrapError::AuthenticationFailed))?; + let mut buf = Buffer::into_buf(alloc).or(Err(KeywrapError::IntegrityCheckFailed))?; + buf.lend_mut(self.conn, Opcode::AesKwp.to_u32().unwrap()).or(Err(KeywrapError::IntegrityCheckFailed))?; let ret = buf.to_original::().unwrap(); + // note: this return vector (which may contain a key) is not zeroized on drop anymore due to a + // regression correlated to a fix to a crypto algorithm. This was pushed as a hotfix for v0.9.9. match ret.result { None => { // no error, proceed if ret.len as usize != expected_len { // The key wrapper will return a vector that is rounded to the nearest 8-bytes, with // a zero-pad on the excess data. Ensure that the zero-pad is intact before lopping it off. if (ret.len as usize) < expected_len { - Err(KeywrapError::InvalidExpectedLen) + Err(KeywrapError::InvalidOutputSize) } else { let mut all_zeroes = true; for &d in ret.data[expected_len..ret.len as usize].iter() { @@ -403,7 +405,7 @@ impl RootKeys { if all_zeroes { Ok(ret.data[..expected_len as usize].to_vec(),) } else { - Err(KeywrapError::InvalidExpectedLen) + Err(KeywrapError::InvalidDataSize) } } } else { @@ -411,6 +413,10 @@ impl RootKeys { } } Some(err) => { + // note: the error message may contain a plaintext copy of the key that is not + // zeroized on drop. However, it only happens in the case of a migration from + // a legacy version (pre v0.9.8) PDDB, by the time this matters it should be + // a non-issue. Err(err) } }