mirror of
https://github.com/devmobasa/wayscriber.git
synced 2026-11-01 07:36:42 +01:00
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.
This commit is contained in:
1 parent
66e9259f6f
commit
b7bfbd6df6
3 files changed
+43
-14
No files matched your search
@@ -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.
|
||||
|
||||
@@ -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<ExitCode> {
|
||||
.parse::<RawFd>()
|
||||
.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<ExitCode> {
|
||||
})
|
||||
}
|
||||
|
||||
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::<i32>() 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::*;
|
||||
|
||||
|
||||
@@ -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 [
|
||||
|
||||
Reference in new issue
Block a user