diff --git a/Cargo.toml b/Cargo.toml index e91c2354b..1047a34e9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,7 @@ name = "visudo" path = "bin/visudo.rs" [dependencies] -libc = "0.2.149" +libc = "0.2.152" glob = "0.3.0" log = { version = "0.4.11", features = ["std"] } diff --git a/src/common/context.rs b/src/common/context.rs index 8a67d73d5..f855dd6b9 100644 --- a/src/common/context.rs +++ b/src/common/context.rs @@ -31,6 +31,7 @@ pub struct Context { pub process: Process, // policy pub use_pty: bool, + pub noexec: bool, } #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] @@ -93,6 +94,7 @@ impl Context { non_interactive: sudo_options.non_interactive, process: Process::new(), use_pty: true, + noexec: false, }) } @@ -117,6 +119,7 @@ impl Context { non_interactive: sudo_options.non_interactive, process: Process::new(), use_pty: true, + noexec: false, }) } @@ -161,6 +164,7 @@ impl Context { non_interactive: sudo_options.non_interactive, process: Process::new(), use_pty: true, + noexec: false, }) } @@ -179,6 +183,7 @@ impl Context { group: &self.target_group, use_pty: self.use_pty, + noexec: self.noexec, }) } } diff --git a/src/defaults/mod.rs b/src/defaults/mod.rs index f6e52d51f..a80bce0de 100644 --- a/src/defaults/mod.rs +++ b/src/defaults/mod.rs @@ -36,6 +36,7 @@ defaults! { env_editor = true rootpw = false targetpw = false + noexec = false apparmor_profile = None (!= None) passwd_tries = 3 [0..=1000] diff --git a/src/exec/mod.rs b/src/exec/mod.rs index b873cbbf6..10bbe2365 100644 --- a/src/exec/mod.rs +++ b/src/exec/mod.rs @@ -1,6 +1,8 @@ mod event; mod io_util; mod no_pty; +#[cfg(target_os = "linux")] +mod noexec; mod use_pty; use std::{ @@ -16,15 +18,18 @@ use std::{ }; use crate::{ + common::bin_serde::BinPipe, exec::no_pty::exec_no_pty, log::{dev_info, dev_warn, user_error}, system::{ + _exit, interface::ProcessId, - killpg, - signal::{consts::*, signal_name}, + kill, killpg, set_target_user, + signal::{consts::*, signal_name, SignalNumber, SignalSet}, + term::UserTerm, wait::{Wait, WaitError, WaitOptions}, + FileCloser, Group, User, }, - system::{kill, set_target_user, signal::SignalNumber, term::UserTerm, Group, User}, }; use self::{ @@ -43,6 +48,7 @@ pub struct RunOptions<'a> { pub group: &'a Group, pub use_pty: bool, + pub noexec: bool, } /// Based on `ogsudo`s `exec_pty` function. @@ -53,6 +59,8 @@ pub fn run_command( options: RunOptions<'_>, env: impl IntoIterator, impl AsRef)>, ) -> io::Result { + let mut file_closer = FileCloser::new(); + // FIXME: should we pipe the stdio streams? let qualified_path = options.command; let mut command = Command::new(qualified_path); @@ -74,6 +82,18 @@ pub fn run_command( command.arg0(OsStr::from_bytes(&process_name)); } + let spawn_noexec_handler = if options.noexec { + #[cfg(not(target_os = "linux"))] + return Err(io::Error::other( + "NOEXEC is currently only supported on Linux", + )); + + #[cfg(target_os = "linux")] + Some(noexec::add_noexec_filter(&mut command, &mut file_closer)) + } else { + None + }; + // Decide if the pwd should be changed. `--chdir` takes precedence over `-i`. let path = options .chdir @@ -108,14 +128,20 @@ pub fn run_command( if options.use_pty { match UserTerm::open() { - Ok(user_tty) => exec_pty(sudo_pid, command, user_tty), + Ok(user_tty) => exec_pty( + sudo_pid, + file_closer, + spawn_noexec_handler, + command, + user_tty, + ), Err(err) => { dev_info!("Could not open user's terminal, not allocating a pty: {err}"); - exec_no_pty(sudo_pid, command) + exec_no_pty(sudo_pid, file_closer, spawn_noexec_handler, command) } } } else { - exec_no_pty(sudo_pid, command) + exec_no_pty(sudo_pid, file_closer, spawn_noexec_handler, command) } } @@ -126,6 +152,45 @@ pub enum ExitReason { Signal(i32), } +fn exec_command( + file_closer: FileCloser, + mut command: Command, + original_set: Option, + mut errpipe_tx: BinPipe, +) -> ! { + // Restore the signal mask now that the handlers have been setup. + if let Some(set) = original_set { + if let Err(err) = set.set_mask() { + dev_warn!("cannot restore signal mask: {err}"); + } + } + + // SAFETY: We immediately exec after this call and if the exec fails we only access stderr + // and errpipe before exiting without running atexit handlers using _exit + if let Err(err) = unsafe { file_closer.close_the_universe() } { + dev_warn!("failed to close the universe: {err}"); + // Send the error to the monitor using the pipe. + if let Some(error_code) = err.raw_os_error() { + errpipe_tx.write(&error_code).ok(); + } + + // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. + _exit(1); + } + + let err = command.exec(); + + dev_warn!("failed to execute command: {err}"); + // If `exec` returns, it means that executing the command failed. Send the error to the + // monitor using the pipe. + if let Some(error_code) = err.raw_os_error() { + errpipe_tx.write(&error_code).ok(); + } + + // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. + _exit(1); +} + // Kill the process with increasing urgency. // // Based on `terminate_command`. diff --git a/src/exec/no_pty.rs b/src/exec/no_pty.rs index 41b361d34..3d0957b20 100644 --- a/src/exec/no_pty.rs +++ b/src/exec/no_pty.rs @@ -1,4 +1,4 @@ -use std::{ffi::c_int, io, os::unix::process::CommandExt, process::Command}; +use std::{ffi::c_int, io, process::Command}; use super::{ event::PollEvent, @@ -14,10 +14,10 @@ use crate::{ }, }; use crate::{ - exec::{handle_sigchld, signal_fmt}, + exec::{exec_command, handle_sigchld, noexec::SpawnNoexecHandler, signal_fmt}, log::{dev_error, dev_info, dev_warn}, system::{ - _exit, fork, getpgid, getpgrp, + fork, getpgid, getpgrp, interface::ProcessId, kill, killpg, term::{Terminal, UserTerm}, @@ -26,7 +26,12 @@ use crate::{ }, }; -pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Result { +pub(super) fn exec_no_pty( + sudo_pid: ProcessId, + mut file_closer: FileCloser, + spawn_noexec_handler: Option, + command: Command, +) -> io::Result { // FIXME (ogsudo): Initialize the policy plugin's session here. // Block all the signals until we are done setting up the signal handlers so we don't miss @@ -39,12 +44,10 @@ pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Resu } }; - let mut file_closer = FileCloser::new(); - // FIXME (ogsudo): Some extra config happens here if selinux is available. // Use a pipe to get the IO error if `exec` fails. - let (mut errpipe_tx, errpipe_rx) = BinPipe::pair()?; + let (errpipe_tx, errpipe_rx) = BinPipe::pair()?; // Don't close the error pipe as we need it to retrieve the error code if the command execution // fails. @@ -56,39 +59,13 @@ pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Resu err })? else { - // Restore the signal mask now that the handlers have been setup. - if let Some(set) = original_set { - if let Err(err) = set.set_mask() { - dev_warn!("cannot restore signal mask: {err}"); - } - } - - // SAFETY: We immediately exec after this call and if the exec fails we only access stderr - // and errpipe before exiting without running atexit handlers using _exit - if let Err(err) = unsafe { file_closer.close_the_universe() } { - dev_warn!("failed to close the universe: {err}"); - // Send the error to the monitor using the pipe. - if let Some(error_code) = err.raw_os_error() { - errpipe_tx.write(&error_code).ok(); - } - - // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. - _exit(1); - } - - let err = command.exec(); - - dev_warn!("failed to execute command: {err}"); - // If `exec` returns, it means that executing the command failed. Send the error to the - // monitor using the pipe. - if let Some(error_code) = err.raw_os_error() { - errpipe_tx.write(&error_code).ok(); - } - - // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. - _exit(1); + exec_command(file_closer, command, original_set, errpipe_tx); }; + if let Some(spawner) = spawn_noexec_handler { + spawner.spawn(); + } + dev_info!("executed command with pid {command_pid}"); let mut registry = EventRegistry::new(); diff --git a/src/exec/noexec.rs b/src/exec/noexec.rs new file mode 100644 index 000000000..087a6573f --- /dev/null +++ b/src/exec/noexec.rs @@ -0,0 +1,341 @@ +// On Linux we can use a seccomp() filter to disable exec. + +use std::alloc::{handle_alloc_error, GlobalAlloc, Layout}; +use std::ffi::c_void; +use std::mem::{align_of, size_of, zeroed}; +use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd}; +use std::os::unix::net::UnixStream; +use std::os::unix::process::CommandExt; +use std::process::Command; +use std::ptr::{self, addr_of}; +use std::{cmp, io, thread}; + +use libc::{ + c_int, c_uint, c_ulong, close, cmsghdr, iovec, msghdr, prctl, recvmsg, seccomp_data, + seccomp_notif, seccomp_notif_resp, seccomp_notif_sizes, sendmsg, sock_filter, sock_fprog, + syscall, SYS_execve, SYS_execveat, SYS_seccomp, __errno_location, BPF_ABS, BPF_JEQ, BPF_JMP, + BPF_JUMP, BPF_K, BPF_LD, BPF_RET, BPF_STMT, CMSG_DATA, CMSG_FIRSTHDR, CMSG_LEN, CMSG_SPACE, + EACCES, ENOENT, MSG_TRUNC, PR_SET_NO_NEW_PRIVS, SCM_RIGHTS, SECCOMP_FILTER_FLAG_NEW_LISTENER, + SECCOMP_GET_NOTIF_SIZES, SECCOMP_RET_ALLOW, SECCOMP_SET_MODE_FILTER, + SECCOMP_USER_NOTIF_FLAG_CONTINUE, SOL_SOCKET, +}; + +use crate::system::FileCloser; + +const SECCOMP_RET_USER_NOTIF: c_uint = 0x7fc00000; +const SECCOMP_IOCTL_NOTIF_RECV: c_ulong = 0xc0502100; +const SECCOMP_IOCTL_NOTIF_SEND: c_ulong = 0xc0182101; + +/// # Safety +/// +/// You must follow the rules the Linux man page specifies for the chosen +/// seccomp operation. +unsafe fn seccomp(operation: c_uint, flags: c_uint, args: *mut T) -> c_int { + // SAFETY: By function invariant. + unsafe { syscall(SYS_seccomp, operation, flags, args) as c_int } +} + +struct NotifyAllocs { + req: *mut seccomp_notif, + req_size: usize, + resp: *mut seccomp_notif_resp, +} + +/// Linux reserves the right to demand the memory for an object of type T +/// to be over-allocated; this function ensures that happens. +fn alloc_dynamic(runtime_size: u16) -> (*mut T, usize) { + // FIXME put this in a const block once the MSRV has been bumped enough + assert!(size_of::() > 0); + + let layout = Layout::from_size_align( + cmp::max(runtime_size.into(), size_of::()), + align_of::(), + ) + .unwrap(); + + // SAFETY: We assert that T is bigger than 0 bytes and as such the computed layout is also + // bigger. + let ptr = unsafe { std::alloc::System.alloc_zeroed(layout).cast::() }; + if ptr.is_null() { + handle_alloc_error(layout); + } + + (ptr, layout.size()) +} + +fn alloc_notify_allocs() -> NotifyAllocs { + let mut sizes = seccomp_notif_sizes { + seccomp_notif: 0, + seccomp_notif_resp: 0, + seccomp_data: 0, + }; + // SAFETY: A valid seccomp_notif_sizes pointer is passed in + if unsafe { seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &mut sizes) } == -1 { + panic!( + "failed to get sizes for seccomp unotify data structures: {}", + io::Error::last_os_error(), + ); + } + + let (req, req_size) = alloc_dynamic::(sizes.seccomp_notif); + let (resp, _) = alloc_dynamic::(sizes.seccomp_notif_resp); + + NotifyAllocs { + req, + req_size, + resp, + } +} + +/// Returns 'None' if the ioctl failed with E_NOENT, 'Some(())' if it succeeded. +/// This aborts the program in any other situation. +/// +/// # Safety +/// +/// `ioctl(fd, request, ptr)` must be safe to call +unsafe fn ioctl(fd: RawFd, request: libc::c_ulong, ptr: *mut T) -> Option<()> { + // SAFETY: By function contract + if unsafe { libc::ioctl(fd, request as _, ptr) } == -1 { + // SAFETY: Trivial + if unsafe { *__errno_location() } == ENOENT { + None + } else { + // SAFETY: Not actually unsafe + unsafe { + libc::abort(); + } + } + } else { + Some(()) + } +} + +/// # Safety +/// +/// The argument must be a valid seccomp_unotify fd. +unsafe fn handle_notifications(notify_fd: OwnedFd) -> ! { + let NotifyAllocs { + req, + req_size, + resp, + } = alloc_notify_allocs(); + + // SAFETY: See individual SAFETY comments + let handle_syscall = |create_response: fn(&mut _)| unsafe { + // SECCOMP_IOCTL_NOTIF_RECV expects the target struct to be zeroed + // SAFETY: req is at least req_size bytes big. + std::ptr::write_bytes(req.cast::(), 0, req_size); + + // SAFETY: A valid pointer to a seccomp_notify is passed in; notify_fd is valid. + ioctl(notify_fd.as_raw_fd(), SECCOMP_IOCTL_NOTIF_RECV, req)?; + + // Allow the first execve call as this is sudo itself starting the target executable. + // SAFETY: resp is a valid pointer to a seccomp_notify_resp. + (*resp).id = (*req).id; + create_response(&mut *resp); + + // SAFETY: A valid pointer to a seccomp_notify_resp is passed in; notify_fd is valid. + ioctl(notify_fd.as_raw_fd(), SECCOMP_IOCTL_NOTIF_SEND, resp) + }; + + loop { + if handle_syscall(|resp| { + resp.val = 0; + resp.error = 0; + resp.flags = SECCOMP_USER_NOTIF_FLAG_CONTINUE as _ + }) + .is_some() + { + break; + } + } + + loop { + handle_syscall(|resp| { + resp.val = 0; + resp.error = -EACCES; + resp.flags = 0; + }); + } +} + +//We must use vectored reads with ancillary data. +// +//NOTE: some day we can witch to using send/recv_vectored_with_ancillary; see: +// - https://doc.rust-lang.org/std/os/unix/net/struct.UnixDatagram.html#method.recv_vectored_with_ancillary +// - https://doc.rust-lang.org/std/os/unix/net/struct.UnixDatagram.html#method.send_vectored_with_ancillary +// but this is (at the time of writing) unstable. + +#[repr(C)] +union SingleRightAnciliaryData { + // SAFETY: Not actually unsafe + #[allow(clippy::undocumented_unsafe_blocks)] // Clippy doesn't understand the safety comment + buf: [u8; unsafe { CMSG_SPACE(size_of::() as u32) as usize }], + _align: cmsghdr, +} + +/// Receives a raw file descriptor from the provided UnixStream +fn receive_fd(rx_fd: UnixStream) -> RawFd { + let mut data = [0u8; 1]; + let mut iov = iovec { + iov_base: &mut data as *mut [u8; 1] as *mut c_void, + iov_len: 1, + }; + + // SAFETY: msghdr can be zero-initialized + let mut msg: msghdr = unsafe { zeroed() }; + msg.msg_name = ptr::null_mut(); + msg.msg_namelen = 0; + msg.msg_iov = &mut iov; + msg.msg_iovlen = 1; + + // SAFETY: SingleRightAnciliaryData can be zero-initialized. + let mut control: SingleRightAnciliaryData = unsafe { zeroed() }; + // SAFETY: The buf field is valid when zero-initialized. + msg.msg_controllen = unsafe { control.buf.len() as _ }; + msg.msg_control = &mut control as *mut _ as *mut libc::c_void; + + // SAFETY: A valid socket fd and a valid initialized msghdr are passed in. + if unsafe { recvmsg(rx_fd.as_raw_fd(), &mut msg, 0) } == -1 { + panic!("failed to recvmsg: {}", io::Error::last_os_error()); + } + + if msg.msg_flags & MSG_TRUNC == MSG_TRUNC { + unreachable!("unexpected internal error in seccomp filter"); + } + + // SAFETY: The kernel correctly initializes everything on recvmsg for this to be safe. + unsafe { + let cmsgp = CMSG_FIRSTHDR(&msg); + if cmsgp.is_null() + || (*cmsgp).cmsg_len != CMSG_LEN(size_of::() as u32) as _ + || (*cmsgp).cmsg_level != SOL_SOCKET + || (*cmsgp).cmsg_type != SCM_RIGHTS + { + unreachable!("unexpected response from Linux kernel"); + } + CMSG_DATA(cmsgp).cast::().read() + } +} + +fn send_fd(tx_fd: UnixStream, notify_fd: RawFd) -> io::Result<()> { + let mut data = [0u8; 1]; + let mut iov = iovec { + iov_base: &mut data as *mut [u8; 1] as *mut c_void, + iov_len: 1, + }; + + // SAFETY: msghdr can be zero-initialized + let mut msg: msghdr = unsafe { zeroed() }; + msg.msg_name = ptr::null_mut(); + msg.msg_namelen = 0; + msg.msg_iov = &mut iov; + msg.msg_iovlen = 1; + + // SAFETY: SingleRightAnciliaryData can be zero-initialized. + let mut control: SingleRightAnciliaryData = unsafe { zeroed() }; + // SAFETY: The buf field is valid when zero-initialized. + msg.msg_controllen = unsafe { control.buf.len() as _ }; + msg.msg_control = &mut control as *mut _ as *mut _; + // SAFETY: msg.msg_control is correctly initialized and this follows + // the contract of the various CMSG_* macros. + unsafe { + let cmsgp = CMSG_FIRSTHDR(&msg); + (*cmsgp).cmsg_level = SOL_SOCKET; + (*cmsgp).cmsg_type = SCM_RIGHTS; + (*cmsgp).cmsg_len = CMSG_LEN(size_of::() as u32) as _; + ptr::write(CMSG_DATA(cmsgp).cast::(), notify_fd); + } + + // SAFETY: A valid socket fd and a valid initialized msghdr are passed in. + if unsafe { sendmsg(tx_fd.as_raw_fd(), &msg, 0) } == -1 { + Err(io::Error::last_os_error()) + } else { + Ok(()) + } +} + +pub(crate) struct SpawnNoexecHandler(UnixStream); + +impl SpawnNoexecHandler { + pub(super) fn spawn(self) { + thread::spawn(move || { + let notify_fd = receive_fd(self.0); + // SAFETY: notify_fd is a valid seccomp_unotify fd. + unsafe { handle_notifications(OwnedFd::from_raw_fd(notify_fd)) }; + }); + } +} + +pub(crate) fn add_noexec_filter( + command: &mut Command, + file_closer: &mut FileCloser, +) -> SpawnNoexecHandler { + let (tx_fd, rx_fd) = UnixStream::pair().unwrap(); + + file_closer.except(&tx_fd); + + // wrap tx_fd so it can be moved into the closure + let mut tx_fd = Some(tx_fd); + + // SAFETY: See individual SAFETY comments + unsafe { + // SAFETY: The closure only calls async-signal-safe functions. + command.pre_exec(move || { + let tx_fd = tx_fd.take().unwrap(); + + // FIXME replace with offset_of!(seccomp_data, nr) once MSRV is bumped to 1.77 + // SAFETY: seccomp_data can be safely zero-initialized. + let dummy: seccomp_data = zeroed(); + let nr_offset = (&dummy.nr) as *const _ as usize - &dummy as *const _ as usize; + + // SAFETY: libc unnecessarily marks these functions as unsafe + let exec_filter: [sock_filter; 5] = [ + // Load syscall number into the accumulator + BPF_STMT((BPF_LD | BPF_ABS) as _, nr_offset as _), + // Jump to user notify for execve/execveat + BPF_JUMP((BPF_JMP | BPF_JEQ | BPF_K) as _, SYS_execve as _, 2, 0), + BPF_JUMP((BPF_JMP | BPF_JEQ | BPF_K) as _, SYS_execveat as _, 1, 0), + // Allow non-matching syscalls + BPF_STMT((BPF_RET | BPF_K) as _, SECCOMP_RET_ALLOW), + // Notify sudo about execve/execveat syscall + BPF_STMT((BPF_RET | BPF_K) as _, SECCOMP_RET_USER_NOTIF as _), + ]; + + let exec_fprog = sock_fprog { + len: 5, + filter: addr_of!(exec_filter) as *mut sock_filter, + }; + + // SAFETY: Trivially safe as it doesn't touch any memory. + // SECCOMP_SET_MODE_FILTER will fail unless the process has + // CAP_SYS_ADMIN or the no_new_privs bit is set. + if prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) == -1 { + return Err(io::Error::last_os_error()); + } + + // While the man page warns againt using seccomp_unotify as security + // mechanism, the TOCTOU problem that is described there isn't + // relevant here. We only SECCOMP_USER_NOTIF_FLAG_CONTINUE the first + // execve which is done by ourself and thus trusted. + // SAFETY: Passes a valid sock_fprog as argument. + let notify_fd = seccomp( + SECCOMP_SET_MODE_FILTER, + SECCOMP_FILTER_FLAG_NEW_LISTENER as _, + addr_of!(exec_fprog).cast_mut(), + ); + if notify_fd < 0 { + return Err(io::Error::last_os_error()); + } + + send_fd(tx_fd, notify_fd)?; + + // SAFETY: Nothing will access the notify_fd after this call. + close(notify_fd); + + Ok(()) + }); + } + + SpawnNoexecHandler(rx_fd) +} diff --git a/src/exec/use_pty/monitor.rs b/src/exec/use_pty/monitor.rs index d334eb553..ec1f73364 100644 --- a/src/exec/use_pty/monitor.rs +++ b/src/exec/use_pty/monitor.rs @@ -1,4 +1,4 @@ -use std::{convert::Infallible, ffi::c_int, io, os::unix::process::CommandExt, process::Command}; +use std::{convert::Infallible, ffi::c_int, io, process::Command}; use crate::exec::{opt_fmt, signal_fmt}; use crate::system::signal::{ @@ -9,6 +9,7 @@ use crate::{ common::bin_serde::BinPipe, exec::{ event::{EventRegistry, Process}, + exec_command, io_util::{retry_while_interrupted, was_interrupted}, use_pty::backchannel::{MonitorBackchannel, MonitorMessage, ParentMessage}, }, @@ -93,14 +94,23 @@ pub(super) fn exec_monitor( else { drop(errpipe_rx); - match exec_command( - command, - foreground, - pty_follower, - file_closer, - errpipe_tx, - original_set, - ) {} + // FIXME (ogsudo): Do any additional configuration that needs to be run after `fork` but before `exec` + let command_pid = ProcessId::new(std::process::id() as i32); + + 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. + if foreground { + while !pty_follower.tcgetpgrp().is_ok_and(|pid| pid == command_pid) { + std::thread::yield_now(); + } + } + + // Done with the pty follower. + drop(pty_follower); + + exec_command(file_closer, command, original_set, errpipe_tx) }; // Send the command's PID to the parent. @@ -188,63 +198,6 @@ pub(super) fn exec_monitor( _exit(1); } -fn exec_command( - mut command: Command, - foreground: bool, - pty_follower: PtyFollower, - file_closer: FileCloser, - mut errpipe_tx: BinPipe, - original_set: Option, -) -> ! { - // FIXME (ogsudo): Do any additional configuration that needs to be run after `fork` but before `exec` - let command_pid = ProcessId::new(std::process::id() as i32); - - 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. - if foreground { - while !pty_follower.tcgetpgrp().is_ok_and(|pid| pid == command_pid) { - std::thread::yield_now(); - } - } - - // Done with the pty follower. - drop(pty_follower); - - // Restore the signal mask now that the handlers have been setup. - if let Some(set) = original_set { - if let Err(err) = set.set_mask() { - dev_warn!("cannot restore signal mask: {err}"); - } - } - - // SAFETY: We immediately exec after this call and if the exec fails we only access stderr - // and errpipe before exiting without running atexit handlers using _exit - if let Err(err) = unsafe { file_closer.close_the_universe() } { - dev_warn!("failed to close the universe: {err}"); - // Send the error to the monitor using the pipe. - if let Some(error_code) = err.raw_os_error() { - errpipe_tx.write(&error_code).ok(); - } - - // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. - _exit(1); - } - - let err = command.exec(); - - dev_warn!("failed to execute command: {err}"); - // If `exec_command` returns, it means that executing the command failed. Send the error to - // the monitor using the pipe. - if let Some(error_code) = err.raw_os_error() { - errpipe_tx.write(&error_code).ok(); - } - - // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. - _exit(1); -} - struct MonitorClosure<'a> { /// The command PID. /// diff --git a/src/exec/use_pty/parent.rs b/src/exec/use_pty/parent.rs index aaf45de09..db3c3e9ac 100644 --- a/src/exec/use_pty/parent.rs +++ b/src/exec/use_pty/parent.rs @@ -4,6 +4,7 @@ use std::io; use std::process::{Command, Stdio}; use crate::exec::event::{EventHandle, EventRegistry, PollEvent, Process, StopReason}; +use crate::exec::noexec::SpawnNoexecHandler; use crate::exec::use_pty::monitor::exec_monitor; use crate::exec::use_pty::SIGCONT_FG; use crate::exec::{cond_fmt, handle_sigchld, signal_fmt, terminate_process, HandleSigchld}; @@ -29,6 +30,8 @@ use super::{CommandStatus, SIGCONT_BG}; pub(in crate::exec) fn exec_pty( sudo_pid: ProcessId, + mut file_closer: FileCloser, + spawn_noexec_handler: Option, mut command: Command, user_tty: UserTerm, ) -> io::Result { @@ -58,8 +61,6 @@ pub(in crate::exec) fn exec_pty( // Fetch the parent process group so we can signals to it. let parent_pgrp = getpgrp(); - let mut file_closer = FileCloser::new(); - // Set all the IO streams for the command to the follower side of the pty. let mut clone_follower = || -> io::Result { let follower = pty.follower.try_clone().map_err(|err| { @@ -206,6 +207,10 @@ pub(in crate::exec) fn exec_pty( _exit(1); }; + if let Some(spawner) = spawn_noexec_handler { + spawner.spawn(); + } + // Close the file descriptors that we don't access drop(pty.follower); drop(backchannels.monitor); diff --git a/src/su/context.rs b/src/su/context.rs index 23e94246d..5beeb7ecc 100644 --- a/src/su/context.rs +++ b/src/su/context.rs @@ -218,6 +218,7 @@ impl SuContext { group: &self.group, use_pty: true, + noexec: false, } } } diff --git a/src/sudo/env/environment.rs b/src/sudo/env/environment.rs index 3eaf74546..03e911f6d 100644 --- a/src/sudo/env/environment.rs +++ b/src/sudo/env/environment.rs @@ -282,6 +282,7 @@ mod tests { use_pty: true, #[cfg(feature = "apparmor")] apparmor_profile: None, + noexec: false, } ), expected, diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs index 8ab782881..335554cd4 100644 --- a/src/sudo/env/tests.rs +++ b/src/sudo/env/tests.rs @@ -132,6 +132,7 @@ fn create_test_context(sudo_options: SudoRunOptions) -> Context { process: Process::new(), use_session_records: false, use_pty: true, + noexec: false, bell: false, } } @@ -171,6 +172,7 @@ fn test_environment_variable_filtering() { trust_environment: false, #[cfg(feature = "apparmor")] apparmor_profile: None, + noexec: false, }, ) .unwrap(); diff --git a/src/sudo/pipeline.rs b/src/sudo/pipeline.rs index 1ce09b105..0467128c0 100644 --- a/src/sudo/pipeline.rs +++ b/src/sudo/pipeline.rs @@ -237,6 +237,7 @@ fn apply_policy_to_context( // in case the user could set these from the commandline, something more fancy // could be needed, but here we copy these -- perhaps we should split up the Context type context.use_pty = controls.use_pty; + context.noexec = controls.noexec; Ok(()) } diff --git a/src/sudoers/ast.rs b/src/sudoers/ast.rs index 9b7ba8248..be59e34e4 100644 --- a/src/sudoers/ast.rs +++ b/src/sudoers/ast.rs @@ -97,6 +97,18 @@ pub enum EnvironmentControl { Nosetenv = HARDENED_ENUM_VALUE_2, } +#[derive(Copy, Clone, Default, PartialEq)] +#[cfg_attr(test, derive(Debug, Eq))] +#[repr(u32)] +pub enum ExecControl { + #[default] + Implicit = HARDENED_ENUM_VALUE_0, + // PASSWD: + Exec = HARDENED_ENUM_VALUE_1, + // NOPASSWD: + Noexec = HARDENED_ENUM_VALUE_2, +} + /// Commands in /etc/sudoers can have attributes attached to them, such as NOPASSWD, NOEXEC, ... #[derive(Default, Clone, PartialEq)] #[cfg_attr(test, derive(Debug, Eq))] @@ -105,6 +117,7 @@ pub struct Tag { pub(super) cwd: Option, pub(super) env: EnvironmentControl, pub(super) apparmor_profile: Option, + pub(super) noexec: ExecControl, } impl Tag { @@ -375,11 +388,11 @@ impl Parse for MetaOrTag { }; let result: Modifier = match keyword.as_str() { - // we do not support these, and that should make sudo-rs "fail safe" - "INTERCEPT" | "NOEXEC" => unrecoverable!( + // we do not support this, and that should make sudo-rs "fail safe" + "INTERCEPT" => unrecoverable!( pos = start_pos, stream, - "NOEXEC and INTERCEPT are not supported by sudo-rs" + "INTERCEPT is not supported by sudo-rs" ), // this is less fatal "LOG_INPUT" | "NOLOG_INPUT" | "LOG_OUTPUT" | "NOLOG_OUTPUT" | "MAIL" | "NOMAIL" => { @@ -391,8 +404,11 @@ impl Parse for MetaOrTag { } // 'FOLLOW' and 'NOFOLLOW' are only usable in a sudoedit context, which will result in - // a parse error elsewhere. 'EXEC' and 'NOINTERCEPT' are the default behaviour. - "FOLLOW" | "NOFOLLOW" | "EXEC" | "NOINTERCEPT" => switch(|_| {})?, + // a parse error elsewhere. 'NOINTERCEPT' is the default behaviour. + "FOLLOW" | "NOFOLLOW" | "NOINTERCEPT" => switch(|_| {})?, + + "EXEC" => switch(|tag| tag.noexec = ExecControl::Exec)?, + "NOEXEC" => switch(|tag| tag.noexec = ExecControl::Noexec)?, "SETENV" => switch(|tag| tag.env = EnvironmentControl::Setenv)?, "NOSETENV" => switch(|tag| tag.env = EnvironmentControl::Nosetenv)?, diff --git a/src/sudoers/policy.rs b/src/sudoers/policy.rs index 6a1de0d4c..296ff00a7 100644 --- a/src/sudoers/policy.rs +++ b/src/sudoers/policy.rs @@ -4,7 +4,7 @@ use super::Judgement; use crate::common::{ SudoPath, HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2, }; -use crate::sudoers::ast::Tag; +use crate::sudoers::ast::{ExecControl, Tag}; use crate::system::{time::Duration, Hostname, User}; /// Data types and traits that represent what the "terms and conditions" are after a succesful /// permission check. @@ -54,6 +54,7 @@ impl super::Settings { pub struct Restrictions<'a> { pub use_pty: bool, pub trust_environment: bool, + pub noexec: bool, pub env_keep: &'a HashSet, pub env_check: &'a HashSet, pub chdir: DirChange<'a>, @@ -95,6 +96,11 @@ impl Judgement { super::EnvironmentControl::Setenv => true, super::EnvironmentControl::Nosetenv => false, }, + noexec: match tag.noexec { + ExecControl::Implicit => self.settings.noexec(), + ExecControl::Exec => false, + ExecControl::Noexec => true, + }, env_keep: self.settings.env_keep(), env_check: self.settings.env_check(), chdir: match tag.cwd.as_ref() { diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers.rs index 26a1e457e..dab622672 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/sudoers.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/sudoers.rs @@ -12,6 +12,7 @@ mod host_alias; mod host_list; mod include; mod includedir; +mod noexec; mod run_as; mod runas_alias; mod secure_path; diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers/noexec.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers/noexec.rs new file mode 100644 index 000000000..73dc85d65 --- /dev/null +++ b/test-framework/sudo-compliance-tests/src/sudo/sudoers/noexec.rs @@ -0,0 +1,136 @@ +//! Test the NOEXEC tag and the noexec option + +use sudo_test::{Command, Env, BIN_TRUE}; + +use crate::{Result, USERNAME}; + +#[test] +fn sanity_check() -> Result<()> { + let env = Env("Defaults noexec\nALL ALL=(ALL:ALL) NOPASSWD: ALL") + .user(USERNAME) + .build(); + + Command::new("sudo") + .arg("/usr/bin/true") + .as_user(USERNAME) + .output(&env) + .assert_success(); + + Ok(()) +} + +#[test] +fn exec_denied() -> Result<()> { + let env = Env("Defaults noexec\nALL ALL=(ALL:ALL) NOPASSWD: ALL") + .user(USERNAME) + .build(); + + let output = Command::new("sudo") + .args(["sh", "-c", BIN_TRUE]) + .as_user(USERNAME) + .output(&env); + + assert!( + !output.status().success(), + "stdout:\n{}\n\nstderr:\n{}", + output.stdout_unchecked(), + output.stderr(), + ); + + assert!(output.stderr().contains("Permission denied")); + + Ok(()) +} + +#[test] +fn exec_denied_second_time() -> Result<()> { + let env = Env("Defaults noexec\nALL ALL=(ALL:ALL) NOPASSWD: ALL") + .user(USERNAME) + .build(); + + let output = Command::new("sudo") + .args(["sh", "-c"]) + .arg(format!("{BIN_TRUE} || {BIN_TRUE}")) + .as_user(USERNAME) + .output(&env); + + assert!( + !output.status().success(), + "stdout:\n{}\n\nstderr:\n{}", + output.stdout_unchecked(), + output.stderr(), + ); + + assert_eq!( + output.stderr(), + "sh: 1: /usr/bin/true: Permission denied +sh: 1: /usr/bin/true: Permission denied" + ); + + Ok(()) +} + +#[test] +fn exec_denied_noexec_tag() -> Result<()> { + let env = Env("ALL ALL=(ALL:ALL) NOPASSWD: NOEXEC: ALL") + .user(USERNAME) + .build(); + + let output = Command::new("sudo") + .args(["sh", "-c", BIN_TRUE]) + .as_user(USERNAME) + .output(&env); + + assert!( + !output.status().success(), + "stdout:\n{}\n\nstderr:\n{}", + output.stdout_unchecked(), + output.stderr(), + ); + + assert!( + output.stderr().contains("Permission denied"), + "stderr:\n{}", + output.stderr(), + ); + + Ok(()) +} + +#[test] +fn exec_overrides_noexec_default() -> Result<()> { + let env = Env("Defaults noexec\nALL ALL=(ALL:ALL) NOPASSWD: EXEC: ALL") + .user(USERNAME) + .build(); + + Command::new("sudo") + .args(["sh", "-c", BIN_TRUE]) + .as_user(USERNAME) + .output(&env) + .assert_success(); + + Ok(()) +} + +#[test] +fn no_use_pty_works() -> Result<()> { + let env = Env("Defaults noexec, !use_pty\nALL ALL=(ALL:ALL) NOPASSWD: ALL") + .user(USERNAME) + .build(); + + let output = Command::new("sudo") + .args(["sh", "-c", BIN_TRUE]) + .as_user(USERNAME) + .output(&env); + + assert!( + !output.status().success(), + "stdout:\n{}\n\nstderr:\n{}", + output.stdout_unchecked(), + output.stderr(), + ); + + assert!(output.stderr().contains("Permission denied")); + + Ok(()) +}