From c1ff9e7811192c16ce931c1dab36fbcafd142bba Mon Sep 17 00:00:00 2001 From: Maximiliano Sandoval Date: Mon, 28 Jul 2025 21:07:00 +0200 Subject: [PATCH 1/2] client: Add new Mac struct for constant time eq Prevents timing attacks [1] that can be used to guess possible macs. The subtle crate is already a dependency of digest. [1] https://en.wikipedia.org/wiki/Timing_attack --- client/src/crypto/native.rs | 4 +-- client/src/crypto/openssl.rs | 11 +++----- client/src/file/api/attribute_value.rs | 4 +-- client/src/file/api/encrypted_item.rs | 16 +++++++---- client/src/file/item.rs | 6 ++-- client/src/lib.rs | 4 ++- client/src/mac.rs | 39 ++++++++++++++++++++++++++ 7 files changed, 63 insertions(+), 21 deletions(-) create mode 100644 client/src/mac.rs diff --git a/client/src/crypto/native.rs b/client/src/crypto/native.rs index 1078fbe75..b3fc8691e 100644 --- a/client/src/crypto/native.rs +++ b/client/src/crypto/native.rs @@ -123,10 +123,10 @@ pub(crate) fn mac_len() -> usize { MacAlg::output_size() } -pub(crate) fn compute_mac(data: impl AsRef<[u8]>, key: &Key) -> Result, super::Error> { +pub(crate) fn compute_mac(data: impl AsRef<[u8]>, key: &Key) -> Result { let mut mac = MacAlg::new_from_slice(key.as_ref()).unwrap(); mac.update(data.as_ref()); - Ok(mac.finalize().into_bytes().to_vec()) + Ok(crate::Mac::new(mac.finalize().into_bytes().to_vec())) } pub(crate) fn verify_mac( diff --git a/client/src/crypto/openssl.rs b/client/src/crypto/openssl.rs index 1b6ba2a96..c0910de71 100644 --- a/client/src/crypto/openssl.rs +++ b/client/src/crypto/openssl.rs @@ -14,7 +14,7 @@ use openssl::{ }; use zeroize::Zeroizing; -use crate::{Key, file}; +use crate::{Key, Mac, file}; const ENC_ALG: Nid = Nid::AES_128_CBC; const MAC_ALG: Nid = Nid::SHA256; @@ -142,12 +142,12 @@ pub(crate) fn mac_len() -> usize { md.size() } -pub(crate) fn compute_mac(data: impl AsRef<[u8]>, key: &Key) -> Result, super::Error> { +pub(crate) fn compute_mac(data: impl AsRef<[u8]>, key: &Key) -> Result { let md = MessageDigest::from_nid(MAC_ALG).unwrap(); let mac_key = PKey::hmac(key.as_ref())?; let mut signer = Signer::new(md, &mac_key)?; signer.update(data.as_ref())?; - signer.sign_to_vec().map_err(From::from) + signer.sign_to_vec().map_err(From::from).map(Mac::new) } pub(crate) fn verify_mac( @@ -155,10 +155,7 @@ pub(crate) fn verify_mac( key: &Key, expected: impl AsRef<[u8]>, ) -> Result { - Ok(memcmp::eq( - compute_mac(&data, key)?.as_slice(), - expected.as_ref(), - )) + Ok(memcmp::eq(compute_mac(&data, key)?.as_slice(), expected.as_ref())) } pub(crate) fn verify_checksum_md5(digest: impl AsRef<[u8]>, content: impl AsRef<[u8]>) -> bool { diff --git a/client/src/file/api/attribute_value.rs b/client/src/file/api/attribute_value.rs index eb6790545..74e8e29ec 100644 --- a/client/src/file/api/attribute_value.rs +++ b/client/src/file/api/attribute_value.rs @@ -2,14 +2,14 @@ use serde::{Deserialize, Serialize}; use zbus::zvariant::Type; use zeroize::{Zeroize, ZeroizeOnDrop}; -use crate::{Key, crypto}; +use crate::{Key, Mac, crypto}; /// An attribute value. #[derive(Deserialize, Serialize, Type, Clone, Debug, Eq, PartialEq, Zeroize, ZeroizeOnDrop)] pub struct AttributeValue(String); impl AttributeValue { - pub(crate) fn mac(&self, key: &Key) -> Result, crate::crypto::Error> { + pub(crate) fn mac(&self, key: &Key) -> Result { crypto::compute_mac(self.0.as_bytes(), key) } } diff --git a/client/src/file/api/encrypted_item.rs b/client/src/file/api/encrypted_item.rs index f3e4a1197..6d801b686 100644 --- a/client/src/file/api/encrypted_item.rs +++ b/client/src/file/api/encrypted_item.rs @@ -4,17 +4,17 @@ use serde::{Deserialize, Serialize}; use zbus::zvariant::Type; use super::{Error, Item}; -use crate::{Key, crypto}; +use crate::{Key, Mac, crypto}; #[derive(Deserialize, Serialize, Type, Debug, Clone)] pub(crate) struct EncryptedItem { - pub(crate) hashed_attributes: HashMap>, + pub(crate) hashed_attributes: HashMap, pub(crate) blob: Vec, } impl EncryptedItem { - pub fn has_attribute(&self, key: &str, value_mac: &[u8]) -> bool { - self.hashed_attributes.get(key).map(|b| b.as_slice()) == Some(value_mac) + pub fn has_attribute(&self, key: &str, value_mac: &Mac) -> bool { + self.hashed_attributes.get(key) == Some(value_mac) } pub fn decrypt(self, key: &Key) -> Result { @@ -43,13 +43,17 @@ impl EncryptedItem { } fn validate( - hashed_attributes: &HashMap>, + hashed_attributes: &HashMap, item: &Item, key: &Key, ) -> Result<(), Error> { for (attribute_key, hashed_attribute) in hashed_attributes.iter() { if let Some(attribute_plaintext) = item.attributes().get(attribute_key) { - if !crypto::verify_mac(attribute_plaintext.as_bytes(), key, hashed_attribute)? { + if !crypto::verify_mac( + attribute_plaintext.as_bytes(), + key, + hashed_attribute.as_slice(), + )? { return Err(Error::HashedAttributeMac(attribute_key.to_owned())); } } else { diff --git a/client/src/file/item.rs b/client/src/file/item.rs index ecf00a0a7..2a8c7af92 100644 --- a/client/src/file/item.rs +++ b/client/src/file/item.rs @@ -155,8 +155,8 @@ impl Item { let mut blob = crypto::encrypt(&*decrypted, key, iv)?; blob.extend_from_slice(iv); - let mut mac = crypto::compute_mac(&blob, key)?; - blob.append(&mut mac); + let mac = crypto::compute_mac(&blob, key)?; + blob.extend_from_slice(mac.as_slice()); let hashed_attributes = self .attributes @@ -228,7 +228,7 @@ mod tests { let encrypted_item_blob = &encrypted.blob[..n - n_mac - n_iv]; let item_mac = crypto::compute_mac(&encrypted.blob[..n - n_mac], &key).unwrap(); - assert_eq!(&blob[n - n_mac..], &item_mac); + assert_eq!(&blob[n - n_mac..], item_mac.as_slice()); assert_eq!(&blob[n - n_mac - n_iv..n - n_mac], &iv); assert_eq!( encrypted_item_blob, diff --git a/client/src/lib.rs b/client/src/lib.rs index ddef7c098..bb566f078 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -18,6 +18,7 @@ use std::collections::HashMap; mod error; mod key; +mod mac; mod migration; #[cfg(feature = "unstable")] @@ -25,6 +26,7 @@ mod migration; pub use key::Key; #[cfg(not(feature = "unstable"))] pub(crate) use key::Key; +pub use mac::Mac; #[cfg(not(feature = "unstable"))] mod crypto; @@ -63,7 +65,7 @@ pub trait AsAttributes { fn hash<'a>( &'a self, key: &Key, - ) -> Vec<(&'a str, std::result::Result, crate::crypto::Error>)> { + ) -> Vec<(&'a str, std::result::Result)> { self.as_attributes() .into_iter() .map(|(k, v)| (k, crate::file::AttributeValue::from(v).mac(key))) diff --git a/client/src/mac.rs b/client/src/mac.rs new file mode 100644 index 000000000..088cd1c39 --- /dev/null +++ b/client/src/mac.rs @@ -0,0 +1,39 @@ +use serde::{Deserialize, Serialize}; +#[cfg(feature = "native_crypto")] +use subtle::ConstantTimeEq; +use zbus::zvariant::Type; + +// There is no constructor to avoid performing sanity checks, e.g. length. +/// A message authentication code. It provides constant-time comparison when +/// compared against another mac or against a slice of bytes. +#[derive(Deserialize, Serialize, Type, Debug, Clone)] +pub struct Mac(Vec); + +impl Mac { + pub(crate) fn new(inner: Vec) -> Self { + Mac(inner) + } + + /// Constant-time comparison against a slice of bytes. + pub fn verify_slice(&self, other: &[u8]) -> bool { + #[cfg(feature = "native_crypto")] + { + self.0.ct_eq(other).into() + } + #[cfg(feature = "openssl_crypto")] + { + openssl::memcmp::eq(&self.0, other) + } + } + + // This is made private to prevent non-constant-time comparisons. + pub(crate) fn as_slice(&self) -> &[u8] { + self.0.as_slice() + } +} + +impl PartialEq for Mac { + fn eq(&self, other: &Self) -> bool { + self.verify_slice(&other.0) + } +} From 9369f86942620573dad98d3f99e1c58e48045c82 Mon Sep 17 00:00:00 2001 From: Maximiliano Sandoval Date: Tue, 29 Jul 2025 18:08:53 +0200 Subject: [PATCH 2/2] client: crypto: Rename expected parameter So it is clear we are talking about a mac. --- client/src/crypto/native.rs | 4 ++-- client/src/crypto/openssl.rs | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/client/src/crypto/native.rs b/client/src/crypto/native.rs index b3fc8691e..ef1b746e2 100644 --- a/client/src/crypto/native.rs +++ b/client/src/crypto/native.rs @@ -132,11 +132,11 @@ pub(crate) fn compute_mac(data: impl AsRef<[u8]>, key: &Key) -> Result, key: &Key, - expected: impl AsRef<[u8]>, + expected_mac: impl AsRef<[u8]>, ) -> Result { let mut mac = MacAlg::new_from_slice(key.as_ref()).unwrap(); mac.update(data.as_ref()); - Ok(mac.verify_slice(expected.as_ref()).is_ok()) + Ok(mac.verify_slice(expected_mac.as_ref()).is_ok()) } pub(crate) fn verify_checksum_md5(digest: impl AsRef<[u8]>, content: impl AsRef<[u8]>) -> bool { diff --git a/client/src/crypto/openssl.rs b/client/src/crypto/openssl.rs index c0910de71..fcd18e46f 100644 --- a/client/src/crypto/openssl.rs +++ b/client/src/crypto/openssl.rs @@ -153,9 +153,12 @@ pub(crate) fn compute_mac(data: impl AsRef<[u8]>, key: &Key) -> Result, key: &Key, - expected: impl AsRef<[u8]>, + expected_mac: impl AsRef<[u8]>, ) -> Result { - Ok(memcmp::eq(compute_mac(&data, key)?.as_slice(), expected.as_ref())) + Ok(memcmp::eq( + compute_mac(&data, key)?.as_slice(), + expected_mac.as_ref(), + )) } pub(crate) fn verify_checksum_md5(digest: impl AsRef<[u8]>, content: impl AsRef<[u8]>) -> bool {