From 47c7ca8152806b5527b614f7727577a2fa671e99 Mon Sep 17 00:00:00 2001 From: Marie Ramlow Date: Thu, 23 Jul 2026 17:22:39 +0200 Subject: [PATCH] Implement app id trust with systemd unit name and dbus name checks --- Cargo.lock | 12 ++ Cargo.toml | 2 + credentialsd/Cargo.toml | 2 + credentialsd/src/gateway/dbus.rs | 183 ++++++++++++++++++++++++------- credentialsd/src/gateway/mod.rs | 101 +++++++++-------- 5 files changed, 209 insertions(+), 91 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index aa6e064..03c7c12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -817,6 +817,7 @@ dependencies = [ "nfc1 0.7.1", "rand 0.10.2", "ring", + "rustix", "serde", "serde_json", "tokio", @@ -824,6 +825,7 @@ dependencies = [ "tracing", "tracing-subscriber", "zbus", + "zbus_systemd", ] [[package]] @@ -4862,6 +4864,16 @@ dependencies = [ "zvariant", ] +[[package]] +name = "zbus_systemd" +version = "0.26100.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4508893b3fc7829cbd654a318942b0716fb08da8a272a646a970f382cfb7d27e" +dependencies = [ + "serde", + "zbus", +] + [[package]] name = "zerocopy" version = "0.8.55" diff --git a/Cargo.toml b/Cargo.toml index bc9a01c..e304a24 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,4 +12,6 @@ serde = { version = "=1.0.229", features = ["derive"] } tracing = "=0.1.44" tracing-subscriber = "=0.3.23" zbus = { version = "=5.18.0", default-features = false } +zbus_systemd = "0.26100.0" zvariant = "=5.13.1" +rustix = "1.1.4" diff --git a/credentialsd/Cargo.toml b/credentialsd/Cargo.toml index 04f54f3..6807ca2 100644 --- a/credentialsd/Cargo.toml +++ b/credentialsd/Cargo.toml @@ -13,6 +13,8 @@ serde.workspace = true tracing.workspace = true tracing-subscriber.workspace = true zbus = { workspace = true, default-features = false, features = ["tokio"] } +zbus_systemd = { workspace = true, features = ["zbus-async-tokio", "systemd1"] } +rustix = { workspace = true } async-stream = "=0.3.6" async-trait = "=0.1.91" diff --git a/credentialsd/src/gateway/dbus.rs b/credentialsd/src/gateway/dbus.rs index ca28a8c..2024451 100644 --- a/credentialsd/src/gateway/dbus.rs +++ b/credentialsd/src/gateway/dbus.rs @@ -1,15 +1,24 @@ -use std::{collections::HashMap, fmt::Display, os::fd::AsRawFd, sync::Arc}; +use std::{ + collections::HashMap, + fmt::Display, + os::fd::{AsFd, AsRawFd, BorrowedFd}, + str::FromStr, + sync::Arc, +}; use serde::{Deserialize, Serialize, ser::SerializeTuple}; use tokio::sync::Mutex as AsyncMutex; use zbus::{ - Connection, DBusError, interface, + Connection, DBusError, + fdo::PropertiesProxy, + interface, message::Header, - names::{BusName, UniqueName}, - zvariant::{DeserializeDict, Optional, Type, Value}, + names::{BusName, InterfaceName, UniqueName}, + zvariant::{self, DeserializeDict, Optional, OwnedFd, OwnedValue, Type, Value}, }; use credentialsd_common::model::WindowHandle; +use zbus_systemd::systemd1::ManagerProxy; use crate::{ DBUS_SERVICE_NAME, @@ -337,11 +346,11 @@ async fn validate_app_details( return Err(Error::SecurityError); }; - let Some(pid) = query_peer_pid_via_fdinfo(connection, unique_name).await else { + let Some(service_info) = query_peer_service_info(connection, unique_name).await else { return Err(Error::SecurityError); }; - if claimed_app_id.is_empty() || !super::should_trust_app_id(pid).await { + if claimed_app_id.is_empty() || !super::should_trust_app_id(&service_info).await { tracing::warn!( ?claimed_app_id, "App ID could not be verified. Rejecting request." @@ -371,19 +380,39 @@ async fn validate_app_details( Ok(RequestContext { app_id, - pid, + pid: service_info.pid, request_kind, }) } -async fn query_peer_pid_via_fdinfo( +pub struct ServiceInfo { + pub names: Vec, + pub executable: String, + pub pid: u32, +} + +async fn query_peer_service_info( connection: &Connection, sender_unique_name: &UniqueName<'_>, -) -> Option { +) -> Option { let dbus_proxy = match zbus::fdo::DBusProxy::new(connection).await { Ok(p) => p, - Err(e) => { - tracing::error!("Failed to establish DBus proxy to query peer info: {e:?}"); + Err(error) => { + tracing::error!( + ?error, + "Failed to establish DBus proxy to query service name info." + ); + return None; + } + }; + + let manager_proxy = match ManagerProxy::new(connection).await { + Ok(p) => p, + Err(error) => { + tracing::error!( + ?error, + "Failed to establish DBus proxy to query service name info." + ); return None; } }; @@ -393,47 +422,123 @@ async fn query_peer_pid_via_fdinfo( .await { Ok(c) => c, - Err(e) => { - tracing::error!("Failed to get peer credentials: {e:?}"); + Err(error) => { + tracing::error!(?error, "Failed to get peer credentials"); return None; } }; - let pidfd = match peer_credentials.process_fd() { - Some(p) => p.as_raw_fd(), - None => { - tracing::error!("Failed to get process fd from peer credentials"); - return None; - } + let Some(pidfd) = peer_credentials + .process_fd() + .and_then(|p| p.as_fd().try_clone_to_owned().ok()) + else { + tracing::error!("Failed to get process fd from peer credentials"); + return None; }; - let fdinfo_str = match std::fs::read_to_string(format!("/proc/self/fdinfo/{pidfd}")) { - Ok(fdinfo) => fdinfo, - Err(e) => { - tracing::error!("Failed to read fdinfo from procfs: {e}"); - return None; - } + let Some(pid) = read_pid_from_procfs(pidfd.as_fd()) else { + tracing::error!("Failed to read PID from fdinfo"); + return None; }; - // Find the line that starts with "Pid:" - let pid_line = match fdinfo_str.lines().find(|line| line.starts_with("Pid:")) { - Some(line) => line, - None => { - tracing::error!("Failed to read PID from fdinfo"); + let unit = match manager_proxy.get_unit_by_pidfd(OwnedFd::from(pidfd)).await { + Ok(unit) => unit, + Err(error) => { + tracing::error!(?error, "Failed to get systemd unit from pidfd."); return None; } }; - let pid_str = pid_line[4..].trim(); + let properties_proxy = + match PropertiesProxy::new(connection, "org.freedesktop.systemd1", unit.0).await { + Ok(p) => p, + Err(error) => { + tracing::error!( + ?error, + "Failed to establish DBus proxy to query unit properties." + ); + return None; + } + }; - // std::process::id() also returns u32 - let pid: u32 = match pid_str.parse() { - Ok(id) => id, - Err(e) => { - tracing::error!("Failed to parse PID from fdinfo entry: {e}"); - return None; - } + let names = read_dbus_property( + &properties_proxy, + InterfaceName::from_static_str("org.freedesktop.systemd1.Unit").unwrap(), + "Names", + ) + .await?; + let executable: Vec = read_dbus_property( + &properties_proxy, + InterfaceName::from_static_str("org.freedesktop.systemd1.Service").unwrap(), + "ExecStart", + ) + .await?; + + let Some((executable, pid)) = executable + .into_iter() + .find(|e| e.pid == pid) + .map(|e| (e.path, e.pid)) + else { + tracing::error!(?names, pid, "Couldn't find process in systemd unit."); + return None; }; - Some(pid) + Some(ServiceInfo { + names, + executable, + pid, + }) +} + +fn read_pid_from_procfs(pidfd: BorrowedFd) -> Option { + std::fs::read_to_string(format!("/proc/self/fdinfo/{}", pidfd.as_raw_fd())) + .inspect_err(|error| tracing::error!(%error, "Failed to read fdinfo from procfs")) + .ok()? + .lines() + .find(|line| line.starts_with("Pid:")) + .map(|line| line[4..].trim()) + .and_then(|line| { + u32::from_str(line) + .inspect_err( + |error| tracing::error!(%error, "Failed to parse PID from fdinfo entry."), + ) + .ok() + }) +} + +#[derive(Debug, Clone, Type, Deserialize, Value, OwnedValue)] +struct ExecCommand { + pub path: String, + pub argv: Vec, + pub ignore_failure: bool, + pub start_timestamp: u64, + pub start_timestamp_monotonic: u64, + pub exit_timestamp: u64, + pub exit_timestamp_monotonic: u64, + pub pid: u32, + pub code: i32, + pub status: i32, +} + +async fn read_dbus_property<'a, T>( + proxy: &PropertiesProxy<'a>, + interface: InterfaceName<'a>, + name: &str, +) -> Option +where + T: TryFrom, +{ + Some(match proxy.get(interface, name).await { + Ok(n) => match n.try_into() { + Ok(n) => n, + Err(error) => { + tracing::error!(?error, "Failed to read DBus response data."); + return None; + } + }, + Err(error) => { + tracing::error!(?error, property = name, "Failed to read DBus property."); + return None; + } + }) } diff --git a/credentialsd/src/gateway/mod.rs b/credentialsd/src/gateway/mod.rs index 594ea2a..0a171d8 100644 --- a/credentialsd/src/gateway/mod.rs +++ b/credentialsd/src/gateway/mod.rs @@ -20,6 +20,7 @@ use zbus::{ use crate::{ dbus::CredentialRequestController, + gateway::dbus::ServiceInfo, model::{ClientDetails, CredentialRequest, CredentialResponse}, webauthn::{AppId, NavigationContext, Origin}, }; @@ -229,63 +230,59 @@ fn validate_request(context: &RequestContext) -> Result bool { - // Verify if we should trust the peer based on the file name. We verify that - // we're in the same mount namespace before using the exe path. - - // TODO: If the portal is running in a separate mount namespace for security - // reasons, then this check will fail with a false negative. - // In the future, we should retrieve this information from another trusted - // source, e.g. check if the PID is in a cgroup managed by systemd and - // corresponds to the org.freedesktop.portal.Desktop D-Bus service unit. - let Ok(my_mnt_ns) = tokio::fs::read_link("/proc/self/ns/mnt").await else { - tracing::debug!("Could not read peer mount namespace"); - return false; - }; - let Ok(peer_mnt_ns) = tokio::fs::read_link(format!("/proc/{pid}/ns/mnt")).await else { - tracing::debug!("Could not determine our mount namespace"); - return false; - }; - tracing::debug!( - "mount namespace:\n ours: {:?}\n theirs: {:?}", - my_mnt_ns, - peer_mnt_ns - ); - if my_mnt_ns != peer_mnt_ns { - tracing::warn!("Peer mount namespace is not the same as ours, not trusting the request."); - return false; +async fn should_trust_app_id(service_info: &ServiceInfo) -> bool { + // Verify if we should trust the peer if it's part of the xdg-desktop-portal.service systemd unit. + // Fallback to CREDS_TRUSTED_CALLERS when debug assertions are enabled, only if running inside the same mount namespace. + if service_info + .names + .iter() + .any(|n| n == "xdg-desktop-portal.service") + { + return true; } + let ServiceInfo { + executable, pid, .. + } = service_info; + cfg!(debug_assertions) && { + let Ok(my_mnt_ns) = tokio::fs::read_link("/proc/self/ns/mnt").await else { + tracing::debug!("Could not read peer mount namespace"); + return false; + }; + let Ok(peer_mnt_ns) = tokio::fs::read_link(format!("/proc/{pid}/ns/mnt")).await else { + tracing::debug!("Could not determine our mount namespace"); + return false; + }; + tracing::debug!( + "mount namespace:\n ours: {:?}\n theirs: {:?}", + my_mnt_ns, + peer_mnt_ns + ); + if my_mnt_ns != peer_mnt_ns { + tracing::warn!( + "Peer mount namespace is not the same as ours, not trusting the request." + ); + return false; + } - let Ok(exe_path) = tokio::fs::read_link(format!("/proc/{pid}/exe")).await else { - tracing::warn!("Cannot read executable name from procfs"); - return false; - }; - - tracing::debug!(?exe_path, %pid, "Found executable path:"); - let trusted_callers: Vec = if cfg!(debug_assertions) { let trusted_callers_env = std::env::var("CREDSD_TRUSTED_CALLERS").unwrap_or_default(); - trusted_callers_env + let trusted_callers: Vec = trusted_callers_env .split(',') .filter_map(|path| Path::new(path).canonicalize().ok()) - .collect() - } else { - vec![ - PathBuf::from("/usr/lib/xdg-desktop-portal"), - PathBuf::from("/usr/libexec/xdg-desktop-portal"), - PathBuf::from("/usr/local/lib/xdg-desktop-portal"), - PathBuf::from("/usr/local/libexec/xdg-desktop-portal"), - ] - }; - tracing::debug!( - ?trusted_callers, - ?exe_path, - "Testing whether request is from trusted caller" - ); - if !trusted_callers.as_slice().contains(&exe_path) { - tracing::warn!(?exe_path, "Request received from untrusted caller"); - false - } else { - true + .collect(); + tracing::debug!( + ?trusted_callers, + ?executable, + "Testing whether request is from trusted caller" + ); + if !trusted_callers + .as_slice() + .contains(&PathBuf::from(executable)) + { + tracing::warn!(?executable, "Request received from untrusted caller"); + false + } else { + true + } } }