diff --git a/CHANGELOG.md b/CHANGELOG.md index 60b6a2d..795f121 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - daemon: Allow Firefox to contact daemon on more distros with different app IDs (Thank you, @michaelbeaumont!) - daemon: Fix all subsequent requests being rejected after the UI failed to launch. - daemon: Don't busy loop polling for NFC devices when NFC is enabled. +- daemon: Use systemd unit name to determine whether a caller is trusted. (Thank you, @NyCodeGHG!) - ui: Add Bulgarian translation (Thank you, @salif!) - webext: Try to read app ID from associated desktop entry (Thank you, @michaelbeaumont!) - ui: Basic recovery from failed attempts to use devices (e.g. flaky bluetooth) diff --git a/credentialsd/src/gateway/dbus.rs b/credentialsd/src/gateway/dbus.rs index ca28a8c..a39f63d 100644 --- a/credentialsd/src/gateway/dbus.rs +++ b/credentialsd/src/gateway/dbus.rs @@ -1,4 +1,9 @@ -use std::{collections::HashMap, fmt::Display, os::fd::AsRawFd, sync::Arc}; +use std::{ + collections::HashMap, + fmt::Display, + os::fd::{AsFd, OwnedFd}, + sync::Arc, +}; use serde::{Deserialize, Serialize, ser::SerializeTuple}; use tokio::sync::Mutex as AsyncMutex; @@ -336,22 +341,32 @@ async fn validate_app_details( let Some(unique_name) = header.sender() else { return Err(Error::SecurityError); }; - - let Some(pid) = query_peer_pid_via_fdinfo(connection, unique_name).await else { + let claimed_app_id = if !claimed_app_id.is_empty() { + claimed_app_id + } else { + tracing::warn!("App ID not specified on request. Rejecting request."); return Err(Error::SecurityError); }; - if claimed_app_id.is_empty() || !super::should_trust_app_id(pid).await { - tracing::warn!( - ?claimed_app_id, - "App ID could not be verified. Rejecting request." - ); - return Err(Error::SecurityError); - } - // Now we can trust these app detail parameters. - let Ok(app_id) = claimed_app_id.parse::() else { - tracing::warn!("Invalid app ID passed: {claimed_app_id}"); - return Err(Error::SecurityError); + let app_id = { + let Some(pidfd) = query_peer_pidfd(connection, unique_name).await else { + return Err(Error::SecurityError); + }; + + if !super::should_trust_app_id(connection, pidfd.as_fd()).await { + tracing::warn!( + ?claimed_app_id, + "App ID could not be verified. Rejecting request." + ); + return Err(Error::SecurityError); + } + + // Now we can trust these app detail parameters. + let Ok(app_id) = claimed_app_id.parse::() else { + tracing::warn!("Invalid app ID passed: {claimed_app_id}"); + return Err(Error::SecurityError); + }; + app_id }; // Verify that the origin is valid for the given app ID. @@ -371,15 +386,17 @@ async fn validate_app_details( Ok(RequestContext { app_id, - pid, + // TODO: we're no longer using the PID in the UI, but we need to update + // the spec before removing it completely. For now, send invalid PID. + pid: 0, request_kind, }) } -async fn query_peer_pid_via_fdinfo( +async fn query_peer_pidfd( connection: &Connection, sender_unique_name: &UniqueName<'_>, -) -> Option { +) -> Option { let dbus_proxy = match zbus::fdo::DBusProxy::new(connection).await { Ok(p) => p, Err(e) => { @@ -399,41 +416,16 @@ async fn query_peer_pid_via_fdinfo( } }; - 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() 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; - } - }; - - // 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"); - return None; - } - }; - - let pid_str = pid_line[4..].trim(); - - // 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; - } - }; - - Some(pid) + pidfd + .as_fd() + .try_clone_to_owned() + .inspect_err( + |err| tracing::error!(%err, "Failed to duplicate peer process file descriptor"), + ) + .ok() } diff --git a/credentialsd/src/gateway/mod.rs b/credentialsd/src/gateway/mod.rs index 003b14b..644591e 100644 --- a/credentialsd/src/gateway/mod.rs +++ b/credentialsd/src/gateway/mod.rs @@ -7,6 +7,7 @@ mod util; use std::{ collections::HashMap, fmt::Display, + os::fd::{AsRawFd, BorrowedFd}, path::{Path, PathBuf}, sync::Arc, }; @@ -229,20 +230,24 @@ fn validate_request(context: &RequestContext) -> Result bool { +async fn should_trust_app_id(connection: &Connection, pidfd: BorrowedFd<'_>) -> bool { + // Verify by systemd unit name + if let Ok(true) = check_by_systemd_unit_name(connection, pidfd).await { + return true; + } + // 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. - + let Some(pid) = read_pid_from_pidfd(pidfd).await else { + return false; + }; // 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 { + let Ok(peer_mnt_ns) = tokio::fs::read_link(format!("/proc/{}/ns/mnt", pid)).await else { tracing::debug!("Could not determine our mount namespace"); return false; }; @@ -256,12 +261,12 @@ async fn should_trust_app_id(pid: u32) -> bool { return false; } - let Ok(exe_path) = tokio::fs::read_link(format!("/proc/{pid}/exe")).await else { + let Ok(exe_path) = tokio::fs::read_link(format!("/proc/{}/exe", pid)).await else { tracing::warn!("Cannot read executable name from procfs"); return false; }; - tracing::debug!(?exe_path, %pid, "Found executable path:"); + tracing::debug!(?exe_path, pidfd = %pidfd.as_raw_fd(), "Found executable path:"); let mut trusted_callers: Vec = vec![ PathBuf::from("/usr/lib/xdg-desktop-portal"), @@ -289,6 +294,50 @@ async fn should_trust_app_id(pid: u32) -> bool { } } +async fn check_by_systemd_unit_name( + connection: &Connection, + pidfd: BorrowedFd<'_>, +) -> zbus::Result { + let systemd_manager = crate::systemd::Manager::create(connection).await?; + + systemd_manager + .match_unit_name_by_pidfd(pidfd, "xdg-desktop-portal.service") + .await +} + +async fn read_pid_from_pidfd<'a>(pidfd: BorrowedFd<'a>) -> Option { + let pidfd_raw = pidfd.as_raw_fd(); + let fdinfo_str = match std::fs::read_to_string(format!("/proc/self/fdinfo/{pidfd_raw}")) { + Ok(fdinfo) => fdinfo, + Err(e) => { + tracing::error!("Failed to read fdinfo from procfs: {e}"); + 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"); + return None; + } + }; + + let pid_str = pid_line[4..].trim(); + + // 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; + } + }; + + Some(pid) +} + fn check_origin_from_app( app_id: &AppId, origin: Origin, diff --git a/credentialsd/src/main.rs b/credentialsd/src/main.rs index d64aab0..e7241c8 100644 --- a/credentialsd/src/main.rs +++ b/credentialsd/src/main.rs @@ -2,6 +2,7 @@ mod credential_service; mod dbus; mod gateway; mod model; +pub mod systemd; mod webauthn; use std::error::Error; diff --git a/credentialsd/src/systemd.rs b/credentialsd/src/systemd.rs new file mode 100644 index 0000000..8a3ec25 --- /dev/null +++ b/credentialsd/src/systemd.rs @@ -0,0 +1,61 @@ +use std::os::fd::BorrowedFd; + +use zbus::{ + Connection, proxy, + zvariant::{Fd, OwnedObjectPath}, +}; + +pub struct Manager<'a> { + proxy: Systemd1ManagerProxy<'a>, +} + +impl<'a> Manager<'a> { + pub async fn create(connection: &'a Connection) -> zbus::Result { + let proxy = Systemd1ManagerProxy::new(connection).await?; + Ok(Self { proxy }) + } + + /// Returns `true` if the process referred to by `pidfd` is part of the + /// systemd unit identified by `name`. + pub async fn match_unit_name_by_pidfd( + &self, + pidfd: BorrowedFd<'_>, + name: &str, + ) -> zbus::Result { + let (path, _, _) = self + .proxy + .get_unit_by_pidfd(pidfd.into()) + .await + .inspect_err(|err| tracing::error!(%err, "Failed to lookup unit by pidfd"))?; + let conn = self.proxy.inner().connection(); + let systemd_unit = Systemd1UnitProxy::new(conn, path).await?; + let unit_names = systemd_unit + .names() + .await + .inspect_err(|err| tracing::error!(%err, "Failed to look up systemd unit names"))?; + let is_match = unit_names.iter().any(|unit_name| unit_name == name); + Ok(is_match) + } +} + +#[proxy( + interface = "org.freedesktop.systemd1.Manager", + default_service = "org.freedesktop.systemd1", + default_path = "/org/freedesktop/systemd1" +)] +trait Systemd1Manager { + #[zbus(name = "GetUnitByPIDFD")] + fn get_unit_by_pidfd(&self, pidfd: Fd<'_>) -> zbus::Result<(OwnedObjectPath, String, Vec)>; +} + +#[proxy( + interface = "org.freedesktop.systemd1.Unit", + default_service = "org.freedesktop.systemd1" +)] +trait Systemd1Unit { + #[zbus(property)] + fn id(&self) -> zbus::Result; + + #[zbus(property)] + fn names(&self) -> zbus::Result>; +}