From 475bf250748101b25961d6a31b1e20da180e43d8 Mon Sep 17 00:00:00 2001 From: Kotauskas Date: Thu, 20 Aug 2020 15:39:19 +0300 Subject: [PATCH] Fixed credential passing and signal soundness --- Cargo.toml | 7 ++- src/lib.rs | 5 +- src/os/unix/signal.rs | 122 ++++++++++++++++++++++++++++++++++++++-- src/os/unix/udsocket.rs | 69 ++++++++++++++++++----- 4 files changed, 178 insertions(+), 25 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a638f693..fc86fde5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "interprocess" -version = "0.1.1" +version = "0.2.0" authors = ["Kotauskas "] edition = "2018" license = "MIT" @@ -15,7 +15,7 @@ keywords = ["ipc", "shared_memory", "pipe", "mailslot", "unix_domain_socket"] # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -tokio = {version = "0.2", optional = true} +#tokio = {version = "0.2", optional = true} [target.'cfg(windows)'.dependencies] winapi = {version = "0.3.9", features = ["std", "winbase", "winerror", "processthreadsapi", "fileapi", "handleapi", "namedpipeapi"]} @@ -25,7 +25,8 @@ spin = "0.5" cfg-if = "0.1" intmap = "0.7" lazy_static = "1.4" +thiserror = "1.0" [features] default = [] -tokio_integration = ["tokio"] \ No newline at end of file +#tokio_integration = ["tokio"] \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index c1a6196a..438d5985 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -21,8 +21,7 @@ //! //! [MIT license]: https://choosealicense.com/licenses/mit/ " " -// If the trait itself was pub(crate), it wouldn't work as a supertrait on public traits. We use a -// private module instead to make it impossible to name the trait from outside the crate. +#![allow(unused_unsafe)] // If an operating system is not listed here, the `compile_error!` is invoked #[cfg(not(any( @@ -124,6 +123,8 @@ pub(crate) mod private { } }; } + // If the trait itself was pub(crate), it wouldn't work as a supertrait on public traits. We use a + // private module instead to make it impossible to name the trait from outside the crate. pub trait Sealed {} } diff --git a/src/os/unix/signal.rs b/src/os/unix/signal.rs index c453dcad..5037a8b2 100644 --- a/src/os/unix/signal.rs +++ b/src/os/unix/signal.rs @@ -154,6 +154,7 @@ use std::{ }; use spin::RwLock; use intmap::IntMap; +use thiserror::Error; use cfg_if::cfg_if; use lazy_static::lazy_static; cfg_if! { @@ -241,11 +242,48 @@ lazy_static! { /// /// [`HandlerOptions`]: struct.HandlerOptions.html " " #[inline] -pub fn set_handler(signal_type: SignalType, handler: SignalHandler) { +pub fn set_handler(signal_type: SignalType, handler: SignalHandler) -> Result<(), SetHandlerError> { HandlerOptions::for_signal(signal_type) .set_new_handler(handler) .set() } +/// Installs the specified handler for the specified unsafe signal, using the default values for the flags. +/// +/// See [`HandlerOptions`] builder if you'd like to customize the flags. +/// +/// # Safety +/// See the [`set_unsafe`] safety notes. +/// +/// # Example +/// ```no_run +/// use interprocess::os::unix::signal::{self, SignalType, SignalHandler}; +/// +/// let handler = unsafe { +/// // Since signal handlers are restricted to a specific set of system calls, creating a +/// // handler from an arbitrary function is unsafe because it might perform a system call +/// // outside the list, and there's no real way to know that at compile time with the +/// // current version of Rust. Since we're only using the write() system call here, this +/// // is safe. +/// SignalHandler::from_fn(|| { +/// println!("Oh no, the motherboard broke!"); +/// std::process::abort(); +/// }) +/// }; +/// +/// unsafe { +/// // Install our handler for the MemoryBusError signal type. +/// signal::set_unsafe_handler(SignalType::MemoryBusError, handler); +/// } +/// ``` +/// +/// [`HandlerOptions`]: struct.HandlerOptions.html " " +/// [`set_unsafe`]: struct.HandlerOptions.html#method.set_unsafe " " +#[inline] +pub unsafe fn set_unsafe_handler(signal_type: SignalType, handler: SignalHandler) -> Result<(), SetHandlerError> { + HandlerOptions::for_signal(signal_type) + .set_new_handler(handler) + .set_unsafe() +} /// Installs the specified handler for the specified real-time signal, using the default values for the flags. /// /// See [`HandlerOptions`] builder if you'd like to customize the flags. @@ -271,7 +309,7 @@ pub fn set_handler(signal_type: SignalType, handler: SignalHandler) { /// /// [`HandlerOptions`]: struct.HandlerOptions.html " " #[inline] -pub fn set_rthandler(rtsignal: u32, handler: SignalHandler) { +pub fn set_rthandler(rtsignal: u32, handler: SignalHandler) -> Result<(), SetHandlerError> { HandlerOptions::for_rtsignal(rtsignal) .set_new_handler(handler) .set() @@ -429,7 +467,33 @@ impl HandlerOptions { self } /// Installs the signal handler. - pub fn set(self) { + #[inline] + pub fn set(self) -> Result<(), SetHandlerError> { + if let Ok(val) = SignalType::try_from(self.signal) { + if val.is_unsafe() { + return Err(SetHandlerError::UnsafeSignal); + } + } + unsafe {self.set_unsafe()} + } + + /// Installs the signal handler, even if the signal being handled is unsafe. + /// + /// # Safety + /// The handler and all code that may or may not execute afterwards must be prepared for the aftermath of what might've caused the signal. [`SegmentationFault`], for example, might be caused by hitting a stack protector as a response to a thread's stack overflow — in such a case, continuing the program might result in undefined behavior. The Rust runtime actually sets its own handler for `SegmentationFault` signals which converts those signals to proper shutdowns, i.e. ignoring it essentially disables stack protectors, which is unsound. + /// + /// [`SegmentationFault`]: enum.SignalType.html#variant.SegmentationFault " " + pub unsafe fn set_unsafe(self) -> Result<(), SetHandlerError> { + if let Ok(val) = SignalType::try_from(self.signal) { + if val.is_unblockable() { + return Err(SetHandlerError::UnblockableSignal(val)); + } + } else if !is_valid_rtsignal(self.signal as u32) { + return Err(SetHandlerError::RealTimeSignalOutOfBounds { + attempted: self.signal as u32, + max: NUM_REALTIME_SIGNALS, + }); + } let handlers = HANDLERS.upgradeable_read(); let new_flags = self.flags_as_i32(); let mut need_to_upgrade_handle = false; @@ -470,8 +534,8 @@ impl HandlerOptions { self.signal, hook_val, new_flags, - ) - }.expect("unexpected signal handler installation failure") + )? + } } if need_to_upgrade_handle { let mut handlers = handlers.upgrade(); @@ -482,6 +546,7 @@ impl HandlerOptions { new_flags, )); } + Ok(()) } #[inline] @@ -516,6 +581,28 @@ impl HandlerOptions { } } +#[derive(Debug, Error)] +pub enum SetHandlerError { + /// An unsafe signal was attempted to be handled using `set` instead of `unsafe_set`. + #[error("an unsafe signal was attempted to be handled using `set` instead of `unsafe_set`")] + UnsafeSignal, + /// The signal which was attempted to be handled is not allowed to be handled by the POSIX specification. This can either be [`ForceSuspend`] or [`Kill`]. + /// + /// [`Kill`]: enum.SignalType.html#variant.Kill " " + /// [`ForceSuspend`]: enum.SignalType.html#variant.ForceSuspend " " + #[error("the signal {:?} cannot be handled", .0)] + UnblockableSignal (SignalType), + /// The specified real-time signal is not available on this OS. + #[error("the real-time signal number {} is not available ({} is the highest possible)", .attempted, .max)] + RealTimeSignalOutOfBounds { + attempted: u32, + max: u32, + }, + /// An unexpected OS error ocurred during signal handler setup. + #[error("{}", .0)] + UnexpectedSystemCallFailure (#[from] io::Error), +} + /// The actual hook which is passed to `sigaction` which dispatches signals according to the global handler map (the `HANDLERS` static). extern fn signal_receiver(signum: i32) { let catched = panic::catch_unwind(|| { @@ -797,6 +884,7 @@ pub fn send_rt_to_group(signal: impl Into>, pid: impl Into) -> /// /// [`i32`]: https://doc.rust-lang.org/std/primitive.i32.html " " /// [`u32`]: https://doc.rust-lang.org/std/primitive.u32.html " " +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] #[repr(i32)] #[non_exhaustive] pub enum SignalType { @@ -965,6 +1053,30 @@ pub enum SignalType { /// [`setrlimit`]: https://www.man7.org/linux/man-pages/man2/setrlimit.2.html " " FileSizeLimitExceeded = SIGXFSZ, } +impl SignalType { + /// Returns `true` if the value is a special signal which cannot be blocked or handled ([`Kill`] or [`ForceSuspend`]), `false` otherwise. + /// + /// [`Kill`]: #variant.Kill " " + /// [`ForceSuspend`]: #variant.ForceSuspend " " + #[inline] + pub fn is_unblockable(self) -> bool { + match self { + Self::Kill + | Self::ForceSuspend + => true, + _ => false, + } + } + /// Returns `true` if the value is an unsafe signal which requires unsafe code when setting a handling method, `false` otherwise. + pub fn is_unsafe(self) -> bool { + match self { + Self::SegmentationFault + | Self::MemoryBusError + => true, + _ => false, + } + } +} impl From for i32 { fn from(op: SignalType) -> Self { unsafe { diff --git a/src/os/unix/udsocket.rs b/src/os/unix/udsocket.rs index 772beaa7..81d3fa6a 100644 --- a/src/os/unix/udsocket.rs +++ b/src/os/unix/udsocket.rs @@ -16,7 +16,7 @@ use libc::{ pid_t, uid_t, gid_t, AF_UNIX, SOCK_STREAM, SOCK_DGRAM, - SOL_SOCKET, + SOL_SOCKET, SO_PASSCRED, SCM_RIGHTS, SCM_CREDENTIALS, MSG_TRUNC, MSG_CTRUNC, MSG_PEEK, sockaddr_un, @@ -268,12 +268,20 @@ impl UdStreamListener { // but you can cast any narrow pointer to any other narrow pointer &addr as *const _ as *const _, addrlen as u32, - ) != -1 { - // FIXME the standard library uses 128 here without an option to change this - // number, why? If std has solid reasons to do this, remove this notice and - // document the method's behavior on this matter explicitly; otherwise, add - // an option to change this value. - libc::listen(socket, 128) != 1 + ) != -1 + // FIXME the standard library uses 128 here without an option to change this + // number, why? If std has solid reasons to do this, remove this notice and + // document the method's behavior on this matter explicitly; otherwise, add + // an option to change this value. + && libc::listen(socket, 128) != -1 { + let passcred: c_int = 1; + libc::setsockopt( + socket, + SOL_SOCKET, + SO_PASSCRED, + &passcred as *const _ as *const _, + mem::size_of_val(&passcred) as u32, + ) != -1 } else { false } @@ -513,12 +521,23 @@ impl UdStream { } }; let success = unsafe { - libc::connect( + if libc::connect( socket, // Same as in UdSocketListener::bind() &addr as *const _ as *const _, addrlen as u32, - ) != -1 + ) != -1 { + let passcred: c_int = 1; + libc::setsockopt( + socket, + SOL_SOCKET, + SO_PASSCRED, + &passcred as *const _ as *const _, + mem::size_of_val(&passcred) as u32, + ) != -1 + } else { + false + } }; if success { Ok(unsafe { @@ -783,15 +802,24 @@ impl UdSocket { } }; let success = unsafe { - // If binding didn't fail, start listening and return true if it succeeded and false if - // it failed; if binding failed, short-circuit to returning false - libc::bind( + if libc::bind( socket, // Double cast because you cannot cast a reference to a pointer of arbitrary type // but you can cast any narrow pointer to any other narrow pointer &addr as *const _ as *const _, addrlen as u32, - ) != -1 + ) != -1 { + let passcred: c_int = 1; + libc::setsockopt( + socket, + SOL_SOCKET, + SO_PASSCRED, + &passcred as *const _ as *const _, + mem::size_of_val(&passcred) as u32, + ) != -1 + } else { + false + } }; if success { Ok( @@ -841,12 +869,23 @@ impl UdSocket { } }; let success = unsafe { - libc::connect( + if libc::connect( socket, // Same as in UdSocketListener::bind() &addr as *const _ as *const _, addrlen as u32, - ) != -1 + ) != -1 { + let passcred: c_int = 1; + libc::setsockopt( + socket, + SOL_SOCKET, + SO_PASSCRED, + &passcred as *const _ as *const _, + mem::size_of_val(&passcred) as u32, + ) != -1 + } else { + false + } }; if success { Ok(unsafe {