saymrwulf/xous-core
1
0
Fork 0
mirror of https://github.com/saymrwulf/xous-core.git synced 2026-11-08 04:56:43 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

hotfix for incorrect AES-KWP implementation

Browse source 
The crate we originally grabbed for AES-KWP seems to be non-compliant
(or I'm an idiot and I can't figure out how to call it in a way
that is compliant).

See https://github.com/jedisct1/rust-aes-keywrap/issues/9

Turns out that `aes_kw` *is* compliant, but unfortunately, it didn't
exist back when the PDDB was being written. The PDDB started in October
2021, and `aes_kw` was pushed in Jan 2022. *just* barely missed it.

Anyways, caught this error while trying to implement some CI for
backups. Hurray for testing!

This patch removes the encryption path using the incorrect version
of the algorithm, but leaves the decryption path in place. The
system will try to decrypt initially with the correct version,
but if that fails, it will fall back and try the incorrect version.
If that succeeds, it returns an error code with the correct key and
an updated wrapped key, which the caller can store so that future
calls do not fail. Thus, we have a seamless migration from the
incorrect version to the correct version.

Have verified that the aes_kw version *does* work with the CI
script, insofar as it has been written (we just got to that point
and got stuck). So, full speed ahead. Users shouldn't notice this
change.

Also, fwiw, I did take a look at the "incorrect" implementation,
and it doesn't look obviously insecure. Printing out the various
rounds of the wrapping function looks like the first couple rounds
match, but it diverges somewhere probably due to an off-by-one
in a counter or something like that. I didn't chase it out -- just
wanted to make sure that nothing looked like a huge data compromise.

Good news is this comes right on the heels of v0.9.9 release, so
this will be pushed as a hotfix right away.
This commit is contained in:
bunnie 2022-07-25 02:56:20 +08:00
parent c0bd7f7f8a
commit ec5cc29ccd
7 changed files with 200 additions and 91 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
Cargo.lock generated
View file

@ -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",

128
services/pddb/src/backend/hw.rs
View file

@ -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
}

2
services/root-keys/Cargo.toml
View file

@ -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"] }

39
services/root-keys/src/api.rs
View file

@ -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

67
services/root-keys/src/implementation.rs
View file

@ -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
},
});
}
}
}

15
services/root-keys/src/implementation/keywrap.rs
View file

@ -58,11 +58,12 @@ impl Aes256KeyWrap {
}
}
#[allow(dead_code)]
pub fn encapsulate(&self, input: &[u8]) -> Result<Vec<u8>, 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<Vec<u8>, 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)
}

30
services/root-keys/src/lib.rs
View file

@ -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::<KeyWrapper, _>().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<Vec<u8>, 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::<KeyWrapper, _>().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)
}
}