From b7bfbd6df6ae9af9d5985eeb57bb2f493e4834f1 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 30 Oct 2026 22:53:29 +0100 Subject: [PATCH] fix(broker): validate inherited descriptors before borrowing them Borrowing a descriptor requires it to be open at that moment, so the broker entry cannot check a raw number through a borrow. Check both numbers with getsockopt first, as main did, which keeps the rejection of closed descriptors and makes the borrow's safety claim true. The entry exits 126 either way, so this only restores the contract. Pin that rejection with an integration test against the real binary. --- AGENTS.md | 2 +- src/process_broker/server.rs | 42 +++++++++++++++++++++++++----------- tests/cli.rs | 13 +++++++++++ 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b6994c8e..8194e042 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,7 +40,7 @@ - Never reach inside a poisoned lock. Use `expect` when the critical section cannot panic, or a fallback that leaves the data alone. Tests that serialize on process-wide state use `test_env::lock()` or a `test_env::Lease`. - `catch_unwind` is reserved for the reviewed worker-isolation sites that carry an `#[expect]` reason. Tests observe panics through a joined scoped thread. - Getters are `foo()`/`foo_mut()`, a fallible constructor is `try_new`, and type lifetimes have descriptive names. Every `unsafe` block has a `// SAFETY:` comment that says why it is sound. -- Make kernel calls through `std` or `rustix`, which need no `unsafe`, and rename on import when a module name clashes with `std` (`use rustix::fs as rustix_fs`). Use raw `libc` calls only where neither fits: signal handlers, the post-`clone` broker stub, `fail_stop`, the zero-signal pidfd check, the inotify header parser, `dup2` onto a fixed descriptor number, and `localtime_r`. `libc` constants stay fine. +- Make kernel calls through `std` or `rustix`, which need no `unsafe`, and rename on import when a module name clashes with `std` (`use rustix::fs as rustix_fs`). Use raw `libc` calls only where neither fits: signal handlers, the post-`clone` broker stub, `fail_stop`, the zero-signal pidfd check, validating the broker's inherited descriptors, the inotify header parser, `dup2` onto a fixed descriptor number, and `localtime_r`. `libc` constants stay fine. - Convert numbers losslessly with `From` (`f64::from(width)`) wherever it can be called; `const` initializers keep `as`. Take the middle of two values with `midpoint` (`f64::midpoint(a, b)`), which cannot overflow. - Inline named format arguments: `format!("{name}: {err}")`. - Use `map_or`, `map_or_else`, `is_some_and`, or `is_ok_and` instead of `map(..).unwrap_or(..)`. Use `let ... else` for a `match` that only unwraps or leaves, and `if let ... else` for a `match` with one pattern and a fallback. diff --git a/src/process_broker/server.rs b/src/process_broker/server.rs index 5b4646b2..1b41bb21 100644 --- a/src/process_broker/server.rs +++ b/src/process_broker/server.rs @@ -4,13 +4,11 @@ use std::os::unix::process::CommandExt; use std::process::{Child, ExitCode, Stdio}; use std::thread::Builder; use std::time::Duration; -use std::{env, io}; +use std::{env, io, mem}; use anyhow::{Context, Result, anyhow, bail}; use rustix::event::{self, PollFd, PollFlags}; use rustix::io::{self as rustix_io, Errno, FdFlags}; -use rustix::net::SocketType; -use rustix::net::sockopt; use rustix::process::{self as rustix_process, Pid, Signal}; use crate::daemon::protocol_v2::ProtocolId; @@ -38,21 +36,22 @@ pub(crate) fn run_internal_broker_if_requested() -> Option { .parse::() .ok()?; let token = env::var(BROKER_TOKEN_ENV).ok()?; - if fd != BROKER_FD || shutdown_fd != BROKER_SHUTDOWN_FD || !canonical_lower_hex(&token, 64) { + if fd != BROKER_FD + || shutdown_fd != BROKER_SHUTDOWN_FD + || !canonical_lower_hex(&token, 64) + || validate_broker_socket(fd).is_err() + || validate_broker_socket(shutdown_fd).is_err() + { return Some(ExitCode::from(126)); } - // SAFETY: the fixed numbers are never -1, the socket checks below reject a - // number that is not an open broker channel before anything else uses it, - // and nothing in this process closes those channels before it exits. + // SAFETY: both numbers were just checked to be open broker sockets, and + // nothing in this process closes them before it exits. let (fd, shutdown_fd) = unsafe { ( BorrowedFd::borrow_raw(fd), BorrowedFd::borrow_raw(shutdown_fd), ) }; - if validate_broker_socket(fd).is_err() || validate_broker_socket(shutdown_fd).is_err() { - return Some(ExitCode::from(126)); - } // Restore CLOEXEC before any runtime helper can inherit broker internals. for descriptor in [fd, shutdown_fd] { if rustix_io::fcntl_setfd(descriptor, FdFlags::CLOEXEC).is_err() { @@ -71,8 +70,25 @@ pub(crate) fn run_internal_broker_if_requested() -> Option { }) } -fn validate_broker_socket(fd: BorrowedFd<'_>) -> io::Result<()> { - if sockopt::socket_type(fd)? != SocketType::SEQPACKET { +fn validate_broker_socket(fd: RawFd) -> io::Result<()> { + let mut socket_type = 0_i32; + let mut length = mem::size_of::() as libc::socklen_t; + // SAFETY: the output slots are valid for getsockopt, which also rejects a + // number that is not an open descriptor. A borrowed descriptor cannot be + // used here because it requires the number to be open already. + if unsafe { + libc::getsockopt( + fd, + libc::SOL_SOCKET, + libc::SO_TYPE, + (&raw mut socket_type).cast(), + &raw mut length, + ) + } != 0 + { + return Err(io::Error::last_os_error()); + } + if socket_type != libc::SOCK_SEQPACKET { return Err(io::Error::new( io::ErrorKind::InvalidInput, "broker descriptor is not SOCK_SEQPACKET", @@ -542,7 +558,7 @@ fn canonical_lower_hex(value: &str, length: usize) -> bool { mod tests { use std::process::Command; - use rustix::net::{self, AddressFamily, SendFlags, SocketFlags}; + use rustix::net::{self, AddressFamily, SendFlags, SocketFlags, SocketType}; use super::*; diff --git a/tests/cli.rs b/tests/cli.rs index 00838a2e..f47a35a5 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -343,6 +343,19 @@ fn clustered_help_and_version_exit_zero_without_runtime_startup() { .stdout_starts_with("wayscriber "); } +#[cfg(unix)] +#[test] +fn internal_broker_entry_rejects_descriptors_that_are_not_broker_sockets() { + run_command( + wayscriber_cmd() + .env("WAYSCRIBER_INTERNAL_PROCESS_BROKER_FD", "3") + .env("WAYSCRIBER_INTERNAL_PROCESS_BROKER_SHUTDOWN_FD", "4") + .env("WAYSCRIBER_INTERNAL_PROCESS_BROKER_TOKEN", "a".repeat(64)), + ) + .failure() + .code(126); +} + #[test] fn invalid_arguments_exit_two() { for (args, diagnostic) in [