diff --git a/client/fixtures/legacy-no-schema.keyring b/client/fixtures/legacy-no-schema.keyring new file mode 100644 index 00000000..35ba86f3 Binary files /dev/null and b/client/fixtures/legacy-no-schema.keyring differ diff --git a/client/src/file/api/encrypted_item.rs b/client/src/file/api/encrypted_item.rs index a1ea3f98..955fd337 100644 --- a/client/src/file/api/encrypted_item.rs +++ b/client/src/file/api/encrypted_item.rs @@ -5,7 +5,9 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; use zgvariant::Type; use super::{Error, UnlockedItem}; -use crate::{AsAttributes, CONTENT_TYPE_ATTRIBUTE, Key, Mac, crypto}; +use crate::{ + AsAttributes, CONTENT_TYPE_ATTRIBUTE, GENERIC_SCHEMA, Key, Mac, XDG_SCHEMA_ATTRIBUTE, crypto, +}; #[derive(Deserialize, Serialize, Type, Debug, Clone, Zeroize, ZeroizeOnDrop)] pub(crate) struct EncryptedItem { @@ -29,23 +31,42 @@ impl EncryptedItem { pub fn matches(&self, attributes: &impl AsAttributes, key: Option<&Key>) -> bool { match key { Some(key) => { + let generic_schema_mac = crypto::compute_mac(GENERIC_SCHEMA.as_bytes(), key); let hashed = attributes.hash(key); - hashed - .iter() - .all(|(k, v)| v.as_ref().is_ok_and(|v| self.has_attribute(k.as_str(), v))) + hashed.iter().all(|(k, v)| { + if k == XDG_SCHEMA_ATTRIBUTE + && !self.hashed_attributes.contains_key(XDG_SCHEMA_ATTRIBUTE) + { + return v + .as_ref() + .is_ok_and(|v| generic_schema_mac.as_ref().is_ok_and(|g| v == g)); + } + v.as_ref().is_ok_and(|v| self.has_attribute(k.as_str(), v)) + }) } - None => attributes - .search_attributes() - .iter() - .all(|(k, v)| self.has_plaintext_attribute(k.as_str(), v.as_str())), + None => attributes.search_attributes().iter().all(|(k, v)| { + if k == XDG_SCHEMA_ATTRIBUTE + && !self.hashed_attributes.contains_key(XDG_SCHEMA_ATTRIBUTE) + && v == GENERIC_SCHEMA + { + return true; + } + self.has_plaintext_attribute(k.as_str(), v.as_str()) + }), } } pub fn matches_exact(&self, attributes: &impl AsAttributes, key: Option<&Key>) -> bool { // The secret's content type does not identify the item. let attributes = attributes.as_search_attributes(); - let count = self.hashed_attributes.len() + let mut count = self.hashed_attributes.len() - usize::from(self.hashed_attributes.contains_key(CONTENT_TYPE_ATTRIBUTE)); + // Items without a schema are implicitly generic, see `matches` + if !self.hashed_attributes.contains_key(XDG_SCHEMA_ATTRIBUTE) + && attributes.get(XDG_SCHEMA_ATTRIBUTE).map(String::as_str) == Some(GENERIC_SCHEMA) + { + count += 1; + } count == attributes.len() && self.matches(&attributes, key) } diff --git a/client/src/file/api/legacy_keyring/encrypted.rs b/client/src/file/api/legacy_keyring/encrypted.rs index 1347481c..7d273689 100644 --- a/client/src/file/api/legacy_keyring/encrypted.rs +++ b/client/src/file/api/legacy_keyring/encrypted.rs @@ -8,9 +8,9 @@ use std::{ use endi::{Endian, ReadBytes}; use zeroize::{Zeroize, ZeroizeOnDrop}; -use super::{FILE_HEADER, FILE_HEADER_LEN}; +use super::{FILE_HEADER, FILE_HEADER_LEN, item_type_schema}; use crate::{ - AsAttributes, Secret, crypto, + Secret, XDG_SCHEMA_ATTRIBUTE, crypto, file::{Error, UnlockedItem}, }; @@ -22,7 +22,8 @@ pub struct Keyring { salt: Vec, iteration_count: u32, encrypted_content: Vec, - item_count: usize, + // In the same order as the encrypted items + item_types: Vec, } impl Keyring { @@ -43,7 +44,7 @@ impl Keyring { fn read_attributes<'a>( cursor: &mut Cursor<&'a [u8]>, count: usize, - ) -> Result { + ) -> Result, Error> { let mut result = HashMap::new(); for _ in 0..count { let name = Self::read_string(cursor)?.ok_or_else(|| { @@ -71,8 +72,8 @@ impl Keyring { fn read_items(self, decrypted: &[u8]) -> Result, Error> { let mut cursor = Cursor::new(decrypted); - let mut items = Vec::with_capacity(self.item_count); - for _ in 0..self.item_count { + let mut items = Vec::with_capacity(self.item_types.len()); + for &item_type in &self.item_types { let display_name = Self::read_string(&mut cursor)?.unwrap_or_else(|| { #[cfg(feature = "tracing")] tracing::warn!("Item has no label, defaulting to empty"); @@ -90,7 +91,12 @@ impl Keyring { let _ = cursor.read_u32(Endian::Big)?; } let attribute_count = cursor.read_u32(Endian::Big)? as usize; - let attributes = Self::read_attributes(&mut cursor, attribute_count)?; + let mut attributes = Self::read_attributes(&mut cursor, attribute_count)?; + if !attributes.contains_key(XDG_SCHEMA_ATTRIBUTE) + && let Some(schema) = item_type_schema(item_type) + { + attributes.insert(XDG_SCHEMA_ATTRIBUTE, schema.to_owned()); + } items.push(UnlockedItem::new(display_name, &attributes, secret)); let acl_count = cursor.read_u32(Endian::Big)? as usize; Self::skip_acls(&mut cursor, acl_count)?; @@ -128,10 +134,11 @@ impl Keyring { Ok((hi << 32) | lo) } - fn skip_hashed_items(cursor: &mut Cursor<&[u8]>, count: usize) -> Result<(), Error> { + fn read_hashed_item_types(cursor: &mut Cursor<&[u8]>, count: usize) -> Result, Error> { + let mut item_types = Vec::new(); for _ in 0..count { let _id = cursor.read_u32(Endian::Big)?; - let _type = cursor.read_u32(Endian::Big)?; + item_types.push(cursor.read_u32(Endian::Big)?); let num_attributes = cursor.read_u32(Endian::Big)?; for _ in 0..num_attributes { let _name = Self::read_string(cursor)?; @@ -152,7 +159,7 @@ impl Keyring { } } } - Ok(()) + Ok(item_types) } fn skip_acls(cursor: &mut Cursor<&[u8]>, count: usize) -> Result<(), Error> { @@ -188,7 +195,7 @@ impl Keyring { let _ = cursor.read_u32(Endian::Big)?; } let item_count = cursor.read_u32(Endian::Big)? as usize; - Self::skip_hashed_items(&mut cursor, item_count)?; + let item_types = Self::read_hashed_item_types(&mut cursor, item_count)?; let mut size = cursor.read_u32(Endian::Big)? as usize; let pos = cursor.position() as usize; if size > cursor.get_ref()[pos..].len() { @@ -203,7 +210,7 @@ impl Keyring { salt, iteration_count, encrypted_content, - item_count, + item_types, }) } } @@ -262,4 +269,33 @@ mod tests { Ok(()) } + + #[test] + fn legacy_decrypt_schema_from_item_type() -> Result<(), Error> { + // Written by gnome-keyring 48 for an item stored without a schema + let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("fixtures") + .join("legacy-no-schema.keyring"); + let blob = std::fs::read(path)?; + let keyring = Keyring::try_from(blob.as_slice())?; + let items = keyring.decrypt_items(&Secret::blob("test"))?; + + assert_eq!(items.len(), 1); + assert_eq!(items[0].label(), "Unlock password for: user@example.com"); + assert_eq!(items[0].secret(), Secret::blob("my-ssh-passphrase")); + let attributes = items[0].attributes(); + assert_eq!(attributes.len(), 3); // also content-type + assert_eq!( + attributes.get("unique").map(|v| v.as_ref()), + Some("ssh-store:/fake/id_rsa") + ); + assert_eq!( + attributes + .get(crate::XDG_SCHEMA_ATTRIBUTE) + .map(|v| v.as_ref()), + Some(crate::GENERIC_SCHEMA) + ); + + Ok(()) + } } diff --git a/client/src/file/api/legacy_keyring/mod.rs b/client/src/file/api/legacy_keyring/mod.rs index e746a9f5..e55ff20f 100644 --- a/client/src/file/api/legacy_keyring/mod.rs +++ b/client/src/file/api/legacy_keyring/mod.rs @@ -16,6 +16,21 @@ use crate::{ const FILE_HEADER: &[u8] = b"GnomeKeyring\n\r\0\n"; const FILE_HEADER_LEN: usize = FILE_HEADER.len(); +/// The schema gnome-keyring exposes for items without an `xdg:schema` +/// attribute, derived from their type (`gkm_secret_compat_format_item_type`). +/// Adding it keeps schema based lookups matching the migrated items. +fn item_type_schema(item_type: u32) -> Option<&'static str> { + match item_type & 0xffff { + 0 => Some(crate::GENERIC_SCHEMA), + 1 => Some("org.gnome.keyring.NetworkPassword"), + 2 => Some("org.gnome.keyring.Note"), + 3 => Some("org.gnome.keyring.ChainedKeyring"), + 4 => Some("org.gnome.keyring.EncryptionKey"), + 0x100 => Some("org.gnome.keyring.PkStorage"), + _ => None, + } +} + #[derive(Debug)] pub enum Keyring { Encrypted(encrypted::Keyring), diff --git a/client/src/file/api/legacy_keyring/plain.rs b/client/src/file/api/legacy_keyring/plain.rs index d226df04..4a98d275 100644 --- a/client/src/file/api/legacy_keyring/plain.rs +++ b/client/src/file/api/legacy_keyring/plain.rs @@ -25,9 +25,9 @@ use std::{collections::HashMap, io}; use zeroize::Zeroizing; -use super::FILE_HEADER_LEN; +use super::{FILE_HEADER_LEN, item_type_schema}; use crate::{ - Secret, + Secret, XDG_SCHEMA_ATTRIBUTE, file::{Error, UnlockedItem}, }; @@ -85,6 +85,16 @@ impl Keyring { attributes.insert(name, value); } } + if !attributes.contains_key(XDG_SCHEMA_ATTRIBUTE) { + // Like g_key_file_get_integer, defaulting to a generic secret + let item_type = group + .value("item-type") + .and_then(|v| v.trim_end().parse::().ok()) + .unwrap_or_default(); + if let Some(schema) = item_type_schema(item_type as u32) { + attributes.insert(XDG_SCHEMA_ATTRIBUTE.to_owned(), schema.to_owned()); + } + } UnlockedItem::new(label, &attributes, secret) } @@ -261,9 +271,21 @@ mod tests { assert!(!attributes.contains_key("bad-number")); assert!(!attributes.contains_key("missing-number")); assert!(!attributes.contains_key("bad-escape")); + // Derived from item-type=2 + assert_eq!( + attributes.get(XDG_SCHEMA_ATTRIBUTE).map(|v| v.as_ref()), + Some("org.gnome.keyring.Note") + ); assert_eq!(items[2].label(), ""); assert_eq!(items[2].secret(), Secret::blob([])); + assert_eq!( + items[2] + .attributes() + .get(XDG_SCHEMA_ATTRIBUTE) + .map(|v| v.as_ref()), + Some(crate::GENERIC_SCHEMA) + ); Ok(()) } diff --git a/client/src/file/mod.rs b/client/src/file/mod.rs index 0c521038..0d1a8d16 100644 --- a/client/src/file/mod.rs +++ b/client/src/file/mod.rs @@ -35,7 +35,7 @@ pub use locked_keyring::LockedKeyring; pub use unlocked_item::UnlockedItem; pub use unlocked_keyring::{ItemDefinition, UnlockedKeyring}; -use crate::{AsAttributes, Key, Secret}; +use crate::{AsAttributes, GENERIC_SCHEMA, Key, Secret, XDG_SCHEMA_ATTRIBUTE}; #[derive(Debug)] pub enum Item { @@ -87,6 +87,12 @@ impl Item { Self::Unlocked(unlocked) => { let item_attrs = unlocked.attributes(); attributes.search_attributes().iter().all(|(k, value)| { + if k == XDG_SCHEMA_ATTRIBUTE + && !item_attrs.contains_key(XDG_SCHEMA_ATTRIBUTE) + && value == GENERIC_SCHEMA + { + return true; + } item_attrs.get(k.as_str()).map(|v| v.as_ref()) == Some(value.as_str()) }) } diff --git a/client/src/file/unlocked_item.rs b/client/src/file/unlocked_item.rs index bc6e4b6c..6086ed29 100644 --- a/client/src/file/unlocked_item.rs +++ b/client/src/file/unlocked_item.rs @@ -7,7 +7,10 @@ use super::{ Error, LockedItem, api::{EncryptedItem, GVARIANT_ENCODING}, }; -use crate::{AsAttributes, CONTENT_TYPE_ATTRIBUTE, Key, Mac, Secret, crypto, secret::ContentType}; +use crate::{ + AsAttributes, CONTENT_TYPE_ATTRIBUTE, GENERIC_SCHEMA, Key, Mac, Secret, XDG_SCHEMA_ATTRIBUTE, + crypto, secret::ContentType, +}; /// An item stored in the file backend. #[derive( @@ -66,12 +69,19 @@ impl UnlockedItem { pub fn matches_exact(&self, attributes: &impl AsAttributes) -> bool { // The secret's content type does not identify the item. let requested = attributes.as_search_attributes(); - let count = self.attributes.len() + let mut count = self.attributes.len() - usize::from(self.attributes.contains_key(CONTENT_TYPE_ATTRIBUTE)); + // Items without a schema are implicitly generic + let implicit_schema = !self.attributes.contains_key(XDG_SCHEMA_ATTRIBUTE) + && requested.get(XDG_SCHEMA_ATTRIBUTE).map(String::as_str) == Some(GENERIC_SCHEMA); + if implicit_schema { + count += 1; + } count == requested.len() - && requested - .iter() - .all(|(key, value)| self.attributes.get(key) == Some(value)) + && requested.iter().all(|(key, value)| { + (implicit_schema && key == XDG_SCHEMA_ATTRIBUTE) + || self.attributes.get(key) == Some(value) + }) } /// Retrieve the item attributes as a typed schema. diff --git a/client/src/lib.rs b/client/src/lib.rs index 27643720..b5f8ad91 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -55,6 +55,9 @@ pub use zbus; /// to map a Rust struct of simple types to an item attributes with type check. pub const XDG_SCHEMA_ATTRIBUTE: &str = "xdg:schema"; +/// The default schema for items without a specific schema. +pub const GENERIC_SCHEMA: &str = "org.freedesktop.Secret.Generic"; + /// A content type attribute. /// /// Defines the type of the secret stored in the item. diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index dee3a706..4796a3d1 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -1409,3 +1409,43 @@ async fn set_attributes() -> Result<(), Error> { Ok(()) } + +#[tokio::test] +async fn replace_item_without_schema() -> Result<(), Error> { + let keyring = UnlockedKeyring::temporary(Secret::blob("test_password")).await?; + let unique = ("unique", "ssh-store:/home/user/.ssh/id_rsa"); + keyring + .create_item("Migrated", &[unique], "old", false) + .await?; + + // Another schema doesn't replace it + keyring + .create_item( + "Other", + &[unique, (XDG_SCHEMA_ATTRIBUTE, "org.example.Other")], + "other", + true, + ) + .await?; + assert_eq!(keyring.n_items().await, 2); + + // Items without a schema are implicitly generic + keyring + .create_item( + "New", + &[unique, (XDG_SCHEMA_ATTRIBUTE, oo7::GENERIC_SCHEMA)], + "new", + true, + ) + .await?; + let labels = keyring + .items() + .await? + .iter() + .map(|item| item.label().to_owned()) + .collect::>(); + assert_eq!(labels.len(), 2); + assert!(labels.contains(&"New".to_owned()) && labels.contains(&"Other".to_owned())); + + Ok(()) +} diff --git a/server/src/service/tests.rs b/server/src/service/tests.rs index 1827caee..f895c40f 100644 --- a/server/src/service/tests.rs +++ b/server/src/service/tests.rs @@ -1429,6 +1429,39 @@ async fn discover_plain_v0_keyrings() -> Result<(), Box> Ok(()) } +// https://github.com/linux-credentials/oo7/issues/586 +#[tokio::test] +async fn migrate_v0_items_without_schema() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let service = Service::new(temp_dir.path().to_path_buf(), None, true); + + let keyrings_dir = temp_dir.path().join("keyrings"); + tokio::fs::create_dir_all(keyrings_dir.join("v1")).await?; + + // Its item only has a `unique` attribute and the generic secret type + let fixture_path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .join("client/fixtures/legacy-no-schema.keyring"); + tokio::fs::copy(&fixture_path, keyrings_dir.join("login.keyring")).await?; + + let secret = Secret::from("test"); + service.discover_keyrings(Some(secret.clone())).await?; + + let keyring = UnlockedKeyring::open_at(temp_dir.path(), "login", Some(secret)).await?; + let items = keyring.items().await?; + assert_eq!(items.len(), 1); + let attributes = items[0].attributes(); + assert_eq!( + attributes + .get(oo7::XDG_SCHEMA_ATTRIBUTE) + .map(|v| v.as_ref()), + Some(oo7::GENERIC_SCHEMA) + ); + + Ok(()) +} + #[cfg(feature = "kwallet_migration")] #[cfg(target_endian = "little")] #[tokio::test] @@ -1845,3 +1878,70 @@ async fn concurrent_unlock_creates_single_prompt_impl( Ok(()) } + +// Reproducer for https://github.com/linux-credentials/oo7/issues/586 +#[tokio::test] +async fn search_items_missing_xdg_schema() -> Result<(), Box> { + let setup = TestServiceSetup::plain_session(true).await?; + + // Item without xdg:schema, as found in old gnome-keyring migrations. + setup + .create_item( + "Unlock password for: user@example.com", + &[("unique", "ssh-store:/home/user/.ssh/id_rsa")], + "my-ssh-passphrase", + false, + ) + .await?; + + // gcr searches with xdg:schema via libsecret's SECRET_SCHEMA_NONE. + let (unlocked, locked) = setup + .service_api + .search_items(&[ + ("unique", "ssh-store:/home/user/.ssh/id_rsa"), + (oo7::XDG_SCHEMA_ATTRIBUTE, oo7::GENERIC_SCHEMA), + ]) + .await?; + + assert_eq!(unlocked.len(), 1); + assert!(locked.is_empty()); + + Ok(()) +} + +// gcr stores the passphrase again with its schema, which must replace the +// item migrated without one instead of duplicating it. +#[tokio::test] +async fn replace_item_missing_xdg_schema() -> Result<(), Box> { + let setup = TestServiceSetup::plain_session(true).await?; + let unique = ("unique", "ssh-store:/home/user/.ssh/id_rsa"); + + setup + .create_item("Unlock password", &[unique], "old-passphrase", false) + .await?; + setup + .create_item( + "Unlock password", + &[unique, (oo7::XDG_SCHEMA_ATTRIBUTE, oo7::GENERIC_SCHEMA)], + "new-passphrase", + true, + ) + .await?; + + let items = setup.collections[0].items().await?; + assert_eq!(items.len(), 1); + assert_eq!( + items[0].attributes().await?.get(oo7::XDG_SCHEMA_ATTRIBUTE), + Some(&oo7::GENERIC_SCHEMA.to_owned()) + ); + + let server_collection = setup + .server + .collection_from_path(setup.collections[0].inner().path()) + .await + .unwrap(); + let keyring = server_collection.keyring.read().await; + assert_eq!(keyring.as_ref().unwrap().items().await?.len(), 1); + + Ok(()) +}