From 9ceeae1096ea29aa08f5d85d7690d1386213bba2 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Sun, 13 Oct 2024 20:50:16 +0200 Subject: [PATCH 01/22] Implement newtypes for UserId, GroupId, ProcessId and DeviceId keeping the value private as per https://www.howtocodeit.com/articles/ultimate-guide-rust-newtypes#newtype-essentials Formatting --- src/common/context.rs | 2 +- src/exec/interface.rs | 2 +- src/exec/mod.rs | 6 +- src/exec/no_pty.rs | 4 +- src/exec/use_pty/backchannel.rs | 5 +- src/exec/use_pty/monitor.rs | 6 +- src/exec/use_pty/parent.rs | 2 +- src/su/context.rs | 4 +- src/sudo/env/tests.rs | 13 ++-- src/sudo/mod.rs | 2 +- src/sudo/pipeline/list.rs | 2 +- src/sudoers/mod.rs | 8 +- src/sudoers/test/mod.rs | 12 +-- src/system/file/chown.rs | 2 +- src/system/interface.rs | 129 ++++++++++++++++++++++++++------ src/system/mod.rs | 82 +++++++++++--------- src/system/signal/info.rs | 2 +- src/system/term/mod.rs | 14 ++-- src/system/timestamp.rs | 75 ++++++++++--------- src/system/wait.rs | 10 +-- 20 files changed, 242 insertions(+), 140 deletions(-) diff --git a/src/common/context.rs b/src/common/context.rs index c94120634..a790f38f1 100644 --- a/src/common/context.rs +++ b/src/common/context.rs @@ -116,6 +116,6 @@ mod tests { assert_eq!(context.command.command.to_str().unwrap(), "/usr/bin/echo"); assert_eq!(context.command.arguments, ["hello"]); assert_eq!(context.hostname, Hostname::resolve()); - assert_eq!(context.target_user.uid, 0); + assert_eq!(context.target_user.uid.get(), 0); } } diff --git a/src/exec/interface.rs b/src/exec/interface.rs index d04c83423..b4ac72e93 100644 --- a/src/exec/interface.rs +++ b/src/exec/interface.rs @@ -58,7 +58,7 @@ impl RunOptions for Context { } fn pid(&self) -> i32 { - self.process.pid + self.process.pid.get() } fn use_pty(&self) -> bool { diff --git a/src/exec/mod.rs b/src/exec/mod.rs index 7d1bbc093..1881772c9 100644 --- a/src/exec/mod.rs +++ b/src/exec/mod.rs @@ -108,14 +108,14 @@ fn run_command_internal(options: &impl RunOptions, env: Environment) -> io::Resu if options.use_pty() { match UserTerm::open() { - Ok(user_tty) => exec_pty(options.pid(), command, user_tty), + Ok(user_tty) => exec_pty(ProcessId::new(options.pid()), command, user_tty), Err(err) => { dev_info!("Could not open user's terminal, not allocating a pty: {err}"); - exec_no_pty(options.pid(), command) + exec_no_pty(ProcessId::new(options.pid()), command) } } } else { - exec_no_pty(options.pid(), command) + exec_no_pty(ProcessId::new(options.pid()), command) } } diff --git a/src/exec/no_pty.rs b/src/exec/no_pty.rs index 93175b7cc..e9184f1b3 100644 --- a/src/exec/no_pty.rs +++ b/src/exec/no_pty.rs @@ -148,7 +148,7 @@ impl ExecClosure { /// - is the same PID of the command, or /// - is in the process group of the command and either sudo or the command is the leader. fn is_self_terminating(&self, signaler_pid: ProcessId) -> bool { - if signaler_pid != 0 { + if signaler_pid.get() != 0 { if Some(signaler_pid) == self.command_pid { return true; } @@ -213,7 +213,7 @@ impl ExecClosure { dev_warn!( "cannot send {} to {} (sudo): {err}", signal_fmt(signal), - self.sudo_pid + self.sudo_pid.get() ); } diff --git a/src/exec/use_pty/backchannel.rs b/src/exec/use_pty/backchannel.rs index fda2c3905..c5a6701a8 100644 --- a/src/exec/use_pty/backchannel.rs +++ b/src/exec/use_pty/backchannel.rs @@ -73,7 +73,7 @@ impl ParentMessage { Self::CMD_STAT_EXIT => Self::CommandStatus(CommandStatus::Exit(data)), Self::CMD_STAT_TERM => Self::CommandStatus(CommandStatus::Term(data)), Self::CMD_STAT_STOP => Self::CommandStatus(CommandStatus::Stop(data)), - Self::CMD_PID => Self::CommandPid(data), + Self::CMD_PID => Self::CommandPid(ProcessId::new(data)), Self::SHORT_READ => Self::ShortRead, _ => unreachable!(), } @@ -90,7 +90,8 @@ impl ParentMessage { }; let data = match self { - ParentMessage::IoError(data) | ParentMessage::CommandPid(data) => *data, + ParentMessage::IoError(data) => *data, + ParentMessage::CommandPid(data) => data.get(), ParentMessage::CommandStatus(status) => match status { CommandStatus::Exit(data) | CommandStatus::Term(data) diff --git a/src/exec/use_pty/monitor.rs b/src/exec/use_pty/monitor.rs index 9af942ae6..c880af16d 100644 --- a/src/exec/use_pty/monitor.rs +++ b/src/exec/use_pty/monitor.rs @@ -199,9 +199,9 @@ fn exec_command( original_set: Option, ) -> io::Error { // FIXME (ogsudo): Do any additional configuration that needs to be run after `fork` but before `exec` - let command_pid = std::process::id() as ProcessId; + let command_pid = ProcessId::new(std::process::id() as i32); - setpgid(0, command_pid).ok(); + setpgid(ProcessId::new(0), command_pid).ok(); // Wait for the monitor to set us as the foreground group for the pty if we are in the // foreground. @@ -414,7 +414,7 @@ fn is_self_terminating( command_pid: ProcessId, command_pgrp: ProcessId, ) -> bool { - if signaler_pid != 0 { + if signaler_pid.get() != 0 { if signaler_pid == command_pid { return true; } diff --git a/src/exec/use_pty/parent.rs b/src/exec/use_pty/parent.rs index 37e530df6..fe6b8ad60 100644 --- a/src/exec/use_pty/parent.rs +++ b/src/exec/use_pty/parent.rs @@ -433,7 +433,7 @@ impl ParentClosure { /// - is the same PID of the command, or /// - is in the process group of the command and either sudo or the command is the leader. fn is_self_terminating(&self, signaler_pid: ProcessId) -> bool { - if signaler_pid != 0 { + if signaler_pid.get() != 0 { if Some(signaler_pid) == self.command_pid { return true; } diff --git a/src/su/context.rs b/src/su/context.rs index f2083fc6a..0791b8829 100644 --- a/src/su/context.rs +++ b/src/su/context.rs @@ -79,7 +79,7 @@ impl SuContext { .ok_or_else(|| Error::UserNotFound(options.user.clone().into()))?; // check the current user is root - let is_current_root = User::real_uid() == 0; + let is_current_root = User::real_uid().get() == 0; let is_target_root = options.user == "root"; // only root can set a (additional) group @@ -238,7 +238,7 @@ impl RunOptions for SuContext { } fn pid(&self) -> i32 { - self.process.pid + self.process.pid.get() } fn use_pty(&self) -> bool { diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs index 15fc0f68d..7b98917d2 100644 --- a/src/sudo/env/tests.rs +++ b/src/sudo/env/tests.rs @@ -4,6 +4,7 @@ use crate::sudo::{ cli::{SudoAction, SudoRunOptions}, env::environment::get_target_environment, }; +use crate::system::interface::{GroupId, UserId}; use crate::system::{Group, Hostname, Process, User}; use std::collections::{HashMap, HashSet}; @@ -82,8 +83,8 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context { CommandAndArguments::build_from_args(None, sudo_options.positional_args.clone(), &path); let current_user = CurrentUser::fake(User { - uid: 1000, - gid: 1000, + uid: UserId::new(1000), + gid: GroupId::new(1000), name: "test".into(), gecos: String::new(), @@ -94,13 +95,13 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context { }); let current_group = Group { - gid: 1000, + gid: GroupId::new(1000), name: "test".to_string(), }; let root_user = User { - uid: 0, - gid: 0, + uid: UserId::new(1000), + gid: GroupId::new(1000), name: "root".into(), gecos: String::new(), home: "/root".into(), @@ -110,7 +111,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context { }; let root_group = Group { - gid: 0, + gid: GroupId::new(1000), name: "root".to_string(), }; diff --git a/src/sudo/mod.rs b/src/sudo/mod.rs index bf6bc405c..22077e2d5 100644 --- a/src/sudo/mod.rs +++ b/src/sudo/mod.rs @@ -146,7 +146,7 @@ fn self_check() -> Result<(), Error> { const ROOT: u32 = 0; let euid = User::effective_uid(); - if euid == ROOT { + if euid.get() == ROOT { Ok(()) } else { Err(Error::SelfCheck) diff --git a/src/sudo/pipeline/list.rs b/src/sudo/pipeline/list.rs index da1f1d9a4..7f6c7dd70 100644 --- a/src/sudo/pipeline/list.rs +++ b/src/sudo/pipeline/list.rs @@ -87,7 +87,7 @@ impl Pipeline> { } Authorization::Forbidden => { - if context.current_user.uid == 0 { + if context.current_user.uid.get() == 0 { if original_command.is_some() { return Err(Error::Silent); } diff --git a/src/sudoers/mod.rs b/src/sudoers/mod.rs index 6f37d2379..6296d1153 100644 --- a/src/sudoers/mod.rs +++ b/src/sudoers/mod.rs @@ -15,7 +15,7 @@ use std::{io, mem}; use crate::common::resolve::resolve_path; use crate::log::auth_warn; -use crate::system::interface::{UnixGroup, UnixUser}; +use crate::system::interface::{GroupId, UnixGroup, UnixUser, UserId}; use crate::system::{self, can_execute}; use ast::*; use tokens::*; @@ -436,7 +436,7 @@ fn match_user(user: &impl UnixUser) -> impl Fn(&UserSpecifier) -> bool + '_ { move |spec| match spec { UserSpecifier::User(id) => match_identifier(user, id), UserSpecifier::Group(Identifier::Name(name)) => user.in_group_by_name(name.as_cstr()), - UserSpecifier::Group(Identifier::ID(num)) => user.in_group_by_gid(*num), + UserSpecifier::Group(Identifier::ID(num)) => user.in_group_by_gid(GroupId::new(*num)), _ => todo!(), // nonunix-groups, netgroups, etc. } } @@ -447,7 +447,7 @@ fn in_group(user: &impl UnixUser, group: &impl UnixGroup) -> bool { fn match_group(group: &impl UnixGroup) -> impl Fn(&Identifier) -> bool + '_ { move |id| match id { - Identifier::ID(num) => group.as_gid() == *num, + Identifier::ID(num) => group.as_gid().get() == *num, Identifier::Name(name) => group.try_as_name().map_or(false, |s| name == s), } } @@ -514,7 +514,7 @@ where fn match_identifier(user: &impl UnixUser, ident: &ast::Identifier) -> bool { match ident { Identifier::Name(name) => user.has_name(name), - Identifier::ID(num) => user.has_uid(*num), + Identifier::ID(num) => user.has_uid(UserId::new(*num)), } } diff --git a/src/sudoers/test/mod.rs b/src/sudoers/test/mod.rs index daf85ba10..84fd50890 100644 --- a/src/sudoers/test/mod.rs +++ b/src/sudoers/test/mod.rs @@ -20,16 +20,16 @@ impl UnixUser for Named { self.0 == name } - fn has_uid(&self, uid: u32) -> bool { - dummy_cksum(self.0) == uid + fn has_uid(&self, uid: UserId) -> bool { + dummy_cksum(self.0) == uid.get() } fn in_group_by_name(&self, name: &CStr) -> bool { self.has_name(name.to_str().unwrap()) } - fn in_group_by_gid(&self, gid: u32) -> bool { - dummy_cksum(self.0) == gid + fn in_group_by_gid(&self, gid: GroupId) -> bool { + dummy_cksum(self.0) == gid.get() } fn is_root(&self) -> bool { @@ -39,7 +39,7 @@ impl UnixUser for Named { impl UnixGroup for Named { fn as_gid(&self) -> crate::system::interface::GroupId { - dummy_cksum(self.0) + GroupId::new(dummy_cksum(self.0)) } fn try_as_name(&self) -> Option<&str> { Some(self.0) @@ -175,7 +175,7 @@ fn permission_test() { pass!(["user ALL=(ALL:ALL) /bin/foo"], "user" => request! { user, user }, "server"; "/bin/foo" => [authenticate: Authenticate::Nopasswd]); pass!(["user ALL=(ALL:ALL) /bin/foo"], "user" => request! { user, root }, "server"; "/bin/foo" => [authenticate: Authenticate::None]); - assert_eq!(Named("user").as_gid(), 1466); + assert_eq!(Named("user").as_gid().get(), 1466); pass!(["#1466 server=(ALL:ALL) ALL"], "user" => root(), "server"; "/bin/hello"); pass!(["%#1466 server=(ALL:ALL) ALL"], "user" => root(), "server"; "/bin/hello"); FAIL!(["#1466 server=(ALL:ALL) ALL"], "root" => root(), "server"; "/bin/hello"); diff --git a/src/system/file/chown.rs b/src/system/file/chown.rs index bbd9674a7..bd848e083 100644 --- a/src/system/file/chown.rs +++ b/src/system/file/chown.rs @@ -15,6 +15,6 @@ impl Chown for File { // SAFETY: `fchown` is passed a proper file descriptor; and even if the user/group id // is invalid, it will not cause UB. - cerr(unsafe { libc::fchown(fd, owner, group) }).map(|_| ()) + cerr(unsafe { libc::fchown(fd, owner.get(), group.get()) }).map(|_| ()) } } diff --git a/src/system/interface.rs b/src/system/interface.rs index 9b12603da..8c9a2d9dd 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -1,18 +1,107 @@ -use std::ffi::CStr; +use std::{ffi::CStr, fmt::Display, num::ParseIntError, str::FromStr}; -pub type GroupId = libc::gid_t; -pub type UserId = libc::uid_t; -pub type ProcessId = libc::pid_t; -pub type DeviceId = libc::dev_t; +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct GroupId(libc::gid_t); + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct UserId(libc::uid_t); + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct ProcessId(libc::pid_t); + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct DeviceId(libc::dev_t); + +impl GroupId { + pub fn new(id: libc::gid_t) -> Self { + Self(id) + } + + pub fn get(&self) -> libc::gid_t { + self.0 + } +} + +impl UserId { + pub fn new(id: libc::uid_t) -> Self { + Self(id) + } + + pub fn get(&self) -> libc::uid_t { + self.0 + } + + #[cfg(test)] + pub const fn new_const(uid: libc::uid_t) -> Self { + UserId(uid) + } +} + +impl ProcessId { + pub fn new(id: libc::pid_t) -> Self { + Self(id) + } + + pub fn get(&self) -> libc::pid_t { + self.0 + } +} + +impl DeviceId { + pub fn new(id: libc::dev_t) -> Self { + Self(id) + } + + pub fn get(&self) -> libc::dev_t { + self.0 + } +} + +impl Display for GroupId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl Display for UserId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl Display for ProcessId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl Display for DeviceId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl FromStr for GroupId { + type Err = ParseIntError; + + fn from_str(s: &str) -> Result { + s.parse::().map(GroupId::new) + } +} + +impl FromStr for UserId { + type Err = ParseIntError; + + fn from_str(s: &str) -> Result { + s.parse::().map(UserId::new) + } +} -/// This trait/module is here to not make this crate independent (at the present time) in the idiosyncracies of user representation details -/// (which we may decide over time), as well as to make explicit what functionality a user-representation must have; this -/// interface is not set in stone and "easy" to change. pub trait UnixUser { fn has_name(&self, _name: &str) -> bool { false } - fn has_uid(&self, _uid: libc::uid_t) -> bool { + fn has_uid(&self, _uid: UserId) -> bool { false } fn is_root(&self) -> bool { @@ -35,11 +124,11 @@ impl UnixUser for super::User { fn has_name(&self, name: &str) -> bool { self.name == name } - fn has_uid(&self, uid: GroupId) -> bool { + fn has_uid(&self, uid: UserId) -> bool { self.uid == uid } fn is_root(&self) -> bool { - self.has_uid(0) + self.has_uid(UserId::new(0)) } fn in_group_by_name(&self, name_c: &CStr) -> bool { if let Ok(Some(group)) = super::Group::from_name(name_c) { @@ -57,7 +146,6 @@ impl UnixGroup for super::Group { fn as_gid(&self) -> GroupId { self.gid } - fn try_as_name(&self) -> Option<&str> { Some(&self.name) } @@ -65,11 +153,10 @@ impl UnixGroup for super::Group { #[cfg(test)] mod test { - use crate::system::{Group, User}; - use super::*; + use crate::system::{Group, User}; - fn test_user(user: impl UnixUser, name_c: &CStr, uid: libc::uid_t) { + fn test_user(user: impl UnixUser, name_c: &CStr, uid: UserId) { let name = name_c.to_str().unwrap(); assert!(user.has_name(name)); assert!(user.has_uid(uid)); @@ -77,7 +164,7 @@ mod test { assert_eq!(user.is_root(), name == "root"); } - fn test_group(group: impl UnixGroup, name: &str, gid: libc::gid_t) { + fn test_group(group: impl UnixGroup, name: &str, gid: GroupId) { assert_eq!(group.as_gid(), gid); assert_eq!(group.try_as_name(), Some(name)); } @@ -85,22 +172,22 @@ mod test { #[test] fn test_unix_user() { let user = |name| User::from_name(name).unwrap().unwrap(); - test_user(user(cstr!("root")), cstr!("root"), 0); - test_user(user(cstr!("daemon")), cstr!("daemon"), 1); + test_user(user(cstr!("root")), cstr!("root"), UserId::new(0)); + test_user(user(cstr!("daemon")), cstr!("daemon"), UserId::new(1)); } #[test] fn test_unix_group() { let group = |name| Group::from_name(name).unwrap().unwrap(); - test_group(group(cstr!("root")), "root", 0); - test_group(group(cstr!("daemon")), "daemon", 1); + test_group(group(cstr!("root")), "root", GroupId::new(0)); + test_group(group(cstr!("daemon")), "daemon", GroupId::new(1)); } #[test] fn test_default() { impl UnixUser for () {} assert!(!().has_name("root")); - assert!(!().has_uid(0)); + assert!(!().has_uid(UserId::new(0))); assert!(!().is_root()); assert!(!().in_group_by_name(cstr!("root"))); } diff --git a/src/system/mod.rs b/src/system/mod.rs index 2d5ba6b32..2b07c461f 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -138,13 +138,13 @@ pub(crate) unsafe fn fork() -> io::Result { if pid == 0 { Ok(ForkResult::Child) } else { - Ok(ForkResult::Parent(pid)) + Ok(ForkResult::Parent(ProcessId::new(pid))) } } pub fn setsid() -> io::Result { // SAFETY: this function is memory-safe to call - cerr(unsafe { libc::setsid() }) + Ok(ProcessId::new(cerr(unsafe { libc::setsid() })?)) } #[derive(Clone)] @@ -247,10 +247,10 @@ pub fn set_target_user( cmd.pre_exec(move || { cerr(libc::setgroups( target_user.groups.len(), - target_user.groups.as_ptr(), + &mut (*target_user.groups.as_ptr()).get(), ))?; - cerr(libc::setgid(target_group.gid))?; - cerr(libc::setuid(target_user.uid))?; + cerr(libc::setgid(target_group.gid.get()))?; + cerr(libc::setuid(target_user.uid.get()))?; Ok(()) }); @@ -261,33 +261,33 @@ pub fn set_target_user( pub fn kill(pid: ProcessId, signal: SignalNumber) -> io::Result<()> { // SAFETY: This function cannot cause UB even if `pid` is not a valid process ID or if // `signal` is not a valid signal code. - cerr(unsafe { libc::kill(pid, signal) }).map(|_| ()) + cerr(unsafe { libc::kill(pid.get(), signal) }).map(|_| ()) } /// Send a signal to a process group with the specified ID. pub fn killpg(pgid: ProcessId, signal: SignalNumber) -> io::Result<()> { // SAFETY: This function cannot cause UB even if `pgid` is not a valid process ID or if // `signal` is not a valid signal code. - cerr(unsafe { libc::killpg(pgid, signal) }).map(|_| ()) + cerr(unsafe { libc::killpg(pgid.get(), signal) }).map(|_| ()) } /// Get the process group ID of the current process. pub fn getpgrp() -> ProcessId { // SAFETY: This function is always safe to call - unsafe { libc::getpgrp() } + ProcessId::new(unsafe { libc::getpgrp() }) } /// Get a process group ID. pub fn getpgid(pid: ProcessId) -> io::Result { // SAFETY: This function cannot cause UB even if `pid` is not a valid process ID - cerr(unsafe { libc::getpgid(pid) }) + Ok(ProcessId::new(cerr(unsafe { libc::getpgid(pid.get()) })?)) } /// Set a process group ID. pub fn setpgid(pid: ProcessId, pgid: ProcessId) -> io::Result<()> { // SAFETY: This function cannot cause UB even if `pid` or `pgid` are not a valid process IDs: // https://pubs.opengroup.org/onlinepubs/007904975/functions/setpgid.html - cerr(unsafe { libc::setpgid(pid, pgid) }).map(|_| ()) + cerr(unsafe { libc::setpgid(pid.get(), pgid.get()) }).map(|_| ()) } pub fn chown>( @@ -301,7 +301,7 @@ pub fn chown>( // SAFETY: path is a valid pointer to a null-terminated C string; chown cannot cause safety // issues even if uid and/or gid would be invalid identifiers. - cerr(unsafe { libc::chown(path, uid, gid) }).map(|_| ()) + cerr(unsafe { libc::chown(path, uid.get(), gid.get()) }).map(|_| ()) } #[derive(Debug, Clone, PartialEq)] @@ -322,17 +322,17 @@ impl User { /// (It can cause UB if any of `pwd`'s pointed-to strings does not have a null-terminator.) unsafe fn from_libc(pwd: &libc::passwd) -> Result { let mut buf_len: libc::c_int = 32; - let mut groups_buffer: Vec; + let mut groups_buffer: Vec; while { - groups_buffer = vec![0; buf_len as usize]; + groups_buffer = vec![GroupId::new(0); buf_len as usize]; // SAFETY: getgrouplist is passed valid pointers // in particular `groups_buffer` is an array of `buf.len()` bytes, as required let result = unsafe { libc::getgrouplist( pwd.pw_name, pwd.pw_gid, - groups_buffer.as_mut_ptr(), + &mut (*groups_buffer.as_mut_ptr()).get(), &mut buf_len, ) }; @@ -351,8 +351,8 @@ impl User { }); Ok(User { - uid: pwd.pw_uid, - gid: pwd.pw_gid, + uid: UserId::new(pwd.pw_uid), + gid: GroupId::new(pwd.pw_gid), name: SudoString::new(string_from_ptr(pwd.pw_name))?, gecos: string_from_ptr(pwd.pw_gecos), home: SudoPath::new(os_string_from_ptr(pwd.pw_dir).into())?, @@ -373,7 +373,7 @@ impl User { // but we never dereference `pwd_ptr`. cerr(unsafe { libc::getpwuid_r( - uid, + uid.get(), pwd.as_mut_ptr(), buf.as_mut_ptr(), buf.len(), @@ -393,22 +393,22 @@ impl User { pub fn effective_uid() -> UserId { // SAFETY: this function cannot cause memory safety issues - unsafe { libc::geteuid() } + UserId::new(unsafe { libc::geteuid() }) } pub fn effective_gid() -> GroupId { // SAFETY: this function cannot cause memory safety issues - unsafe { libc::getegid() } + GroupId::new(unsafe { libc::getegid() }) } pub fn real_uid() -> UserId { // SAFETY: this function cannot cause memory safety issues - unsafe { libc::getuid() } + UserId::new(unsafe { libc::getuid() }) } pub fn real_gid() -> GroupId { // SAFETY: this function cannot cause memory safety issues - unsafe { libc::getgid() } + GroupId::new(unsafe { libc::getgid() }) } pub fn real() -> Result, Error> { @@ -457,7 +457,7 @@ impl Group { /// null-terminated list; the pointed-to strings are expected to be null-terminated. unsafe fn from_libc(grp: &libc::group) -> Group { Group { - gid: grp.gr_gid, + gid: GroupId::new(grp.gr_gid), name: string_from_ptr(grp.gr_name), } } @@ -470,7 +470,7 @@ impl Group { // SAFETY: analogous to getpwuid_r above cerr(unsafe { libc::getgrgid_r( - gid, + gid.get(), grp.as_mut_ptr(), buf.as_mut_ptr(), buf.len(), @@ -555,15 +555,15 @@ impl Process { pub fn process_id() -> ProcessId { // NOTE libstd casts the `i32` that `libc::getpid` returns into `u32` // here we cast it back into `i32` (`ProcessId`) - std::process::id() as ProcessId + ProcessId::new(std::process::id() as i32) } /// Return the parent process identifier for the current process pub fn parent_id() -> Option { // NOTE libstd casts the `i32` that `libc::getppid` returns into `u32` // here we cast it back into `i32` (`ProcessId`) - let pid = unix::process::parent_id() as ProcessId; - if pid == 0 { + let pid = ProcessId::new(unix::process::parent_id() as i32); + if pid.get() == 0 { None } else { Some(pid) @@ -574,7 +574,7 @@ impl Process { pub fn session_id() -> ProcessId { // SAFETY: this function is explicitly safe to call with argument 0, // and more generally getsid will never cause memory safety issues. - unsafe { libc::getsid(0) } + ProcessId::new(unsafe { libc::getsid(0) }) } /// Returns the device identifier of the TTY device that is currently @@ -590,7 +590,7 @@ impl Process { // int. We convert via u32 because a direct conversion to DeviceId // would use sign extension, which would result in a different bit // representation - Ok(Some(data as u32 as DeviceId)) + Ok(Some(DeviceId::new(data as u64))) } } @@ -697,6 +697,8 @@ mod tests { use libc::SIGKILL; + use crate::system::interface::{GroupId, ProcessId, UserId}; + use super::{ fork, getpgrp, setpgid, wait::{Wait, WaitOptions}, @@ -723,13 +725,13 @@ mod tests { fn test_get_user_and_group_by_id() { let fixed_users = &[(0, "root"), (1, "daemon")]; for &(id, name) in fixed_users { - let root = User::from_uid(id).unwrap().unwrap(); - assert_eq!(root.uid, id as libc::uid_t); + let root = User::from_uid(UserId::new(id)).unwrap().unwrap(); + assert_eq!(root.uid.get(), id); assert_eq!(root.name, name); } for &(id, name) in fixed_users { - let root = Group::from_gid(id).unwrap().unwrap(); - assert_eq!(root.gid, id as libc::gid_t); + let root = Group::from_gid(GroupId::new(id)).unwrap().unwrap(); + assert_eq!(root.gid.get(), id); assert_eq!(root.name, name); } } @@ -762,7 +764,7 @@ mod tests { }, Group { name: name.to_string(), - gid, + gid: GroupId::new(gid), } ) } @@ -790,8 +792,11 @@ mod tests { use super::{getpgid, setpgid}; let pgrp = getpgrp(); - assert_eq!(getpgid(0).unwrap(), pgrp); - assert_eq!(getpgid(std::process::id() as i32).unwrap(), pgrp); + assert_eq!(getpgid(ProcessId::new(0)).unwrap(), pgrp); + assert_eq!( + getpgid(ProcessId::new(std::process::id() as i32)).unwrap(), + pgrp + ); // FIXME fork will deadlock when this test panics if it forked while // another test was panicking. @@ -802,7 +807,10 @@ mod tests { } ForkResult::Parent(child_pid) => { // The child should be in our process group. - assert_eq!(getpgid(child_pid).unwrap(), getpgid(0).unwrap(),); + assert_eq!( + getpgid(child_pid).unwrap(), + getpgid(ProcessId::new(0)).unwrap(), + ); // Move the child to its own process group setpgid(child_pid, child_pid).unwrap(); // The process group of the child should have changed. @@ -816,7 +824,7 @@ mod tests { .arg("1") .spawn() .unwrap(); - super::kill(child.id() as i32, SIGKILL).unwrap(); + super::kill(ProcessId::new(child.id() as i32), SIGKILL).unwrap(); assert!(!child.wait().unwrap().success()); } #[test] diff --git a/src/system/signal/info.rs b/src/system/signal/info.rs index 31c31dbd2..91f021556 100644 --- a/src/system/signal/info.rs +++ b/src/system/signal/info.rs @@ -21,7 +21,7 @@ impl SignalInfo { /// Gets the PID that sent the signal. pub(crate) fn pid(&self) -> ProcessId { // FIXME: some signals don't set si_pid. - unsafe { self.info.si_pid() } + ProcessId::new(unsafe { self.info.si_pid() }) } /// Gets the signal number. diff --git a/src/system/term/mod.rs b/src/system/term/mod.rs index dee5f4ebb..04f0f46f0 100644 --- a/src/system/term/mod.rs +++ b/src/system/term/mod.rs @@ -151,11 +151,13 @@ pub(crate) trait Terminal: sealed::Sealed { impl Terminal for F { /// Get the foreground process group ID associated with this terminal. fn tcgetpgrp(&self) -> io::Result { - cerr(unsafe { libc::tcgetpgrp(self.as_raw_fd()) }) + Ok(ProcessId::new(cerr(unsafe { + libc::tcgetpgrp(self.as_raw_fd()) + })?)) } /// Set the foreground process group ID associated with this terminalto `pgrp`. fn tcsetpgrp(&self, pgrp: ProcessId) -> io::Result<()> { - cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp) }).map(|_| ()) + cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp.get()) }).map(|_| ()) } /// Make the given terminal the controlling terminal of the calling process. @@ -182,7 +184,9 @@ impl Terminal for F { } fn tcgetsid(&self) -> io::Result { - cerr(unsafe { libc::tcgetsid(self.as_raw_fd()) }) + Ok(ProcessId::new(cerr(unsafe { + libc::tcgetsid(self.as_raw_fd()) + })?)) } } @@ -242,13 +246,13 @@ mod tests { // Open a new pseudoterminal. let leader = Pty::open().unwrap().leader; // The pty leader should not have a foreground process group yet. - assert_eq!(leader.tcgetpgrp().unwrap(), 0); + assert_eq!(leader.tcgetpgrp().unwrap().get(), 0); // Create a new session so we can change the controlling terminal. setsid().unwrap(); // Set the pty leader as the controlling terminal. leader.make_controlling_terminal().unwrap(); // Set us as the foreground process group of the pty leader. - let pgid = getpgid(0).unwrap(); + let pgid = getpgid(ProcessId::new(0)).unwrap(); leader.tcsetpgrp(pgid).unwrap(); // Check that we are in fact the foreground process group of the pty leader. assert_eq!(pgid, leader.tcgetpgrp().unwrap()); diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs index 2e6666baf..a6aea6c87 100644 --- a/src/system/timestamp.rs +++ b/src/system/timestamp.rs @@ -12,7 +12,7 @@ use crate::{ use super::{ audit::secure_open_cookie_file, file::FileLock, - interface::UserId, + interface::{DeviceId, ProcessId, UserId}, time::{Duration, SystemTime}, Process, WithProcess, }; @@ -329,12 +329,12 @@ pub enum CreateResult { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RecordScope { Tty { - tty_device: libc::dev_t, - session_pid: libc::pid_t, + tty_device: DeviceId, + session_pid: ProcessId, init_time: SystemTime, }, Ppid { - group_pid: libc::pid_t, + group_pid: ProcessId, init_time: SystemTime, }, } @@ -348,9 +348,9 @@ impl RecordScope { init_time, } => { target.write_all(&[1u8])?; - let b = tty_device.to_le_bytes(); + let b = tty_device.get().to_le_bytes(); target.write_all(&b)?; - let b = session_pid.to_le_bytes(); + let b = session_pid.get().to_le_bytes(); target.write_all(&b)?; init_time.encode(target)?; } @@ -359,7 +359,7 @@ impl RecordScope { init_time, } => { target.write_all(&[2u8])?; - let b = group_pid.to_le_bytes(); + let b = group_pid.get().to_le_bytes(); target.write_all(&b)?; init_time.encode(target)?; } @@ -381,8 +381,8 @@ impl RecordScope { let session_pid = libc::pid_t::from_le_bytes(buf); let init_time = SystemTime::decode(from)?; Ok(RecordScope::Tty { - tty_device, - session_pid, + tty_device: DeviceId::new(tty_device), + session_pid: ProcessId::new(session_pid), init_time, }) } @@ -392,7 +392,7 @@ impl RecordScope { let group_pid = libc::pid_t::from_le_bytes(buf); let init_time = SystemTime::decode(from)?; Ok(RecordScope::Ppid { - group_pid, + group_pid: ProcessId::new(group_pid), init_time, }) } @@ -449,7 +449,7 @@ pub struct SessionRecord { /// or which TTY for interactive sessions scope: RecordScope, /// The user that needs to be authenticated against - auth_user: libc::uid_t, + auth_user: UserId, /// The timestamp at which the time was created. This must always be a time /// originating from a monotonic clock that continues counting during system /// sleep. @@ -486,7 +486,7 @@ impl SessionRecord { self.scope.encode(target)?; // write user id - let buf = self.auth_user.to_le_bytes(); + let buf = self.auth_user.get().to_le_bytes(); target.write_all(&buf)?; // write timestamp @@ -506,6 +506,7 @@ impl SessionRecord { let mut buf = [0; std::mem::size_of::()]; from.read_exact(&mut buf)?; let auth_user = libc::uid_t::from_le_bytes(buf); + let auth_user = UserId::new(auth_user); // timestamp let timestamp = SystemTime::decode(from)?; @@ -569,17 +570,17 @@ mod tests { use crate::system::tests::tempfile; - const TEST_USER_ID: UserId = 1000; + const TEST_USER_ID: UserId = UserId::new_const(1000); #[test] fn can_encode_and_decode() { let tty_sample = SessionRecord::new( RecordScope::Tty { - tty_device: 10, - session_pid: 42, + tty_device: DeviceId::new(10), + session_pid: ProcessId::new(42), init_time: SystemTime::now().unwrap() - Duration::seconds(150), }, - 999, + UserId::new(999), ) .unwrap(); @@ -596,10 +597,10 @@ mod tests { let ppid_sample = SessionRecord::new( RecordScope::Ppid { - group_pid: 42, + group_pid: ProcessId::new(42), init_time: SystemTime::now().unwrap(), }, - 123, + UserId::new(123), ) .unwrap(); let bytes = ppid_sample.as_bytes().unwrap(); @@ -611,40 +612,40 @@ mod tests { fn timestamp_record_matches_works() { let init_time = SystemTime::now().unwrap(); let scope = RecordScope::Tty { - tty_device: 12, - session_pid: 1234, + tty_device: DeviceId::new(12), + session_pid: ProcessId::new(1234), init_time, }; - let tty_sample = SessionRecord::new(scope, 675).unwrap(); + let tty_sample = SessionRecord::new(scope, UserId::new(675)).unwrap(); - assert!(tty_sample.matches(&scope, 675)); - assert!(!tty_sample.matches(&scope, 789)); + assert!(tty_sample.matches(&scope, UserId::new(675))); + assert!(!tty_sample.matches(&scope, UserId::new(789))); assert!(!tty_sample.matches( &RecordScope::Tty { - tty_device: 20, - session_pid: 1234, + tty_device: DeviceId::new(20), + session_pid: ProcessId::new(1234), init_time }, - 675 + UserId::new(675), )); assert!(!tty_sample.matches( &RecordScope::Ppid { - group_pid: 42, + group_pid: ProcessId::new(42), init_time }, - 675 + UserId::new(675), )); // make sure time is different std::thread::sleep(std::time::Duration::from_millis(1)); assert!(!tty_sample.matches( &RecordScope::Tty { - tty_device: 12, - session_pid: 1234, + tty_device: DeviceId::new(12), + session_pid: ProcessId::new(1234), init_time: SystemTime::now().unwrap() }, - 675 + UserId::new(675), )); } @@ -652,11 +653,11 @@ mod tests { fn timestamp_record_written_between_works() { let some_time = SystemTime::now().unwrap() + Duration::minutes(100); let scope = RecordScope::Tty { - tty_device: 12, - session_pid: 1234, + tty_device: DeviceId::new(12), + session_pid: ProcessId::new(1234), init_time: some_time, }; - let sample = SessionRecord::init(scope, 1234, true, some_time); + let sample = SessionRecord::init(scope, UserId::new(1234), true, some_time); let dur = Duration::seconds(30); @@ -717,11 +718,11 @@ mod tests { let mut srf = SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).unwrap(); let tty_scope = RecordScope::Tty { - tty_device: 0, - session_pid: 0, + tty_device: DeviceId::new(0), + session_pid: ProcessId::new(0), init_time: SystemTime::new(0, 0), }; - let auth_user = 2424; + let auth_user = UserId::new(2424); let res = srf.create(tty_scope, auth_user).unwrap(); let CreateResult::Created { time } = res else { panic!("Expected record to be created"); diff --git a/src/system/wait.rs b/src/system/wait.rs index 4b8407c18..e2f494e23 100644 --- a/src/system/wait.rs +++ b/src/system/wait.rs @@ -28,14 +28,14 @@ impl Wait for ProcessId { let mut status: c_int = 0; // SAFETY: a valid pointer is passed to `waitpid` - let pid = cerr(unsafe { libc::waitpid(self, &mut status, options.flags) }) + let pid = cerr(unsafe { libc::waitpid(self.get(), &mut status, options.flags) }) .map_err(WaitError::Io)?; if pid == 0 && options.flags & WNOHANG != 0 { return Err(WaitError::NotReady); } - Ok((pid, WaitStatus { status })) + Ok((ProcessId::new(pid), WaitStatus { status })) } } @@ -169,7 +169,7 @@ mod tests { .spawn() .unwrap(); - let command_pid = command.id() as ProcessId; + let command_pid = ProcessId::new(command.id() as i32); let (pid, status) = command_pid.wait(WaitOptions::new()).unwrap(); assert_eq!(command_pid, pid); @@ -196,7 +196,7 @@ mod tests { .spawn() .unwrap(); - let command_pid = command.id() as ProcessId; + let command_pid = ProcessId::new(command.id() as i32); kill(command_pid, SIGSTOP).unwrap(); @@ -225,7 +225,7 @@ mod tests { .spawn() .unwrap(); - let command_pid = command.id() as ProcessId; + let command_pid = ProcessId::new(command.id() as i32); let mut count = 0; let (pid, status) = loop { From 4d09d202fe887555aca20ea2b84e6ceedc50200a Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Sun, 13 Oct 2024 21:23:14 +0200 Subject: [PATCH 02/22] Fix breaking changes in test env --- src/sudo/env/tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs index 7b98917d2..8a92ad8f4 100644 --- a/src/sudo/env/tests.rs +++ b/src/sudo/env/tests.rs @@ -100,8 +100,8 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context { }; let root_user = User { - uid: UserId::new(1000), - gid: GroupId::new(1000), + uid: UserId::new(0), + gid: GroupId::new(0), name: "root".into(), gecos: String::new(), home: "/root".into(), @@ -111,7 +111,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context { }; let root_group = Group { - gid: GroupId::new(1000), + gid: GroupId::new(0), name: "root".to_string(), }; From 479ee493aeaac8757d7d2f68c48f057163da6036 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Sun, 13 Oct 2024 21:36:24 +0200 Subject: [PATCH 03/22] Return docs --- src/system/interface.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/system/interface.rs b/src/system/interface.rs index 8c9a2d9dd..8a575ac6f 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -97,6 +97,9 @@ impl FromStr for UserId { } } +/// This trait/module is here to not make this crate independent (at the present time) in the idiosyncracies of user representation details +/// (which we may decide over time), as well as to make explicit what functionality a user-representation must have; this +/// interface is not set in stone and "easy" to change. pub trait UnixUser { fn has_name(&self, _name: &str) -> bool { false @@ -138,6 +141,7 @@ impl UnixUser for super::User { } } fn in_group_by_gid(&self, gid: GroupId) -> bool { + self.groups.contains(&gid) } } From 41b0e08c13bfc547a4e77b1d1ee0d194b21471cf Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Sun, 13 Oct 2024 22:00:06 +0200 Subject: [PATCH 04/22] Formatting --- src/system/interface.rs | 1 - src/system/mod.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/system/interface.rs b/src/system/interface.rs index 8a575ac6f..e1092ee7b 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -141,7 +141,6 @@ impl UnixUser for super::User { } } fn in_group_by_gid(&self, gid: GroupId) -> bool { - self.groups.contains(&gid) } } diff --git a/src/system/mod.rs b/src/system/mod.rs index 2b07c461f..3718b8678 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -247,7 +247,7 @@ pub fn set_target_user( cmd.pre_exec(move || { cerr(libc::setgroups( target_user.groups.len(), - &mut (*target_user.groups.as_ptr()).get(), + &(*target_user.groups.as_ptr()).get(), ))?; cerr(libc::setgid(target_group.gid.get()))?; cerr(libc::setuid(target_user.uid.get()))?; From 99adc61dc5dde1ed458d7d48c1adb5568de79eed Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Sun, 13 Oct 2024 23:02:38 +0200 Subject: [PATCH 05/22] Don't use newtypes for groups_buffer getgrouplist doesn't like it. --- src/system/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/system/mod.rs b/src/system/mod.rs index 3718b8678..e2effe815 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -322,17 +322,17 @@ impl User { /// (It can cause UB if any of `pwd`'s pointed-to strings does not have a null-terminator.) unsafe fn from_libc(pwd: &libc::passwd) -> Result { let mut buf_len: libc::c_int = 32; - let mut groups_buffer: Vec; + let mut groups_buffer: Vec; while { - groups_buffer = vec![GroupId::new(0); buf_len as usize]; + groups_buffer = vec![0; buf_len as usize]; // SAFETY: getgrouplist is passed valid pointers // in particular `groups_buffer` is an array of `buf.len()` bytes, as required let result = unsafe { libc::getgrouplist( pwd.pw_name, pwd.pw_gid, - &mut (*groups_buffer.as_mut_ptr()).get(), + groups_buffer.as_mut_ptr(), &mut buf_len, ) }; @@ -358,7 +358,7 @@ impl User { home: SudoPath::new(os_string_from_ptr(pwd.pw_dir).into())?, shell: os_string_from_ptr(pwd.pw_shell).into(), passwd: string_from_ptr(pwd.pw_passwd), - groups: groups_buffer, + groups: groups_buffer.iter().map(|id| GroupId::new(*id)).collect::>(), }) } From 037f45a12d12a85f670137b2df5af4c898169edc Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Sun, 13 Oct 2024 23:03:40 +0200 Subject: [PATCH 06/22] Format --- src/system/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/system/mod.rs b/src/system/mod.rs index e2effe815..7b167975e 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -358,7 +358,10 @@ impl User { home: SudoPath::new(os_string_from_ptr(pwd.pw_dir).into())?, shell: os_string_from_ptr(pwd.pw_shell).into(), passwd: string_from_ptr(pwd.pw_passwd), - groups: groups_buffer.iter().map(|id| GroupId::new(*id)).collect::>(), + groups: groups_buffer + .iter() + .map(|id| GroupId::new(*id)) + .collect::>(), }) } From 74a329da617b30618e9ea2923fc7b38e2535025c Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Sun, 13 Oct 2024 23:15:20 +0200 Subject: [PATCH 07/22] Don't dereference pointer when type is not equal Again, we're working with newtypes here, not the ids. --- src/system/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/system/mod.rs b/src/system/mod.rs index 7b167975e..6595deb0e 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -247,7 +247,12 @@ pub fn set_target_user( cmd.pre_exec(move || { cerr(libc::setgroups( target_user.groups.len(), - &(*target_user.groups.as_ptr()).get(), + target_user + .groups + .iter() + .map(|g| g.get()) + .collect::>() + .as_ptr(), ))?; cerr(libc::setgid(target_group.gid.get()))?; cerr(libc::setuid(target_user.uid.get()))?; From 845991d162165793e9f7ed25993ac5b0b25f8626 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Mon, 14 Oct 2024 17:54:00 +0200 Subject: [PATCH 08/22] Fix merge Never doing that in GH again... --- src/system/interface.rs | 14 ++++++++------ src/system/mod.rs | 8 ++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/system/interface.rs b/src/system/interface.rs index 38ef22b10..77ddb4514 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -156,12 +156,9 @@ impl UnixGroup for super::Group { #[cfg(test)] mod test { - use std::ffi::CString; - - use crate::system::{Group, User, ROOT_GROUP_NAME}; - use super::*; - use crate::system::{Group, User}; + use crate::system::{Group, User, ROOT_GROUP_NAME}; + use std::ffi::CString; fn test_user(user: impl UnixUser, name_c: &CStr, uid: UserId) { let name = name_c.to_str().unwrap(); @@ -189,8 +186,13 @@ mod test { #[test] fn test_unix_group() { + let group = |name| Group::from_name(name).unwrap().unwrap(); let root_group_cstr = CString::new(ROOT_GROUP_NAME).unwrap(); - test_group(group(root_group_cstr.as_c_str()), ROOT_GROUP_NAME, GroupId::new(0)); + test_group( + group(root_group_cstr.as_c_str()), + ROOT_GROUP_NAME, + GroupId::new(0), + ); test_group(group(cstr!("daemon")), "daemon", GroupId::new(1)); } diff --git a/src/system/mod.rs b/src/system/mod.rs index fdc00d757..d04db746f 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -738,15 +738,15 @@ mod tests { #[test] fn test_get_user_and_group_by_id() { let fixed_users = &[ - (0, "root"), + (UserId::new(0), "root"), ( User::from_name(cstr!("daemon")).unwrap().unwrap().uid, "daemon", ), ]; for &(id, name) in fixed_users { - let root = User::from_uid(UserId::new(id)).unwrap().unwrap(); - assert_eq!(root.uid.get(), id); + let root = User::from_uid(id).unwrap().unwrap(); + assert_eq!(root.uid, id); assert_eq!(root.name, name); } @@ -759,7 +759,7 @@ mod tests { ]; for &(id, name) in fixed_groups { let root = Group::from_gid(id).unwrap().unwrap(); - assert_eq!(root.gid.get(), id); + assert_eq!(root.gid, id); assert_eq!(root.name, name); } } From 673acda92a2e99a81446f912f036c59899054a1e Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Mon, 14 Oct 2024 18:09:52 +0200 Subject: [PATCH 09/22] Replace impl `const fn new` with `OnceCell` --- src/system/interface.rs | 5 ----- src/system/timestamp.rs | 25 ++++++++++++++++++------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/system/interface.rs b/src/system/interface.rs index 77ddb4514..aed7cb78c 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -30,11 +30,6 @@ impl UserId { pub fn get(&self) -> libc::uid_t { self.0 } - - #[cfg(test)] - pub const fn new_const(uid: libc::uid_t) -> Self { - UserId(uid) - } } impl ProcessId { diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs index a6aea6c87..b9c60a761 100644 --- a/src/system/timestamp.rs +++ b/src/system/timestamp.rs @@ -567,10 +567,13 @@ impl SessionRecord { #[cfg(test)] mod tests { use super::*; - use crate::system::tests::tempfile; + use std::cell::OnceCell; - const TEST_USER_ID: UserId = UserId::new_const(1000); + const TEST_USER_ID: OnceCell = OnceCell::new(); + fn test_user_id() -> UserId { + *TEST_USER_ID.get_or_init(|| UserId::new(1000)) + } #[test] fn can_encode_and_decode() { @@ -688,25 +691,33 @@ mod tests { // valid header should remain valid let c = tempfile_with_data(&[0xD0, 0x50, 0x01, 0x00]).unwrap(); let timeout = Duration::seconds(30); - assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); + assert!( + SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() + ); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // invalid headers should be corrected let c = tempfile_with_data(&[0xAB, 0xBA]).unwrap(); - assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); + assert!( + SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() + ); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // empty header should be filled in let c = tempfile_with_data(&[]).unwrap(); - assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); + assert!( + SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() + ); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // invalid version should reset file let c = tempfile_with_data(&[0xD0, 0x50, 0xAB, 0xBA, 0x0, 0x0]).unwrap(); - assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); + assert!( + SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() + ); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); } @@ -716,7 +727,7 @@ mod tests { let timeout = Duration::seconds(30); let c = tempfile_with_data(&[]).unwrap(); let mut srf = - SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).unwrap(); + SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).unwrap(); let tty_scope = RecordScope::Tty { tty_device: DeviceId::new(0), session_pid: ProcessId::new(0), From 82765a026f7148d8cfca141461a7ce21a5eabe27 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Mon, 14 Oct 2024 18:10:05 +0200 Subject: [PATCH 10/22] Formatting --- src/system/timestamp.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs index b9c60a761..0f441557b 100644 --- a/src/system/timestamp.rs +++ b/src/system/timestamp.rs @@ -691,33 +691,25 @@ mod tests { // valid header should remain valid let c = tempfile_with_data(&[0xD0, 0x50, 0x01, 0x00]).unwrap(); let timeout = Duration::seconds(30); - assert!( - SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() - ); + assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // invalid headers should be corrected let c = tempfile_with_data(&[0xAB, 0xBA]).unwrap(); - assert!( - SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() - ); + assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // empty header should be filled in let c = tempfile_with_data(&[]).unwrap(); - assert!( - SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() - ); + assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // invalid version should reset file let c = tempfile_with_data(&[0xD0, 0x50, 0xAB, 0xBA, 0x0, 0x0]).unwrap(); - assert!( - SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok() - ); + assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); } From d1eee331ea33933acff9ba07bdbce65354029518 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Mon, 14 Oct 2024 18:16:17 +0200 Subject: [PATCH 11/22] Use static TEST_USER_ID with OnceLock --- src/system/timestamp.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs index 0f441557b..5367d1fb2 100644 --- a/src/system/timestamp.rs +++ b/src/system/timestamp.rs @@ -568,9 +568,9 @@ impl SessionRecord { mod tests { use super::*; use crate::system::tests::tempfile; - use std::cell::OnceCell; + use std::sync::OnceLock; - const TEST_USER_ID: OnceCell = OnceCell::new(); + static TEST_USER_ID: OnceLock = OnceLock::new(); fn test_user_id() -> UserId { *TEST_USER_ID.get_or_init(|| UserId::new(1000)) } From 7780bd6d650ae740a1814a60e04446a908412113 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 15 Oct 2024 21:07:53 +0200 Subject: [PATCH 12/22] Make pid() return `ProcessId` --- src/exec/interface.rs | 8 +++++--- src/exec/mod.rs | 6 +++--- src/su/context.rs | 9 ++++++--- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/exec/interface.rs b/src/exec/interface.rs index b4ac72e93..ef74fb240 100644 --- a/src/exec/interface.rs +++ b/src/exec/interface.rs @@ -2,6 +2,7 @@ use std::io::{self, ErrorKind}; use std::path::PathBuf; use crate::common::SudoPath; +use crate::system::interface::ProcessId; use crate::{ common::{context::LaunchType, Context}, system::{Group, User}, @@ -16,7 +17,8 @@ pub trait RunOptions { fn user(&self) -> &User; fn requesting_user(&self) -> &User; fn group(&self) -> &Group; - fn pid(&self) -> i32; + fn pid(&self) -> ProcessId; + fn use_pty(&self) -> bool; } @@ -57,8 +59,8 @@ impl RunOptions for Context { &self.target_group } - fn pid(&self) -> i32 { - self.process.pid.get() + fn pid(&self) -> ProcessId { + self.process.pid } fn use_pty(&self) -> bool { diff --git a/src/exec/mod.rs b/src/exec/mod.rs index 1881772c9..7d1bbc093 100644 --- a/src/exec/mod.rs +++ b/src/exec/mod.rs @@ -108,14 +108,14 @@ fn run_command_internal(options: &impl RunOptions, env: Environment) -> io::Resu if options.use_pty() { match UserTerm::open() { - Ok(user_tty) => exec_pty(ProcessId::new(options.pid()), command, user_tty), + Ok(user_tty) => exec_pty(options.pid(), command, user_tty), Err(err) => { dev_info!("Could not open user's terminal, not allocating a pty: {err}"); - exec_no_pty(ProcessId::new(options.pid()), command) + exec_no_pty(options.pid(), command) } } } else { - exec_no_pty(ProcessId::new(options.pid()), command) + exec_no_pty(options.pid(), command) } } diff --git a/src/su/context.rs b/src/su/context.rs index 0791b8829..755b00b20 100644 --- a/src/su/context.rs +++ b/src/su/context.rs @@ -5,11 +5,14 @@ use std::{ path::{Path, PathBuf}, }; -use crate::common::{error::Error, resolve::CurrentUser, Environment}; use crate::common::{resolve::is_valid_executable, SudoPath}; use crate::exec::RunOptions; use crate::log::user_warn; use crate::system::{Group, Process, User}; +use crate::{ + common::{error::Error, resolve::CurrentUser, Environment}, + system::interface::ProcessId, +}; use super::cli::SuRunOptions; @@ -237,8 +240,8 @@ impl RunOptions for SuContext { &self.group } - fn pid(&self) -> i32 { - self.process.pid.get() + fn pid(&self) -> ProcessId { + self.process.pid } fn use_pty(&self) -> bool { From ffa3f467d7a45d00a4a450591b8f37fb9072bfb7 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 15 Oct 2024 21:08:13 +0200 Subject: [PATCH 13/22] Remove redundant get() call --- src/exec/no_pty.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/exec/no_pty.rs b/src/exec/no_pty.rs index e9184f1b3..46bfc107c 100644 --- a/src/exec/no_pty.rs +++ b/src/exec/no_pty.rs @@ -213,7 +213,7 @@ impl ExecClosure { dev_warn!( "cannot send {} to {} (sudo): {err}", signal_fmt(signal), - self.sudo_pid.get() + self.sudo_pid ); } From 10a45ed91ea46f72b862873e8ae90da523c6538c Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 15 Oct 2024 21:08:31 +0200 Subject: [PATCH 14/22] Add UserId::ROOT const --- src/common/context.rs | 7 +++++-- src/sudo/env/tests.rs | 2 +- src/system/interface.rs | 8 +++++--- src/system/mod.rs | 2 +- src/system/timestamp.rs | 16 ++++++---------- 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/common/context.rs b/src/common/context.rs index 1510e734a..0ddd1849e 100644 --- a/src/common/context.rs +++ b/src/common/context.rs @@ -95,7 +95,10 @@ impl Context { #[cfg(test)] mod tests { - use crate::{sudo::SudoAction, system::Hostname}; + use crate::{ + sudo::SudoAction, + system::{interface::UserId, Hostname}, + }; use std::collections::HashMap; use super::Context; @@ -121,6 +124,6 @@ mod tests { } assert_eq!(context.command.arguments, ["hello"]); assert_eq!(context.hostname, Hostname::resolve()); - assert_eq!(context.target_user.uid.get(), 0); + assert_eq!(context.target_user.uid, UserId::ROOT); } } diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs index 8a92ad8f4..ea6e3897b 100644 --- a/src/sudo/env/tests.rs +++ b/src/sudo/env/tests.rs @@ -100,7 +100,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context { }; let root_user = User { - uid: UserId::new(0), + uid: UserId::ROOT, gid: GroupId::new(0), name: "root".into(), gecos: String::new(), diff --git a/src/system/interface.rs b/src/system/interface.rs index aed7cb78c..fa7088819 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -30,6 +30,8 @@ impl UserId { pub fn get(&self) -> libc::uid_t { self.0 } + + pub const ROOT: Self = Self(0); } impl ProcessId { @@ -126,7 +128,7 @@ impl UnixUser for super::User { self.uid == uid } fn is_root(&self) -> bool { - self.has_uid(UserId::new(0)) + self.has_uid(UserId::ROOT) } fn in_group_by_name(&self, name_c: &CStr) -> bool { if let Ok(Some(group)) = super::Group::from_name(name_c) { @@ -175,7 +177,7 @@ mod test { #[test] fn test_unix_user() { let user = |name| User::from_name(name).unwrap().unwrap(); - test_user(user(cstr!("root")), cstr!("root"), UserId::new(0)); + test_user(user(cstr!("root")), cstr!("root"), UserId::ROOT); test_user(user(cstr!("daemon")), cstr!("daemon"), UserId::new(1)); } @@ -196,7 +198,7 @@ mod test { #[test] fn test_default() { assert!(!().has_name("root")); - assert!(!().has_uid(UserId::new(0))); + assert!(!().has_uid(UserId::ROOT)); assert!(!().is_root()); assert!(!().in_group_by_name(cstr!("root"))); } diff --git a/src/system/mod.rs b/src/system/mod.rs index 475b4683d..4b5f1fe8c 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -757,7 +757,7 @@ mod tests { #[test] fn test_get_user_and_group_by_id() { let fixed_users = &[ - (UserId::new(0), "root"), + (UserId::ROOT, "root"), ( User::from_name(cstr!("daemon")).unwrap().unwrap().uid, "daemon", diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs index 5367d1fb2..1085829d8 100644 --- a/src/system/timestamp.rs +++ b/src/system/timestamp.rs @@ -568,12 +568,8 @@ impl SessionRecord { mod tests { use super::*; use crate::system::tests::tempfile; - use std::sync::OnceLock; - static TEST_USER_ID: OnceLock = OnceLock::new(); - fn test_user_id() -> UserId { - *TEST_USER_ID.get_or_init(|| UserId::new(1000)) - } + static TEST_USER_ID: UserId = UserId::ROOT; #[test] fn can_encode_and_decode() { @@ -691,25 +687,25 @@ mod tests { // valid header should remain valid let c = tempfile_with_data(&[0xD0, 0x50, 0x01, 0x00]).unwrap(); let timeout = Duration::seconds(30); - assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); + assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // invalid headers should be corrected let c = tempfile_with_data(&[0xAB, 0xBA]).unwrap(); - assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); + assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // empty header should be filled in let c = tempfile_with_data(&[]).unwrap(); - assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); + assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); // invalid version should reset file let c = tempfile_with_data(&[0xD0, 0x50, 0xAB, 0xBA, 0x0, 0x0]).unwrap(); - assert!(SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).is_ok()); + assert!(SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).is_ok()); let v = data_from_tempfile(c).unwrap(); assert_eq!(&v[..], &[0xD0, 0x50, 0x01, 0x00]); } @@ -719,7 +715,7 @@ mod tests { let timeout = Duration::seconds(30); let c = tempfile_with_data(&[]).unwrap(); let mut srf = - SessionRecordFile::new(test_user_id(), c.try_clone().unwrap(), timeout).unwrap(); + SessionRecordFile::new(TEST_USER_ID, c.try_clone().unwrap(), timeout).unwrap(); let tty_scope = RecordScope::Tty { tty_device: DeviceId::new(0), session_pid: ProcessId::new(0), From e2ca69efc5534b9fedffa91a48af87ccac5ddd65 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 15 Oct 2024 21:30:42 +0200 Subject: [PATCH 15/22] keep id structs the same size as their field --- src/system/interface.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/system/interface.rs b/src/system/interface.rs index fa7088819..6a60d113d 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -1,14 +1,18 @@ use std::{ffi::CStr, fmt::Display, num::ParseIntError, str::FromStr}; +#[repr(transparent)] #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct GroupId(libc::gid_t); +#[repr(transparent)] #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct UserId(libc::uid_t); +#[repr(transparent)] #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ProcessId(libc::pid_t); +#[repr(transparent)] #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct DeviceId(libc::dev_t); From 1a3fd1be08df2cb6dc9cd9d53c0a2ff3866b7371 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Wed, 16 Oct 2024 15:50:00 +0200 Subject: [PATCH 16/22] Use ROOT const on more places --- src/sudo/mod.rs | 5 ++--- src/sudo/pipeline/list.rs | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/sudo/mod.rs b/src/sudo/mod.rs index 22077e2d5..b89b584da 100644 --- a/src/sudo/mod.rs +++ b/src/sudo/mod.rs @@ -3,6 +3,7 @@ use crate::common::resolve::CurrentUser; use crate::common::{Context, Error}; use crate::log::dev_info; +use crate::system::interface::UserId; use crate::system::kernel::kernel_check; use crate::system::timestamp::RecordScope; use crate::system::User; @@ -143,10 +144,8 @@ fn sudo_process() -> Result<(), Error> { } fn self_check() -> Result<(), Error> { - const ROOT: u32 = 0; - let euid = User::effective_uid(); - if euid.get() == ROOT { + if euid == UserId::ROOT { Ok(()) } else { Err(Error::SelfCheck) diff --git a/src/sudo/pipeline/list.rs b/src/sudo/pipeline/list.rs index 7f6c7dd70..76de11d1a 100644 --- a/src/sudo/pipeline/list.rs +++ b/src/sudo/pipeline/list.rs @@ -5,7 +5,7 @@ use crate::{ pam::CLIConverser, sudo::{cli::SudoListOptions, pam::PamAuthenticator, SudoersPolicy}, sudoers::{Authorization, ListRequest, Policy, Request, Sudoers}, - system::User, + system::{interface::UserId, User}, }; use super::{Pipeline, PolicyPlugin}; @@ -87,7 +87,7 @@ impl Pipeline> { } Authorization::Forbidden => { - if context.current_user.uid.get() == 0 { + if context.current_user.uid == UserId::ROOT { if original_command.is_some() { return Err(Error::Silent); } From 458efe70fec62f90d689c54999dbae743ba7f194 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Wed, 16 Oct 2024 16:26:17 +0200 Subject: [PATCH 17/22] Use transparent struct on GroupId, remove transparent on UserId, ProcessId and DeviceId --- src/system/interface.rs | 11 ++++++++--- src/system/mod.rs | 8 ++------ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/system/interface.rs b/src/system/interface.rs index 6a60d113d..19afa3ab4 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -1,18 +1,23 @@ use std::{ffi::CStr, fmt::Display, num::ParseIntError, str::FromStr}; +/// Represents a group ID in the system. +/// +/// `GroupId` is transparent because the memory mapping should stay the same as the underlying +/// type, so we can safely cast as a pointer. +/// See the implementation in `system::mod::set_target_user`. #[repr(transparent)] #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct GroupId(libc::gid_t); -#[repr(transparent)] +/// Represents a user ID in the system. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct UserId(libc::uid_t); -#[repr(transparent)] +/// Represents a process ID in the system. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ProcessId(libc::pid_t); -#[repr(transparent)] +/// Represents a device ID in the system. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct DeviceId(libc::dev_t); diff --git a/src/system/mod.rs b/src/system/mod.rs index 4b5f1fe8c..ea521f5f4 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -266,12 +266,8 @@ pub fn set_target_user( cmd.pre_exec(move || { cerr(libc::setgroups( target_user.groups.len(), - target_user - .groups - .iter() - .map(|g| g.get()) - .collect::>() - .as_ptr(), + // We can cast to gid_t because `GroupId` is marked as transparent + target_user.groups.as_ptr().cast::(), ))?; cerr(libc::setgid(target_group.gid.get()))?; cerr(libc::setuid(target_user.uid.get()))?; From f62cc4ec8bfd5eca9703108d90e252d131b3a7e7 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 22 Oct 2024 11:26:25 +0200 Subject: [PATCH 18/22] Add `is_valid` for ProcessId --- src/exec/no_pty.rs | 2 +- src/exec/use_pty/monitor.rs | 2 +- src/exec/use_pty/parent.rs | 2 +- src/system/interface.rs | 4 ++++ src/system/mod.rs | 2 +- 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/exec/no_pty.rs b/src/exec/no_pty.rs index 46bfc107c..23f0d79b2 100644 --- a/src/exec/no_pty.rs +++ b/src/exec/no_pty.rs @@ -148,7 +148,7 @@ impl ExecClosure { /// - is the same PID of the command, or /// - is in the process group of the command and either sudo or the command is the leader. fn is_self_terminating(&self, signaler_pid: ProcessId) -> bool { - if signaler_pid.get() != 0 { + if signaler_pid.is_valid() { if Some(signaler_pid) == self.command_pid { return true; } diff --git a/src/exec/use_pty/monitor.rs b/src/exec/use_pty/monitor.rs index 4af90eadf..ec0708903 100644 --- a/src/exec/use_pty/monitor.rs +++ b/src/exec/use_pty/monitor.rs @@ -414,7 +414,7 @@ fn is_self_terminating( command_pid: ProcessId, command_pgrp: ProcessId, ) -> bool { - if signaler_pid.get() != 0 { + if signaler_pid.is_valid() { if signaler_pid == command_pid { return true; } diff --git a/src/exec/use_pty/parent.rs b/src/exec/use_pty/parent.rs index fe6b8ad60..d56442fa5 100644 --- a/src/exec/use_pty/parent.rs +++ b/src/exec/use_pty/parent.rs @@ -433,7 +433,7 @@ impl ParentClosure { /// - is the same PID of the command, or /// - is in the process group of the command and either sudo or the command is the leader. fn is_self_terminating(&self, signaler_pid: ProcessId) -> bool { - if signaler_pid.get() != 0 { + if signaler_pid.is_valid() { if Some(signaler_pid) == self.command_pid { return true; } diff --git a/src/system/interface.rs b/src/system/interface.rs index 19afa3ab4..22d7e202e 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -51,6 +51,10 @@ impl ProcessId { pub fn get(&self) -> libc::pid_t { self.0 } + + pub fn is_valid(&self) -> bool { + self.0 != 0 + } } impl DeviceId { diff --git a/src/system/mod.rs b/src/system/mod.rs index 5120dbcba..4e7c9106d 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -586,7 +586,7 @@ impl Process { // NOTE libstd casts the `i32` that `libc::getppid` returns into `u32` // here we cast it back into `i32` (`ProcessId`) let pid = ProcessId::new(unix::process::parent_id() as i32); - if pid.get() == 0 { + if !pid.is_valid() { None } else { Some(pid) From 8eff83f34669f9acf20e01e4092b6e742ed5db42 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 22 Oct 2024 11:26:45 +0200 Subject: [PATCH 19/22] Use `UserId::ROOT` --- src/su/context.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/su/context.rs b/src/su/context.rs index 755b00b20..bdf7d442d 100644 --- a/src/su/context.rs +++ b/src/su/context.rs @@ -5,7 +5,7 @@ use std::{ path::{Path, PathBuf}, }; -use crate::common::{resolve::is_valid_executable, SudoPath}; +use crate::{common::{resolve::is_valid_executable, SudoPath}, system::interface::UserId}; use crate::exec::RunOptions; use crate::log::user_warn; use crate::system::{Group, Process, User}; @@ -82,7 +82,7 @@ impl SuContext { .ok_or_else(|| Error::UserNotFound(options.user.clone().into()))?; // check the current user is root - let is_current_root = User::real_uid().get() == 0; + let is_current_root = User::real_uid() == UserId::ROOT; let is_target_root = options.user == "root"; // only root can set a (additional) group From fd07c7ed54d2cd52ea0c7ac17e4452119082a150 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 22 Oct 2024 11:27:09 +0200 Subject: [PATCH 20/22] Remove more get calls --- src/sudoers/mod.rs | 2 +- src/sudoers/test/mod.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/sudoers/mod.rs b/src/sudoers/mod.rs index ca5e45705..2b82aefa3 100644 --- a/src/sudoers/mod.rs +++ b/src/sudoers/mod.rs @@ -443,7 +443,7 @@ fn in_group(user: &impl UnixUser, group: &impl UnixGroup) -> bool { fn match_group(group: &impl UnixGroup) -> impl Fn(&Identifier) -> bool + '_ { move |id| match id { - Identifier::ID(num) => group.as_gid().get() == *num, + Identifier::ID(num) => group.as_gid() == GroupId::new(*num), Identifier::Name(name) => group.try_as_name().map_or(false, |s| name == s), } } diff --git a/src/sudoers/test/mod.rs b/src/sudoers/test/mod.rs index 84fd50890..ea32d31a3 100644 --- a/src/sudoers/test/mod.rs +++ b/src/sudoers/test/mod.rs @@ -21,7 +21,7 @@ impl UnixUser for Named { } fn has_uid(&self, uid: UserId) -> bool { - dummy_cksum(self.0) == uid.get() + UserId::new(dummy_cksum(self.0)) == uid } fn in_group_by_name(&self, name: &CStr) -> bool { @@ -29,7 +29,7 @@ impl UnixUser for Named { } fn in_group_by_gid(&self, gid: GroupId) -> bool { - dummy_cksum(self.0) == gid.get() + GroupId::new(dummy_cksum(self.0)) == gid } fn is_root(&self) -> bool { @@ -38,7 +38,7 @@ impl UnixUser for Named { } impl UnixGroup for Named { - fn as_gid(&self) -> crate::system::interface::GroupId { + fn as_gid(&self) -> GroupId { GroupId::new(dummy_cksum(self.0)) } fn try_as_name(&self) -> Option<&str> { @@ -175,7 +175,7 @@ fn permission_test() { pass!(["user ALL=(ALL:ALL) /bin/foo"], "user" => request! { user, user }, "server"; "/bin/foo" => [authenticate: Authenticate::Nopasswd]); pass!(["user ALL=(ALL:ALL) /bin/foo"], "user" => request! { user, root }, "server"; "/bin/foo" => [authenticate: Authenticate::None]); - assert_eq!(Named("user").as_gid().get(), 1466); + assert_eq!(Named("user").as_gid(), GroupId::new(1466)); pass!(["#1466 server=(ALL:ALL) ALL"], "user" => root(), "server"; "/bin/hello"); pass!(["%#1466 server=(ALL:ALL) ALL"], "user" => root(), "server"; "/bin/hello"); FAIL!(["#1466 server=(ALL:ALL) ALL"], "root" => root(), "server"; "/bin/hello"); From 86afd0a59fd4d60a299381595d069cfdf6ea1c92 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 22 Oct 2024 11:27:19 +0200 Subject: [PATCH 21/22] Rename get to inner --- src/exec/use_pty/backchannel.rs | 2 +- src/system/file/chown.rs | 2 +- src/system/interface.rs | 8 ++++---- src/system/mod.rs | 18 +++++++++--------- src/system/term/mod.rs | 4 ++-- src/system/timestamp.rs | 8 ++++---- src/system/wait.rs | 2 +- 7 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/exec/use_pty/backchannel.rs b/src/exec/use_pty/backchannel.rs index c5a6701a8..b63bc334b 100644 --- a/src/exec/use_pty/backchannel.rs +++ b/src/exec/use_pty/backchannel.rs @@ -91,7 +91,7 @@ impl ParentMessage { let data = match self { ParentMessage::IoError(data) => *data, - ParentMessage::CommandPid(data) => data.get(), + ParentMessage::CommandPid(data) => data.inner(), ParentMessage::CommandStatus(status) => match status { CommandStatus::Exit(data) | CommandStatus::Term(data) diff --git a/src/system/file/chown.rs b/src/system/file/chown.rs index bd848e083..4a4ff6ae5 100644 --- a/src/system/file/chown.rs +++ b/src/system/file/chown.rs @@ -15,6 +15,6 @@ impl Chown for File { // SAFETY: `fchown` is passed a proper file descriptor; and even if the user/group id // is invalid, it will not cause UB. - cerr(unsafe { libc::fchown(fd, owner.get(), group.get()) }).map(|_| ()) + cerr(unsafe { libc::fchown(fd, owner.inner(), group.inner()) }).map(|_| ()) } } diff --git a/src/system/interface.rs b/src/system/interface.rs index 22d7e202e..fc4a7ff34 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -26,7 +26,7 @@ impl GroupId { Self(id) } - pub fn get(&self) -> libc::gid_t { + pub fn inner(&self) -> libc::gid_t { self.0 } } @@ -36,7 +36,7 @@ impl UserId { Self(id) } - pub fn get(&self) -> libc::uid_t { + pub fn inner(&self) -> libc::uid_t { self.0 } @@ -48,7 +48,7 @@ impl ProcessId { Self(id) } - pub fn get(&self) -> libc::pid_t { + pub fn inner(&self) -> libc::pid_t { self.0 } @@ -62,7 +62,7 @@ impl DeviceId { Self(id) } - pub fn get(&self) -> libc::dev_t { + pub fn inner(&self) -> libc::dev_t { self.0 } } diff --git a/src/system/mod.rs b/src/system/mod.rs index 4e7c9106d..6f28dd92e 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -269,8 +269,8 @@ pub fn set_target_user( // We can cast to gid_t because `GroupId` is marked as transparent target_user.groups.as_ptr().cast::(), ))?; - cerr(libc::setgid(target_group.gid.get()))?; - cerr(libc::setuid(target_user.uid.get()))?; + cerr(libc::setgid(target_group.gid.inner()))?; + cerr(libc::setuid(target_user.uid.inner()))?; Ok(()) }); @@ -281,14 +281,14 @@ pub fn set_target_user( pub fn kill(pid: ProcessId, signal: SignalNumber) -> io::Result<()> { // SAFETY: This function cannot cause UB even if `pid` is not a valid process ID or if // `signal` is not a valid signal code. - cerr(unsafe { libc::kill(pid.get(), signal) }).map(|_| ()) + cerr(unsafe { libc::kill(pid.inner(), signal) }).map(|_| ()) } /// Send a signal to a process group with the specified ID. pub fn killpg(pgid: ProcessId, signal: SignalNumber) -> io::Result<()> { // SAFETY: This function cannot cause UB even if `pgid` is not a valid process ID or if // `signal` is not a valid signal code. - cerr(unsafe { libc::killpg(pgid.get(), signal) }).map(|_| ()) + cerr(unsafe { libc::killpg(pgid.inner(), signal) }).map(|_| ()) } /// Get the process group ID of the current process. @@ -300,14 +300,14 @@ pub fn getpgrp() -> ProcessId { /// Get a process group ID. pub fn getpgid(pid: ProcessId) -> io::Result { // SAFETY: This function cannot cause UB even if `pid` is not a valid process ID - Ok(ProcessId::new(cerr(unsafe { libc::getpgid(pid.get()) })?)) + Ok(ProcessId::new(cerr(unsafe { libc::getpgid(pid.inner()) })?)) } /// Set a process group ID. pub fn setpgid(pid: ProcessId, pgid: ProcessId) -> io::Result<()> { // SAFETY: This function cannot cause UB even if `pid` or `pgid` are not a valid process IDs: // https://pubs.opengroup.org/onlinepubs/007904975/functions/setpgid.html - cerr(unsafe { libc::setpgid(pid.get(), pgid.get()) }).map(|_| ()) + cerr(unsafe { libc::setpgid(pid.inner(), pgid.inner()) }).map(|_| ()) } pub fn chown>( @@ -321,7 +321,7 @@ pub fn chown>( // SAFETY: path is a valid pointer to a null-terminated C string; chown cannot cause safety // issues even if uid and/or gid would be invalid identifiers. - cerr(unsafe { libc::chown(path, uid.get(), gid.get()) }).map(|_| ()) + cerr(unsafe { libc::chown(path, uid.inner(), gid.inner()) }).map(|_| ()) } #[derive(Debug, Clone, PartialEq)] @@ -396,7 +396,7 @@ impl User { // but we never dereference `pwd_ptr`. cerr(unsafe { libc::getpwuid_r( - uid.get(), + uid.inner(), pwd.as_mut_ptr(), buf.as_mut_ptr(), buf.len(), @@ -493,7 +493,7 @@ impl Group { // SAFETY: analogous to getpwuid_r above cerr(unsafe { libc::getgrgid_r( - gid.get(), + gid.inner(), grp.as_mut_ptr(), buf.as_mut_ptr(), buf.len(), diff --git a/src/system/term/mod.rs b/src/system/term/mod.rs index 525146f3f..0ea416a4f 100644 --- a/src/system/term/mod.rs +++ b/src/system/term/mod.rs @@ -157,7 +157,7 @@ impl Terminal for F { } /// Set the foreground process group ID associated with this terminalto `pgrp`. fn tcsetpgrp(&self, pgrp: ProcessId) -> io::Result<()> { - cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp.get()) }).map(|_| ()) + cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp.inner()) }).map(|_| ()) } /// Make the given terminal the controlling terminal of the calling process. @@ -246,7 +246,7 @@ mod tests { // Open a new pseudoterminal. let leader = Pty::open().unwrap().leader; // The pty leader should not have a foreground process group yet. - assert_eq!(leader.tcgetpgrp().unwrap().get(), 0); + assert_eq!(leader.tcgetpgrp().unwrap().inner(), 0); // Create a new session so we can change the controlling terminal. setsid().unwrap(); // Set the pty leader as the controlling terminal. diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs index 1085829d8..02a2709be 100644 --- a/src/system/timestamp.rs +++ b/src/system/timestamp.rs @@ -348,9 +348,9 @@ impl RecordScope { init_time, } => { target.write_all(&[1u8])?; - let b = tty_device.get().to_le_bytes(); + let b = tty_device.inner().to_le_bytes(); target.write_all(&b)?; - let b = session_pid.get().to_le_bytes(); + let b = session_pid.inner().to_le_bytes(); target.write_all(&b)?; init_time.encode(target)?; } @@ -359,7 +359,7 @@ impl RecordScope { init_time, } => { target.write_all(&[2u8])?; - let b = group_pid.get().to_le_bytes(); + let b = group_pid.inner().to_le_bytes(); target.write_all(&b)?; init_time.encode(target)?; } @@ -486,7 +486,7 @@ impl SessionRecord { self.scope.encode(target)?; // write user id - let buf = self.auth_user.get().to_le_bytes(); + let buf = self.auth_user.inner().to_le_bytes(); target.write_all(&buf)?; // write timestamp diff --git a/src/system/wait.rs b/src/system/wait.rs index 5711e8d49..072fc9d9a 100644 --- a/src/system/wait.rs +++ b/src/system/wait.rs @@ -33,7 +33,7 @@ impl Wait for ProcessId { let mut status: c_int = 0; // SAFETY: a valid pointer is passed to `waitpid` - let pid = cerr(unsafe { libc::waitpid(self.get(), &mut status, options.flags) }) + let pid = cerr(unsafe { libc::waitpid(self.inner(), &mut status, options.flags) }) .map_err(WaitError::Io)?; if pid == 0 && options.flags & WNOHANG != 0 { From 552c44068756ed0f5b4c497881a9c67ca55ab758 Mon Sep 17 00:00:00 2001 From: van-sprundel Date: Tue, 22 Oct 2024 11:39:33 +0200 Subject: [PATCH 22/22] Fix branch rebase --- src/su/context.rs | 5 ++++- src/system/mod.rs | 5 ++++- src/system/signal/info.rs | 3 ++- src/system/term/mod.rs | 8 +++++--- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/su/context.rs b/src/su/context.rs index bdf7d442d..75ee0d07e 100644 --- a/src/su/context.rs +++ b/src/su/context.rs @@ -5,7 +5,6 @@ use std::{ path::{Path, PathBuf}, }; -use crate::{common::{resolve::is_valid_executable, SudoPath}, system::interface::UserId}; use crate::exec::RunOptions; use crate::log::user_warn; use crate::system::{Group, Process, User}; @@ -13,6 +12,10 @@ use crate::{ common::{error::Error, resolve::CurrentUser, Environment}, system::interface::ProcessId, }; +use crate::{ + common::{resolve::is_valid_executable, SudoPath}, + system::interface::UserId, +}; use super::cli::SuRunOptions; diff --git a/src/system/mod.rs b/src/system/mod.rs index cd5f76332..3d58a342a 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -989,7 +989,10 @@ mod tests { assert!("SR".contains(read_proc_stat::(Current, 2).unwrap())); let parent = Process::parent_id().unwrap(); // field 3 is always the parent process - assert_eq!(parent, read_proc_stat::(Current, 3).unwrap()); + assert_eq!( + parent, + ProcessId::new(read_proc_stat::(Current, 3).unwrap()) + ); // this next field should always be 0 (which precedes an important bit of info for us!) assert_eq!(0, read_proc_stat::(Current, 20).unwrap()); } diff --git a/src/system/signal/info.rs b/src/system/signal/info.rs index bc6d44793..c48e028e3 100644 --- a/src/system/signal/info.rs +++ b/src/system/signal/info.rs @@ -23,7 +23,8 @@ impl SignalInfo { pub(crate) fn signaler_pid(&self) -> Option { if self.is_user_signaled() { // SAFETY: si_pid is always initialized if the signal is user signaled. - unsafe { Some(self.info.si_pid()) } + let id = unsafe { self.info.si_pid() }; + Some(ProcessId::new(id)) } else { None } diff --git a/src/system/term/mod.rs b/src/system/term/mod.rs index cc4934eef..91eb7cc54 100644 --- a/src/system/term/mod.rs +++ b/src/system/term/mod.rs @@ -164,12 +164,13 @@ impl Terminal for F { /// Get the foreground process group ID associated with this terminal. fn tcgetpgrp(&self) -> io::Result { // SAFETY: tcgetpgrp cannot cause UB - cerr(unsafe { libc::tcgetpgrp(self.as_raw_fd()) }) + let id = cerr(unsafe { libc::tcgetpgrp(self.as_raw_fd()) })?; + Ok(ProcessId::new(id)) } /// Set the foreground process group ID associated with this terminal to `pgrp`. fn tcsetpgrp(&self, pgrp: ProcessId) -> io::Result<()> { // SAFETY: tcsetpgrp cannot cause UB - cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp) }).map(|_| ()) + cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp.inner()) }).map(|_| ()) } /// Make the given terminal the controlling terminal of the calling process. @@ -201,7 +202,8 @@ impl Terminal for F { fn tcgetsid(&self) -> io::Result { // SAFETY: tcgetsid cannot cause UB - cerr(unsafe { libc::tcgetsid(self.as_raw_fd()) }) + let id = cerr(unsafe { libc::tcgetsid(self.as_raw_fd()) })?; + Ok(ProcessId::new(id)) } }