From 0716e60b4da6273478b0c27c024f5fdcbd560c35 Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Mon, 30 Mar 2026 12:17:08 +0200 Subject: [PATCH 1/6] pam: Pass socket path as a param Avoids using unsafe and touching the user env --- pam/src/socket.rs | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/pam/src/socket.rs b/pam/src/socket.rs index 7468f8dd1..7a0149c3d 100644 --- a/pam/src/socket.rs +++ b/pam/src/socket.rs @@ -54,7 +54,7 @@ pub fn send_secret_to_daemon( tracing::debug!("Already running as target user (UID={uid}, GID={gid})",); let runtime = tokio::runtime::Runtime::new().map_err(SocketError::Connect)?; return runtime - .block_on(async { send_secret_to_daemon_async(message, uid, auto_start).await }); + .block_on(async { send_secret_to_daemon_async(message, uid, auto_start, None).await }); } // Need to fork and switch credentials @@ -92,8 +92,9 @@ pub fn send_secret_to_daemon( } }; - let result = runtime - .block_on(async { send_secret_to_daemon_async(message, uid, auto_start).await }); + let result = runtime.block_on(async { + send_secret_to_daemon_async(message, uid, auto_start, None).await + }); match result { Ok(_) => unsafe { libc::_exit(0) }, @@ -189,10 +190,11 @@ async fn send_secret_to_daemon_async( message: PamMessage, uid: u32, auto_start: bool, + socket_path: Option, ) -> Result<(), SocketError> { - let socket_path = std::env::var("OO7_PAM_SOCKET") - .map(PathBuf::from) - .unwrap_or_else(|_| PathBuf::from(format!("/run/user/{uid}/oo7-pam.sock"))); + let socket_path = socket_path + .or_else(|| std::env::var("OO7_PAM_SOCKET").ok().map(PathBuf::from)) + .unwrap_or_else(|| PathBuf::from(format!("/run/user/{uid}/oo7-pam.sock"))); tracing::debug!("Connecting to daemon socket at: {}", socket_path.display()); @@ -284,13 +286,9 @@ mod tests { let temp_dir = tempfile::tempdir()?; let socket_path = temp_dir.path().join("test.sock"); - // Set the environment variable to use our test socket - unsafe { - std::env::set_var("OO7_PAM_SOCKET", socket_path.to_str().unwrap()); - } - + let socket_path_clone = socket_path.clone(); let server = tokio::spawn(async move { - let listener = UnixListener::bind(&socket_path).unwrap(); + let listener = UnixListener::bind(&socket_path_clone).unwrap(); let (mut stream, _) = listener.accept().await.unwrap(); let mut length_bytes = [0u8; 4]; @@ -309,15 +307,11 @@ mod tests { let message = PamMessage::unlock("testuser".to_string(), b"testpassword".to_vec()); - let result = send_secret_to_daemon_async(message, 1000, false).await; + let result = send_secret_to_daemon_async(message, 1000, false, Some(socket_path)).await; assert!(result.is_ok()); server.await?; - unsafe { - std::env::remove_var("OO7_PAM_SOCKET"); - } - Ok(()) } } From 471ae2cfa746b63676c3efd4f18a35bb29a1fdcf Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Mon, 30 Mar 2026 12:21:27 +0200 Subject: [PATCH 2/6] server/pam: Avoid unsafe usage by using rustix where possible We already depend on it. --- server/src/pam_listener/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/pam_listener/mod.rs b/server/src/pam_listener/mod.rs index ef959a5b2..0e5adce3b 100644 --- a/server/src/pam_listener/mod.rs +++ b/server/src/pam_listener/mod.rs @@ -53,7 +53,7 @@ pub struct PamListener { impl PamListener { pub fn new(service: Service) -> Self { - let uid = unsafe { libc::getuid() }; + let uid = rustix::process::getuid().as_raw(); let socket_path = service .pam_socket .clone() @@ -108,7 +108,7 @@ impl PamListener { // 1. Root (UID 0) as PAM modules run as root during authentication // 2. Same UID as us let peer_cred = stream.peer_cred()?; - let our_uid = unsafe { libc::getuid() }; + let our_uid = rustix::process::getuid().as_raw(); let peer_uid = peer_cred.uid(); if peer_uid != 0 && peer_uid != our_uid { From bf2bfcf721c0b61dfde1fc1e3cf40c0f0b29f47c Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Mon, 30 Mar 2026 12:29:18 +0200 Subject: [PATCH 3/6] server: Deny unsafe usage Except in capability as that requires libc ffi functions for now. --- server/src/capability.rs | 2 ++ server/src/lib.rs | 1 + server/src/main.rs | 1 + 3 files changed, 4 insertions(+) diff --git a/server/src/capability.rs b/server/src/capability.rs index 697be7779..3f7e68ea7 100644 --- a/server/src/capability.rs +++ b/server/src/capability.rs @@ -1,3 +1,5 @@ +#![allow(unsafe_code)] + use rustix::{ mm::{MlockAllFlags, mlockall}, process::{Gid, Uid, getgid, getuid}, diff --git a/server/src/lib.rs b/server/src/lib.rs index 43c857042..768c5cfb9 100644 --- a/server/src/lib.rs +++ b/server/src/lib.rs @@ -1,6 +1,7 @@ // Library interface for oo7_server // Only expose test utilities when the test-util feature is enabled #![allow(unused)] +#![deny(unsafe_code)] pub(crate) mod collection; pub(crate) mod error; diff --git a/server/src/main.rs b/server/src/main.rs index 9d6fec7eb..b373cd115 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -1,3 +1,4 @@ +#![deny(unsafe_code)] mod capability; mod collection; mod error; From 946197961e413b6a821df70473c57c33142feda6 Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Mon, 30 Mar 2026 12:35:41 +0200 Subject: [PATCH 4/6] Deny unsafe code globally And an explicit allow where the use case is legtimate. This is done for all the crates except pam module as it is mostly ffi code so it is not really that useful. --- cli/src/main.rs | 12 +++++++++--- client/src/file/locked_keyring.rs | 7 +++++-- client/src/file/unlocked_keyring.rs | 15 ++++++++------- client/src/lib.rs | 1 + kwallet/cli/src/main.rs | 2 ++ kwallet/parser/src/lib.rs | 1 + python/src/lib.rs | 2 +- 7 files changed, 27 insertions(+), 13 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 63a7e8e1b..2f0b8ee30 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -1,3 +1,5 @@ +#![deny(unsafe_code)] + use std::{ collections::HashMap, fmt, @@ -376,9 +378,13 @@ impl Commands { }; let keyring = match (path, secret) { - (Some(path), Some(secret)) => unsafe { - Keyring::File(oo7::file::UnlockedKeyring::load_unchecked(path, secret).await?) - }, + (Some(path), Some(secret)) => { + #[allow(unsafe_code)] + unsafe { + Keyring::File(oo7::file::UnlockedKeyring::load_unchecked(path, secret).await?) + } + } + (Some(_), None) => { return Err(Error::new("A keyring requires a secret.")); } diff --git a/client/src/file/locked_keyring.rs b/client/src/file/locked_keyring.rs index 6bbab6bea..d2466312b 100644 --- a/client/src/file/locked_keyring.rs +++ b/client/src/file/locked_keyring.rs @@ -77,8 +77,11 @@ impl LockedKeyring { /// /// # Safety /// - /// The method doesn't validate that the secret can decrypt all the items in - /// the keyring. + /// This method skips validation and doesn't verify that the secret can + /// decrypt all items in the keyring. Use only for recovery scenarios where + /// you need to access a partially corrupted keyring. The keyring may + /// contain items that cannot be decrypted with the provided secret. + #[allow(unsafe_code)] pub async unsafe fn unlock_unchecked(self, secret: Secret) -> Result { self.unlock_inner(secret, false).await } diff --git a/client/src/file/unlocked_keyring.rs b/client/src/file/unlocked_keyring.rs index 60640378e..5bfbfdedd 100644 --- a/client/src/file/unlocked_keyring.rs +++ b/client/src/file/unlocked_keyring.rs @@ -49,7 +49,7 @@ impl UnlockedKeyring { Self::load_inner(path, secret, true).await } - /// Load from a keyring file. + /// Load from a keyring file without validating the secret. /// /// # Arguments /// @@ -58,12 +58,12 @@ impl UnlockedKeyring { /// /// # Safety /// - /// The secret is not validated to be the correct one to decrypt the keyring - /// items. Allowing the API user to write new items with a different - /// secret on top of previously added items with a different secret. - /// - /// As it is not a supported behaviour, this API is mostly meant for - /// recovering broken keyrings. + /// This method skips validation and doesn't verify that the secret can + /// decrypt all items in the keyring. Use only for recovery scenarios where + /// you need to access a partially corrupted keyring. The keyring may + /// contain items that cannot be decrypted with the provided secret, and + /// writing new items may use a different secret than existing items. + #[allow(unsafe_code)] #[cfg_attr(feature = "tracing", tracing::instrument(skip(secret), fields(path = ?path.as_ref())))] pub async unsafe fn load_unchecked( path: impl AsRef, @@ -83,6 +83,7 @@ impl UnlockedKeyring { if validate_items { LockedKeyring::load(path).await?.unlock(secret).await } else { + #[allow(unsafe_code)] unsafe { LockedKeyring::load(path) .await? diff --git a/client/src/lib.rs b/client/src/lib.rs index 320d165b7..a252e19fa 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -1,6 +1,7 @@ #![cfg_attr(docsrs, feature(doc_cfg))] #![deny(rustdoc::broken_intra_doc_links)] #![doc = include_str!("../README.md")] +#![deny(unsafe_code)] #[cfg(all(all(feature = "tokio", feature = "async-std"), not(doc)))] compile_error!("You can't enable both async-std & tokio features at once"); #[cfg(all(not(feature = "tokio"), not(feature = "async-std"), not(doc)))] diff --git a/kwallet/cli/src/main.rs b/kwallet/cli/src/main.rs index 2b2b984b2..f982f4b0e 100644 --- a/kwallet/cli/src/main.rs +++ b/kwallet/cli/src/main.rs @@ -1,3 +1,5 @@ +#![forbid(unsafe_code)] + use std::{collections::HashMap, io::Write, path::PathBuf, process::ExitCode}; use clap::Parser; diff --git a/kwallet/parser/src/lib.rs b/kwallet/parser/src/lib.rs index eaeddbfbf..d38e0ce72 100644 --- a/kwallet/parser/src/lib.rs +++ b/kwallet/parser/src/lib.rs @@ -1,3 +1,4 @@ +#![forbid(unsafe_code)] //! Read-only parser for KWallet file format //! //! This crate provides parsing support for KWallet wallet files: diff --git a/python/src/lib.rs b/python/src/lib.rs index 6465d0b43..95f9978b1 100644 --- a/python/src/lib.rs +++ b/python/src/lib.rs @@ -1,4 +1,4 @@ -#![allow(unsafe_op_in_unsafe_fn)] +#![forbid(unsafe_code)] use std::{collections::HashMap, sync::Arc}; From df375409cdcd1c03480539049298f512bfaa60b5 Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Mon, 30 Mar 2026 12:37:15 +0200 Subject: [PATCH 5/6] readme: Mention kwallet crates --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index b46966ab3..8a90a768d 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,8 @@ The repository consists of the following projects: - [pam](./pam/): PAM integration for the server implementation - [portal](./portal/): [org.freedesktop.impl.portal.Secret](https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.impl.portal.Secret.html) implementation - [server](./server/): [org.freedesktop.secrets](https://specifications.freedesktop.org/secret-service-spec/latest/) server implementation +- [kwallet/cli](./kwallet/cli): a simple CLI to read KWallet `.kwl` files +- [kwallet/parser](./kwallet//parser): Parse Kwallet `.kwl` files, used for automatic migration on the server side. ## Hacking on oo7 services From 096576e330604e364317d4b40cf7098efe97718e Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Mon, 30 Mar 2026 12:40:14 +0200 Subject: [PATCH 6/6] ci: Use --all-targets for clippy jobs --- .github/workflows/CI.yml | 4 ++-- client/examples/file_tracing.rs | 2 +- server/src/service/mod.rs | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index f6eed1166..256366631 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -106,11 +106,11 @@ jobs: with: components: clippy - name: Clippy workspace (default features) - run: cargo clippy --workspace --tests -- -D warnings + run: cargo clippy --workspace --all-targets -- -D warnings - name: Clippy client (tracing / async-std / native crypto) run: cargo clippy -p oo7 --no-default-features --features tracing,async-std,native_crypto,schema -- -D warnings - name: Clippy client (tracing / tokio / OpenSSL) - run: cargo clippy -p oo7 --no-default-features --features tracing,tokio,openssl_crypto,schema --tests -- -D warnings + run: cargo clippy -p oo7 --no-default-features --features tracing,tokio,openssl_crypto,schema --all-targets -- -D warnings meson: name: Meson diff --git a/client/examples/file_tracing.rs b/client/examples/file_tracing.rs index 6cf252002..b8581517d 100644 --- a/client/examples/file_tracing.rs +++ b/client/examples/file_tracing.rs @@ -294,7 +294,7 @@ async fn test_search_performance() -> oo7::Result<()> { let start = Instant::now(); let items = keyring.items().await?; let all_items_time = start.elapsed(); - let valid_items = items.iter().count(); + let valid_items = items.len(); info!( "Get all items: {:?} (found {} valid items)", all_items_time, valid_items diff --git a/server/src/service/mod.rs b/server/src/service/mod.rs index 08de7aa21..30c9eee10 100644 --- a/server/src/service/mod.rs +++ b/server/src/service/mod.rs @@ -580,6 +580,7 @@ impl Service { } #[cfg(any(test, feature = "test-util"))] + #[allow(dead_code)] pub async fn run_with_connection( connection: zbus::Connection, data_dir: std::path::PathBuf,