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

@@ -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"

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<String>,
 }
-
-// Only serialize users not system accounts
-const fn invalid_uid(uid: &NonZeroU32) -> bool {
-    uid.get() < 1000
-}

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"] }

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::<u32>().ok())
+                .unwrap_or(1000),
+            uid_max: login_defs
+                .get("UID_MAX")
+                .and_then(|x| x.parse::<u32>().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;

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<F: FnOnce() -> T, T>(user: &pwd::Passwd, f: F) -> Result<T, io::Error> {
+    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<String, GreeterError> {
+        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

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<Vec<UserData>, Box<dyn Error>> {
 }
 
 fn user_data_fallback() -> Vec<UserData> {
+    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()
     }