From 6e3971d392660578bc6b948870591c5543a90083 Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Sat, 26 Sep 2026 17:07:49 +0200 Subject: [PATCH] client: Stop checking secret length As that can lead to failures when the user has been using a short secret in gnome-keyring or other keyring formats. Enforcing a stronger password doesn't help anywhere at all. --- client/src/crypto/mod.rs | 2 +- client/src/crypto/native.rs | 4 +-- client/src/crypto/openssl.rs | 4 +-- .../src/file/api/legacy_keyring/encrypted.rs | 7 +--- client/src/file/api/mod.rs | 32 +++++-------------- client/src/file/error.rs | 5 --- client/src/file/unlocked_keyring.rs | 5 +-- client/tests/file_unlocked_keyring.rs | 23 +++++++------ 8 files changed, 28 insertions(+), 54 deletions(-) diff --git a/client/src/crypto/mod.rs b/client/src/crypto/mod.rs index c0410357f..a28f9f800 100644 --- a/client/src/crypto/mod.rs +++ b/client/src/crypto/mod.rs @@ -56,7 +56,7 @@ mod test { let salt = &[0x92, 0xf4, 0xc0, 0x34, 0x0f, 0x5f, 0x36, 0xf9]; let iteration_count = 1782; let password = b"test"; - let (key, iv) = legacy_derive_key_and_iv(password, Ok(()), salt, iteration_count).unwrap(); + let (key, iv) = legacy_derive_key_and_iv(password, salt, iteration_count).unwrap(); assert_eq!(key.as_ref(), &expected_key[..]); assert_eq!(iv, &expected_iv[..]); } diff --git a/client/src/crypto/native.rs b/client/src/crypto/native.rs index 872d74484..89dde3ccf 100644 --- a/client/src/crypto/native.rs +++ b/client/src/crypto/native.rs @@ -173,7 +173,6 @@ pub(crate) fn derive_key( pub(crate) fn legacy_derive_key_and_iv( secret: impl AsRef<[u8]>, - key_strength: Result<(), file::WeakKeyError>, salt: impl AsRef<[u8]>, iteration_count: usize, ) -> Result<(Key, Vec), super::Error> { @@ -215,7 +214,8 @@ pub(crate) fn legacy_derive_key_and_iv( } let iv = buffer.split_off(EncAlg::key_size()); - Ok((Key::new_with_strength(buffer, key_strength), iv)) + // Only used to decrypt the legacy keyring, never to encrypt + Ok((Key::new_with_strength(buffer, Ok(())), iv)) } /// from https://github.com/plietar/librespot/blob/master/core/src/util/mod.rs#L53 diff --git a/client/src/crypto/openssl.rs b/client/src/crypto/openssl.rs index d2d350e06..97a51d0a2 100644 --- a/client/src/crypto/openssl.rs +++ b/client/src/crypto/openssl.rs @@ -195,7 +195,6 @@ pub(crate) fn derive_key( pub(crate) fn legacy_derive_key_and_iv( secret: impl AsRef<[u8]>, - key_strength: Result<(), file::WeakKeyError>, salt: impl AsRef<[u8]>, iteration_count: usize, ) -> Result<(Key, Vec), super::Error> { @@ -234,5 +233,6 @@ pub(crate) fn legacy_derive_key_and_iv( } let iv = buffer.split_off(cipher.key_len()); - Ok((Key::new_with_strength(buffer, key_strength), iv)) + // Only used to decrypt the legacy keyring, never to encrypt + Ok((Key::new_with_strength(buffer, Ok(())), iv)) } diff --git a/client/src/file/api/legacy_keyring/encrypted.rs b/client/src/file/api/legacy_keyring/encrypted.rs index dac2950cf..1347481cd 100644 --- a/client/src/file/api/legacy_keyring/encrypted.rs +++ b/client/src/file/api/legacy_keyring/encrypted.rs @@ -11,7 +11,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; use super::{FILE_HEADER, FILE_HEADER_LEN}; use crate::{ AsAttributes, Secret, crypto, - file::{Error, UnlockedItem, WeakKeyError}, + file::{Error, UnlockedItem}, }; pub const MAJOR_VERSION: u8 = 0; @@ -29,7 +29,6 @@ impl Keyring { pub fn decrypt_items(self, secret: &Secret) -> Result, Error> { let (key, iv) = crypto::legacy_derive_key_and_iv( &**secret, - self.key_strength(secret), &self.salt, self.iteration_count.try_into().unwrap(), )?; @@ -99,10 +98,6 @@ impl Keyring { Ok(items) } - fn key_strength(&self, _secret: &[u8]) -> Result<(), WeakKeyError> { - Ok(()) - } - fn read_byte_array<'a>(cursor: &mut Cursor<&'a [u8]>) -> Result, Error> { let len = cursor.read_u32(Endian::Big)? as usize; if len == 0xffffffff { diff --git a/client/src/file/api/mod.rs b/client/src/file/api/mod.rs index 5a21019b5..2bc493283 100644 --- a/client/src/file/api/mod.rs +++ b/client/src/file/api/mod.rs @@ -30,8 +30,6 @@ const DEFAULT_SALT_SIZE: usize = 32; const MIN_ITERATION_COUNT: u32 = 100000; const MIN_SALT_SIZE: usize = 32; -// FIXME: choose a reasonable value -const MIN_PASSWORD_LENGTH: usize = 4; const FILE_HEADER: &[u8] = b"GnomeKeyring\n\r\0\n"; const FILE_HEADER_LEN: usize = FILE_HEADER.len(); @@ -101,13 +99,13 @@ impl Keyring { }) } - pub fn key_strength(&self, secret: &[u8]) -> Result<(), WeakKeyError> { + /// Check the key derivation parameters stored in the file. + /// The password itself is not checked, its policy is up to the caller. + pub fn key_strength(&self) -> Result<(), WeakKeyError> { if self.iteration_count < MIN_ITERATION_COUNT { Err(WeakKeyError::IterationCountTooLow(self.iteration_count)) } else if self.salt.len() < MIN_SALT_SIZE { Err(WeakKeyError::SaltTooShort(self.salt.len())) - } else if secret.len() < MIN_PASSWORD_LENGTH { - Err(WeakKeyError::PasswordTooShort(secret.len())) } else { Ok(()) } @@ -300,16 +298,7 @@ impl Keyring { pub fn derive_key(&self, secret: &Secret) -> Result { crypto::derive_key( &**secret, - self.key_strength(secret), - &self.salt, - self.iteration_count.try_into().unwrap(), - ) - } - - pub(crate) fn derive_key_unchecked(&self, secret: &Secret) -> Result { - crypto::derive_key( - &**secret, - Ok(()), + self.key_strength(), &self.salt, self.iteration_count.try_into().unwrap(), ) @@ -481,24 +470,19 @@ mod tests { async fn key_strength() -> Result<(), Error> { let mut keyring = Keyring::new()?; keyring.iteration_count = 50000; // Less than MIN_ITERATION_COUNT (100000) - let secret = Secret::from("test-password-that-is-long-enough"); - let result = keyring.key_strength(&secret); + let result = keyring.key_strength(); assert!(matches!( result, Err(WeakKeyError::IterationCountTooLow(50000)) )); - let keyring = Keyring::new()?; - let secret = Secret::from("ab"); - let result = keyring.key_strength(&secret); - assert!(matches!(result, Err(WeakKeyError::PasswordTooShort(2)))); - let mut keyring = Keyring::new()?; keyring.salt = vec![1, 2, 3, 4]; // Less than MIN_SALT_SIZE (32) - let secret = Secret::from("test-password-that-is-long-enough"); - let result = keyring.key_strength(&secret); + let result = keyring.key_strength(); assert!(matches!(result, Err(WeakKeyError::SaltTooShort(4)))); + assert!(Keyring::new()?.key_strength().is_ok()); + Ok(()) } } diff --git a/client/src/file/error.rs b/client/src/file/error.rs index 0d5b24d1d..e31562d36 100644 --- a/client/src/file/error.rs +++ b/client/src/file/error.rs @@ -184,8 +184,6 @@ pub enum WeakKeyError { IterationCountTooLow(u32), /// Avoid attack on existing files SaltTooShort(usize), - /// Just not secure enough to store password - PasswordTooShort(usize), /// Should not occur /// /// Used by [`dbus`](crate::dbus) module that does not currently @@ -200,9 +198,6 @@ impl std::fmt::Display for WeakKeyError { match self { Self::IterationCountTooLow(count) => write!(f, "Iteration count too low: {count}"), Self::SaltTooShort(length) => write!(f, "Salt too short: {length}"), - Self::PasswordTooShort(length) => { - write!(f, "Password too short: {length}") - } Self::StrengthUnknown => write!(f, "Strength unknown"), } } diff --git a/client/src/file/unlocked_keyring.rs b/client/src/file/unlocked_keyring.rs index e435bd8fc..95d992fdc 100644 --- a/client/src/file/unlocked_keyring.rs +++ b/client/src/file/unlocked_keyring.rs @@ -184,10 +184,7 @@ impl UnlockedKeyring { let legacy_keyring = api::LegacyKeyring::try_from(content)?; let mut keyring = api::Keyring::new()?; - let key = secret - .as_ref() - .map(|s| keyring.derive_key_unchecked(s)) - .transpose()?; + let key = secret.as_ref().map(|s| keyring.derive_key(s)).transpose()?; let decrypted_items = legacy_keyring .decrypt_items(&secret.clone().unwrap_or_else(|| Secret::from(vec![])))?; diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index f247f7986..ac936c7d6 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -320,22 +320,25 @@ async fn delete() -> Result<(), Error> { } #[tokio::test] -async fn write_with_weak_key() -> Result<(), Error> { +async fn write_with_short_password() -> Result<(), Error> { let temp_dir = tempdir()?; - let path = temp_dir.path().join("write_with_weak_key.keyring"); + let path = temp_dir.path().join("write_with_short_password.keyring"); - let secret = Secret::from(vec![1, 2]); - let keyring = UnlockedKeyring::load(&path, Some(secret)).await?; + // Password policy is up to the caller, e.g. keyrings migrated from + // gnome-keyring can have any password. + let secret = Secret::from(vec![1]); + let keyring = UnlockedKeyring::load(&path, Some(secret.clone())).await?; let attributes: HashMap<&str, &str> = HashMap::default(); - let result = keyring + keyring .create_item("label", &attributes, "my-password", false) - .await; + .await?; + keyring.write().await?; - assert!(matches!( - result, - Err(Error::WeakKey(WeakKeyError::PasswordTooShort(2))) - )); + let keyring = UnlockedKeyring::load(&path, Some(secret)).await?; + let items = keyring.items().await?; + assert_eq!(items.len(), 1); + assert_eq!(items[0].secret(), Secret::text("my-password")); Ok(()) }