From 63cd93bddd01bf714e98553966d4da12eac0ee5b Mon Sep 17 00:00:00 2001 From: Jeremy Soller Date: Fri, 13 Mar 2026 08:35:18 -0600 Subject: [PATCH] Security hardening (#426) - Switch gid and supplementary groups to user's when reading user's config - Only show users between UID_MIN and UID_MAX in /etc/login.defs - Open accountsservice icons with O_NOFOLLOW to explicitly disallow symlinks --- Cargo.lock | 7 ++++ cosmic-greeter-config/src/user.rs | 6 --- daemon/Cargo.toml | 1 + daemon/src/lib.rs | 65 +++++++++++++++++++++++++++---- daemon/src/main.rs | 57 ++++++++++----------------- src/greeter.rs | 20 ++-------- 6 files changed, 91 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 688b511..5be4e39 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1366,6 +1366,7 @@ dependencies = [ "tracing", "tracing-journald", "tracing-subscriber", + "whitespace-conf", "xdg 3.0.0", "zbus 5.11.0", ] @@ -7368,6 +7369,12 @@ dependencies = [ "web-sys", ] +[[package]] +name = "whitespace-conf" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "88bce969a15062afb2d34155402d78cd50d1ddeca000a42353ef0c71ddd68995" + [[package]] name = "widestring" version = "1.2.0" diff --git a/cosmic-greeter-config/src/user.rs b/cosmic-greeter-config/src/user.rs index e98d643..b31ad7d 100644 --- a/cosmic-greeter-config/src/user.rs +++ b/cosmic-greeter-config/src/user.rs @@ -7,13 +7,7 @@ use std::num::NonZeroU32; /// Per user state for Greeter. #[derive(Debug, Clone, PartialEq, Deserialize, Serialize)] pub struct UserState { - #[serde(skip_serializing_if = "invalid_uid")] pub uid: NonZeroU32, #[serde(skip_serializing_if = "Option::is_none")] pub last_session: Option, } - -// Only serialize users not system accounts -const fn invalid_uid(uid: &NonZeroU32) -> bool { - uid.get() < 1000 -} diff --git a/daemon/Cargo.toml b/daemon/Cargo.toml index 874a447..4bf3fd5 100644 --- a/daemon/Cargo.toml +++ b/daemon/Cargo.toml @@ -23,6 +23,7 @@ serde.workspace = true zbus.workspace = true cosmic-randr-shell.workspace = true kdl.workspace = true +whitespace-conf = "1" #TODO: reduce features tokio = { workspace = true, features = ["full"] } diff --git a/daemon/src/lib.rs b/daemon/src/lib.rs index 309405d..71f5ee5 100644 --- a/daemon/src/lib.rs +++ b/daemon/src/lib.rs @@ -4,6 +4,8 @@ use kdl::KdlDocument; use std::{ collections::BTreeMap, fs, + io::Read, + os::unix::fs::OpenOptionsExt, path::{Path, PathBuf}, }; @@ -12,6 +14,43 @@ pub use cosmic_bg_config::{Color, Source as BgSource, state::State as BgState}; pub use cosmic_comp_config::{CosmicCompConfig, XkbConfig, ZoomConfig}; pub use cosmic_theme::{Theme, ThemeBuilder}; +pub struct UserFilter { + uid_min: u32, + uid_max: u32, +} + +impl UserFilter { + pub fn new() -> Self { + let login_defs_data = fs::read_to_string("/etc/login.defs").unwrap_or_default(); + let login_defs = whitespace_conf::parse(&login_defs_data); + Self { + uid_min: login_defs + .get("UID_MIN") + .and_then(|x| x.parse::().ok()) + .unwrap_or(1000), + uid_max: login_defs + .get("UID_MAX") + .and_then(|x| x.parse::().ok()) + .unwrap_or(65000), + } + } + + pub fn filter(&self, user: &pwd::Passwd) -> bool { + if user.uid < self.uid_min || user.uid > self.uid_max { + // Skip system accounts + return false; + } + + match Path::new(&user.shell).file_name().and_then(|x| x.to_str()) { + // Skip shell ending in false + Some("false") => false, + // Skip shell ending in nologin + Some("nologin") => false, + _ => true, + } + } +} + #[derive(Clone, Debug, Default, serde::Deserialize, serde::Serialize)] pub struct UserData { pub uid: u32, @@ -74,15 +113,27 @@ impl UserData { // It may not exist if the user uses one of the system icons. In that case, we should read the // information in /var/lib/AccountsService/users, and then read the icon path as the user let icon_path = Path::new("/var/lib/AccountsService/icons").join(&self.name); - if icon_path.is_file() { - match fs::read(&icon_path) { - Ok(icon_data) => { - self.icon_opt = Some(icon_data); - } - Err(err) => { - tracing::error!("failed to read icon {:?}: {:?}", icon_path, err); + match fs::OpenOptions::new() + .read(true) + // Do not follow symlinks + .custom_flags(libc::O_NOFOLLOW) + .open(&icon_path) + { + Ok(mut icon_file) => { + let mut icon_data = Vec::new(); + match icon_file.read_to_end(&mut icon_data) { + Ok(count) => { + icon_data.truncate(count); + self.icon_opt = Some(icon_data); + } + Err(err) => { + tracing::error!("failed to read icon data {:?}: {:?}", icon_path, err); + } } } + Err(err) => { + tracing::error!("failed to open icon {:?}: {:?}", icon_path, err); + } } let mut is_dark = true; diff --git a/daemon/src/main.rs b/daemon/src/main.rs index 1dcd6ff..41bd5ed 100644 --- a/daemon/src/main.rs +++ b/daemon/src/main.rs @@ -1,6 +1,6 @@ use color_eyre::eyre::Context; -use cosmic_greeter_daemon::UserData; -use std::{env, error::Error, future::pending, io, path::Path}; +use cosmic_greeter_daemon::{UserData, UserFilter}; +use std::{env, error::Error, ffi::CString, future::pending, io}; use tracing::metadata::LevelFilter; use tracing::warn; use tracing_subscriber::{EnvFilter, fmt, prelude::*}; @@ -10,25 +10,34 @@ use zbus::{DBusError, connection::Builder}; // callback is executed with the permissions of the specified user id. A good test is to see if // the /etc/shadow file can be read with a non-root user, it should fail with EPERM. fn run_as_user T, T>(user: &pwd::Passwd, f: F) -> Result { + use nix::unistd::{Gid, Uid, getgroups, initgroups, setegid, seteuid, setgroups}; + // Save root HOME let root_home_opt = env::var_os("HOME"); + // Save root groups + let root_groups = getgroups().expect("failed to get root groups"); + // Switch to user HOME unsafe { env::set_var("HOME", &user.dir); } - // Switch to user UID - if unsafe { libc::seteuid(user.uid) } != 0 { - return Err(io::Error::last_os_error()); + // Switch to user identity + { + let name_c = CString::new(&*user.name).expect("invalid username"); + initgroups(&name_c, Gid::from_raw(user.gid)) + .expect("failed to set user supplementary groups"); } + setegid(Gid::from_raw(user.gid)).expect("failed to set user gid"); + seteuid(Uid::from_raw(user.uid)).expect("failed to set user uid"); let t = f(); - // Restore root UID - if unsafe { libc::seteuid(0) } != 0 { - panic!("failed to restore root user id") - } + // Restore root identity + seteuid(Uid::from_raw(0)).expect("failed to restore root uid"); + setegid(Gid::from_raw(0)).expect("failed to restore root gid"); + setgroups(&root_groups).expect("failed to restore root supplementary groups"); // Restore root HOME match root_home_opt { @@ -57,42 +66,18 @@ struct GreeterProxy; #[zbus::interface(name = "com.system76.CosmicGreeter")] impl GreeterProxy { fn get_user_data(&mut self) -> Result { + let user_filter = UserFilter::new(); + // The pwd::Passwd method is unsafe (but not labelled as such) due to using global state (libc pwent functions). // To prevent issues, this should only be called once in the entire process space at a time let users: Vec<_> = /* unsafe */ { pwd::Passwd::iter() - .filter(|user| { - if user.uid < 1000 { - // Skip system accounts - return false; - } - - match Path::new(&user.shell).file_name().and_then(|x| x.to_str()) { - // Skip shell ending in false - Some("false") => false, - // Skip shell ending in nologin - Some("nologin") => false, - _ => true, - } - }) + .filter(|user| user_filter.filter(user)) .collect() }; let mut user_datas = Vec::new(); for user in users { - if user.uid < 1000 { - // Skip system accounts - continue; - } - - match Path::new(&user.shell).file_name().and_then(|x| x.to_str()) { - // Skip shell ending in false - Some("false") => continue, - // Skip shell ending in nologin - Some("nologin") => continue, - _ => (), - } - let mut user_data = UserData::from(user.clone()); //IMPORTANT: Assume the identity of the user to ensure we don't read user file data as root diff --git a/src/greeter.rs b/src/greeter.rs index 82b1a16..6c91688 100644 --- a/src/greeter.rs +++ b/src/greeter.rs @@ -37,7 +37,7 @@ use cosmic::{ surface, }; use cosmic_greeter_config::Config as CosmicGreeterConfig; -use cosmic_greeter_daemon::UserData; +use cosmic_greeter_daemon::{UserData, UserFilter}; use cosmic_randr_shell::{KdlParseWithError, List}; use cosmic_settings_subscriptions::cosmic_a11y_manager::{ AccessibilityEvent, AccessibilityRequest, @@ -51,7 +51,6 @@ use std::{ error::Error, fs, io, num::NonZeroU32, - path::Path, process, sync::Arc, time::{Duration, Instant}, @@ -92,24 +91,13 @@ async fn user_data_dbus() -> Result, Box> { } fn user_data_fallback() -> Vec { + let user_filter = UserFilter::new(); + // The pwd::Passwd method is unsafe (but not labelled as such) due to using global state (libc pwent functions). /* unsafe */ { pwd::Passwd::iter() - .filter(|user| { - if user.uid < 1000 { - // Skip system accounts - return false; - } - - match Path::new(&user.shell).file_name().and_then(|x| x.to_str()) { - // Skip shell ending in false - Some("false") => false, - // Skip shell ending in nologin - Some("nologin") => false, - _ => true, - } - }) + .filter(|user| user_filter.filter(user)) .map(UserData::from) .collect() }