check PID of named pipe consumer (#19283)

## Why
The elevated Windows command runner currently trusts the first process
that connects to its parent-created named pipes. Tightening the pipe ACL
already narrows who can reach that boundary, but verifying the connected
client PID gives the parent one more fail-closed check: it only accepts
the exact runner process it just spawned.

## What changed
- validate `GetNamedPipeClientProcessId` after `ConnectNamedPipe` and
reject clients whose PID does not match the spawned runner
- also did some code de-duplication to route the one-shot elevated
capture flow in `windows-sandbox-rs/src/elevated_impl.rs` through
`spawn_runner_transport()` so both elevated codepaths use the same pipe
bootstrap and PID validation

Using the transport unification here also reduces duplication in the
elevated Windows IPC bootstrap, so future hardening to the runner
handshake only needs to land in one place.

## Validation
- `cargo test -p codex-windows-sandbox`
- manual testing: one-shot elevated path via `target/debug/codex.exe
exec` running a randomized shell command and confirming captured output
- manual testing: elevated session path via `target/debug/codex.exe -c
'windows.sandbox="elevated"' sandbox windows -- python -u -c ...` with
stdin/stdout round-trips (`READY`, then `GOT:...` for two input lines)

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
This commit is contained in:
iceweasel-oaiandviyatb-oai authored and GitHub committed 2026-04-24 17:41:08 +00:00
1 parent bcc1caa920
commit e787358f70
4 files changed
+52 -269

No files matched your search

@@ -52,7 +52,6 @@ impl RunnerTransport {
}
pub(crate) fn read_spawn_ready(&mut self) -> Result<()> {
wait_for_complete_frame(&self.pipe_read, RUNNER_SPAWN_READY_TIMEOUT)?;
let msg = read_frame(&mut self.pipe_read)?
.ok_or_else(|| anyhow::anyhow!("runner pipe closed before spawn_ready"))?;
match msg.message {
@@ -64,6 +63,11 @@ impl RunnerTransport {
}
}
pub(crate) fn read_spawn_ready_with_timeout(&mut self) -> Result<()> {
wait_for_complete_frame(&self.pipe_read, RUNNER_SPAWN_READY_TIMEOUT)?;
self.read_spawn_ready()
}
pub(crate) fn into_files(self) -> (File, File) {
(self.pipe_write, self.pipe_read)
}
@@ -134,10 +138,11 @@ pub(crate) fn spawn_runner_transport(
}
return Err(anyhow::anyhow!("CreateProcessWithLogonW failed: {err}"));
}
let expected_runner_pid = pi.dwProcessId;
let connect_result = (|| -> Result<()> {
connect_pipe(h_pipe_in)?;
connect_pipe(h_pipe_out)?;
connect_pipe(h_pipe_in, expected_runner_pid)?;
connect_pipe(h_pipe_out, expected_runner_pid)?;
Ok(())
})();
@@ -1,7 +1,8 @@
//! Named pipe helpers for the elevated Windows sandbox runner.
//!
//! This module generates paired pipe names, creates server‑side pipes with permissive
//! ACLs, and waits for the runner to connect. It is **elevated-path only** and is
//! This module generates paired pipe names, creates server‑side pipes with
//! sandbox-user-scoped ACLs, and waits for the runner to connect. It is
//! **elevated-path only** and is
//! used by the parent to establish the IPC channel for both unified_exec sessions
//! and elevated capture. The legacy restricted‑token path spawns the child directly
//! and does not use these helpers.
@@ -27,6 +28,7 @@ use windows_sys::Win32::Security::PSECURITY_DESCRIPTOR;
use windows_sys::Win32::Security::SECURITY_ATTRIBUTES;
use windows_sys::Win32::System::Pipes::ConnectNamedPipe;
use windows_sys::Win32::System::Pipes::CreateNamedPipeW;
use windows_sys::Win32::System::Pipes::GetNamedPipeClientProcessId;
use windows_sys::Win32::System::Pipes::PIPE_READMODE_BYTE;
use windows_sys::Win32::System::Pipes::PIPE_TYPE_BYTE;
use windows_sys::Win32::System::Pipes::PIPE_WAIT;
@@ -103,8 +105,9 @@ pub fn create_named_pipe(name: &str, access: u32, sandbox_username: &str) -> io:
/// Waits for the runner to connect to a parent-created server pipe.
///
/// This is parent-side only: the runner opens the pipe with `CreateFileW`, while the
/// parent calls `ConnectNamedPipe` and tolerates the already-connected case.
pub fn connect_pipe(h: HANDLE) -> io::Result<()> {
/// parent calls `ConnectNamedPipe`, tolerates the already-connected case, and
/// verifies that the connected client is the runner process we just spawned.
pub fn connect_pipe(h: HANDLE, expected_runner_pid: u32) -> io::Result<()> {
let ok = unsafe { ConnectNamedPipe(h, ptr::null_mut()) };
if ok == 0 {
let err = unsafe { GetLastError() };
@@ -113,5 +116,20 @@ pub fn connect_pipe(h: HANDLE) -> io::Result<()> {
return Err(io::Error::from_raw_os_error(err as i32));
}
}
let mut client_pid = 0;
let ok = unsafe { GetNamedPipeClientProcessId(h, &mut client_pid) };
if ok == 0 {
return Err(io::Error::from_raw_os_error(unsafe {
GetLastError() as i32
}));
}
if client_pid != expected_runner_pid {
return Err(io::Error::new(
io::ErrorKind::PermissionDenied,
format!(
"named pipe client pid {client_pid} did not match runner pid {expected_runner_pid}"
),
));
}
Ok(())
}