diff --git a/src/common/context.rs b/src/common/context.rs index 278aa9a95..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, 0); + assert_eq!(context.target_user.uid, UserId::ROOT); } } diff --git a/src/exec/interface.rs b/src/exec/interface.rs index d04c83423..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,7 +59,7 @@ impl RunOptions for Context { &self.target_group } - fn pid(&self) -> i32 { + fn pid(&self) -> ProcessId { self.process.pid } diff --git a/src/exec/no_pty.rs b/src/exec/no_pty.rs index b7c8e2019..1b9e2c98f 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.is_valid() { if Some(signaler_pid) == self.command_pid { return true; } diff --git a/src/exec/use_pty/backchannel.rs b/src/exec/use_pty/backchannel.rs index fda2c3905..b63bc334b 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.inner(), 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 20e06226c..989faaa14 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. @@ -415,7 +415,7 @@ fn is_self_terminating( command_pid: ProcessId, command_pgrp: ProcessId, ) -> bool { - if signaler_pid != 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 aed7b9dbb..bb98d83c1 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.is_valid() { if Some(signaler_pid) == self.command_pid { return true; } diff --git a/src/su/context.rs b/src/su/context.rs index f2083fc6a..75ee0d07e 100644 --- a/src/su/context.rs +++ b/src/su/context.rs @@ -5,11 +5,17 @@ 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 crate::{ + common::{resolve::is_valid_executable, SudoPath}, + system::interface::UserId, +}; use super::cli::SuRunOptions; @@ -79,7 +85,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() == UserId::ROOT; let is_target_root = options.user == "root"; // only root can set a (additional) group @@ -237,7 +243,7 @@ impl RunOptions for SuContext { &self.group } - fn pid(&self) -> i32 { + fn pid(&self) -> ProcessId { self.process.pid } diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs index 15fc0f68d..ea6e3897b 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::ROOT, + gid: GroupId::new(0), 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(0), name: "root".to_string(), }; diff --git a/src/sudo/mod.rs b/src/sudo/mod.rs index 5fda76c33..faf0d450f 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 == 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 da1f1d9a4..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 == 0 { + if context.current_user.uid == UserId::ROOT { if original_command.is_some() { return Err(Error::Silent); } diff --git a/src/sudoers/mod.rs b/src/sudoers/mod.rs index 8ee572a2d..2b82aefa3 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::*; @@ -432,7 +432,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. } } @@ -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() == *num, + Identifier::ID(num) => group.as_gid() == GroupId::new(*num), Identifier::Name(name) => group.try_as_name().map_or(false, |s| name == s), } } @@ -508,7 +508,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..ea32d31a3 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 { + UserId::new(dummy_cksum(self.0)) == uid } 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 { + GroupId::new(dummy_cksum(self.0)) == gid } fn is_root(&self) -> bool { @@ -38,8 +38,8 @@ impl UnixUser for Named { } impl UnixGroup for Named { - fn as_gid(&self) -> crate::system::interface::GroupId { - dummy_cksum(self.0) + fn as_gid(&self) -> GroupId { + 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(), 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"); diff --git a/src/system/file/chown.rs b/src/system/file/chown.rs index bbd9674a7..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, group) }).map(|_| ()) + cerr(unsafe { libc::fchown(fd, owner.inner(), group.inner()) }).map(|_| ()) } } diff --git a/src/system/interface.rs b/src/system/interface.rs index 50c79fc1c..fc4a7ff34 100644 --- a/src/system/interface.rs +++ b/src/system/interface.rs @@ -1,9 +1,111 @@ -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; +/// 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); + +/// Represents a user ID in the system. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct UserId(libc::uid_t); + +/// Represents a process ID in the system. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct ProcessId(libc::pid_t); + +/// Represents a device ID in the system. +#[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 inner(&self) -> libc::gid_t { + self.0 + } +} + +impl UserId { + pub fn new(id: libc::uid_t) -> Self { + Self(id) + } + + pub fn inner(&self) -> libc::uid_t { + self.0 + } + + pub const ROOT: Self = Self(0); +} + +impl ProcessId { + pub fn new(id: libc::pid_t) -> Self { + Self(id) + } + + pub fn inner(&self) -> libc::pid_t { + self.0 + } + + pub fn is_valid(&self) -> bool { + self.0 != 0 + } +} + +impl DeviceId { + pub fn new(id: libc::dev_t) -> Self { + Self(id) + } + + pub fn inner(&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 @@ -12,7 +114,7 @@ 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 +137,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::ROOT) } fn in_group_by_name(&self, name_c: &CStr) -> bool { if let Ok(Some(group)) = super::Group::from_name(name_c) { @@ -57,7 +159,6 @@ impl UnixGroup for super::Group { fn as_gid(&self) -> GroupId { self.gid } - fn try_as_name(&self) -> Option<&str> { Some(&self.name) } @@ -65,13 +166,11 @@ 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, ROOT_GROUP_NAME}; + use std::ffi::CString; - 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)); @@ -83,7 +182,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)); } @@ -91,16 +190,20 @@ 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::ROOT); + test_user(user(cstr!("daemon")), cstr!("daemon"), UserId::new(1)); } #[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, 0); - test_group(group(cstr!("daemon")), "daemon", 1); + test_group( + group(root_group_cstr.as_c_str()), + ROOT_GROUP_NAME, + GroupId::new(0), + ); + test_group(group(cstr!("daemon")), "daemon", GroupId::new(1)); } impl UnixUser for () {} @@ -108,7 +211,7 @@ mod test { #[test] fn test_default() { assert!(!().has_name("root")); - assert!(!().has_uid(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 a6b28333d..3d58a342a 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -136,7 +136,7 @@ pub(crate) unsafe fn fork() -> io::Result { if pid == 0 { Ok(ForkResult::Child) } else { - Ok(ForkResult::Parent(pid)) + Ok(ForkResult::Parent(ProcessId::new(pid))) } } @@ -163,7 +163,7 @@ unsafe fn fork_for_test() -> ForkResult { 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)] @@ -266,10 +266,11 @@ pub fn set_target_user( cmd.pre_exec(move || { cerr(libc::setgroups( target_user.groups.len(), - target_user.groups.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))?; - cerr(libc::setuid(target_user.uid))?; + cerr(libc::setgid(target_group.gid.inner()))?; + cerr(libc::setuid(target_user.uid.inner()))?; Ok(()) }); @@ -280,33 +281,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.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, signal) }).map(|_| ()) + cerr(unsafe { libc::killpg(pgid.inner(), 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.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, pgid) }).map(|_| ()) + cerr(unsafe { libc::setpgid(pid.inner(), pgid.inner()) }).map(|_| ()) } pub fn chown>( @@ -320,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, gid) }).map(|_| ()) + cerr(unsafe { libc::chown(path, uid.inner(), gid.inner()) }).map(|_| ()) } #[derive(Debug, Clone, PartialEq)] @@ -370,14 +371,17 @@ 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())?, 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::>(), }) } @@ -392,7 +396,7 @@ impl User { // but we never dereference `pwd_ptr`. cerr(unsafe { libc::getpwuid_r( - uid, + uid.inner(), pwd.as_mut_ptr(), buf.as_mut_ptr(), buf.len(), @@ -412,22 +416,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> { @@ -476,7 +480,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), } } @@ -489,7 +493,7 @@ impl Group { // SAFETY: analogous to getpwuid_r above cerr(unsafe { libc::getgrgid_r( - gid, + gid.inner(), grp.as_mut_ptr(), buf.as_mut_ptr(), buf.len(), @@ -574,15 +578,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.is_valid() { None } else { Some(pid) @@ -593,7 +597,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 @@ -609,7 +613,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))) } } @@ -734,6 +738,8 @@ mod tests { use libc::SIGKILL; + use crate::system::interface::{GroupId, ProcessId, UserId}; + use super::{ fork_for_test, getpgrp, setpgid, wait::{Wait, WaitOptions}, @@ -759,7 +765,7 @@ mod tests { #[test] fn test_get_user_and_group_by_id() { let fixed_users = &[ - (0, "root"), + (UserId::ROOT, "root"), ( User::from_name(cstr!("daemon")).unwrap().unwrap().uid, "daemon", @@ -767,11 +773,12 @@ mod tests { ]; for &(id, name) in fixed_users { let root = User::from_uid(id).unwrap().unwrap(); - assert_eq!(root.uid, id as libc::uid_t); + assert_eq!(root.uid, id); assert_eq!(root.name, name); } + let fixed_groups = &[ - (0, ROOT_GROUP_NAME), + (GroupId::new(0), ROOT_GROUP_NAME), ( Group::from_name(cstr!("daemon")).unwrap().unwrap().gid, "daemon", @@ -779,7 +786,7 @@ mod tests { ]; for &(id, name) in fixed_groups { let root = Group::from_gid(id).unwrap().unwrap(); - assert_eq!(root.gid, id as libc::gid_t); + assert_eq!(root.gid, id); assert_eq!(root.name, name); } } @@ -812,7 +819,7 @@ mod tests { }, Group { name: name.to_string(), - gid, + gid: GroupId::new(gid), } ) } @@ -840,8 +847,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. @@ -852,7 +862,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. @@ -866,7 +879,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] @@ -976,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 ae0bba5d7..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)) } } @@ -261,13 +263,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().inner(), 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..02a2709be 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.inner().to_le_bytes(); target.write_all(&b)?; - let b = session_pid.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.to_le_bytes(); + let b = group_pid.inner().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.inner().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)?; @@ -566,20 +567,19 @@ impl SessionRecord { #[cfg(test)] mod tests { use super::*; - use crate::system::tests::tempfile; - const TEST_USER_ID: UserId = 1000; + static TEST_USER_ID: UserId = UserId::ROOT; #[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 +596,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 +611,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 +652,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 +717,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 adb45a736..072fc9d9a 100644 --- a/src/system/wait.rs +++ b/src/system/wait.rs @@ -33,14 +33,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.inner(), &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 })) } } @@ -174,7 +174,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); @@ -201,7 +201,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(); @@ -230,7 +230,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 {