From cd578f72d6677af5172a5d278254d260cd0468f7 Mon Sep 17 00:00:00 2001 From: Ethan Pailes Date: Fri, 24 Apr 2026 21:09:20 +0000 Subject: [PATCH] fix use-after-free soundness issues This patch fixes some use-after-free soundness issues that are due to the way that Master and Slave are cloneable. Fortunately, I think the API also not being correctly RAII meant that this wasn't causing crashes (though only because we were leaking resources). I'm beginning to think it was a mistake to use the pty crate. This is a breaking change so I'll need to bump the minor version. --- src/descriptor/mod.rs | 68 +++------- src/fork/mod.rs | 12 +- src/fork/pty/master/err.rs | 2 - src/fork/pty/master/mod.rs | 135 +++++++++----------- src/fork/pty/slave/err.rs | 2 - src/fork/pty/slave/mod.rs | 56 ++++---- tests/it_protects_against_use_after_free.rs | 108 ++++++++++++++++ 7 files changed, 211 insertions(+), 172 deletions(-) create mode 100644 tests/it_protects_against_use_after_free.rs diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 6b9859d..ed59e85 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -1,59 +1,23 @@ mod err; pub use self::err::DescriptorError; -use std::ffi::CStr; -use std::os::unix::io::RawFd; +use std::{ + ffi::CStr, + os::fd::{FromRawFd, OwnedFd}, +}; -/// # Safety -/// -/// Implmentations MUST only return valid fds from -/// these methods. Callers MUST NOT use the fds once the -/// returning object has fallen out of scope. -pub unsafe trait Descriptor { - /// If the descriptor has a valid fd, return it - fn take_raw_fd(&mut self) -> Option; - - /// The constructor function `open` opens the path - /// and returns the fd. - fn open( - path: &CStr, - flag: libc::c_int, - mode: Option, - ) -> Result { - // Safety: we've just ensured that path is non-null and the - // other params are valid by construction. - unsafe { - match libc::open(path.as_ptr().cast(), flag, mode.unwrap_or_default()) { - -1 => Err(DescriptorError::OpenFail), - fd => Ok(fd), - } - } - } - - /// The function `close` leaves the fd. - fn close(&mut self) -> Result<(), DescriptorError> { - if let Some(fd) = self.take_raw_fd() { - // Safety: we take the fd here, ensuring that it cannot - // be used or closed again afterwards. N.B. this is only - // safe because there is no way for a descriptor to hand - // out the raw fd for it to be stored somewhere and then - // used by external code. - unsafe { - match libc::close(fd) { - -1 => Err(DescriptorError::CloseFail), - _ => Ok(()), - } - } - } else { - Ok(()) - } - } - - /// The destructor function `drop` call the method `close` - /// and log if an error occurred. - fn drop(&mut self) { - if let Err(e) = self.close() { - log::warn!("error closing fd: {:?}", e); +/// Open the given file as a file descriptor. +pub fn open( + path: &CStr, + flag: libc::c_int, + mode: Option, +) -> Result { + // Safety: we've just ensured that path is non-null and the + // other params are valid by construction. + unsafe { + match libc::open(path.as_ptr().cast(), flag, mode.unwrap_or_default()) { + -1 => Err(DescriptorError::OpenFail), + fd => Ok(OwnedFd::from_raw_fd(fd)), } } } diff --git a/src/fork/mod.rs b/src/fork/mod.rs index c4fac42..a5163df 100644 --- a/src/fork/mod.rs +++ b/src/fork/mod.rs @@ -1,8 +1,6 @@ mod err; mod pty; -use descriptor::Descriptor; - pub use self::err::{ForkError, Result}; pub use self::pty::{Master, MasterError}; pub use self::pty::{Slave, SlaveError}; @@ -132,7 +130,7 @@ impl Fork { pub fn is_parent(&self) -> Result { match *self { Fork::Child(_) => Err(ForkError::IsChild), - Fork::Parent(_, ref master) => Ok(*master), + Fork::Parent(_, ref master) => Ok(master.clone()), } } @@ -145,11 +143,3 @@ impl Fork { } } } - -impl Drop for Fork { - fn drop(&mut self) { - if let Fork::Parent(_, master) = self { - Descriptor::drop(master) - } - } -} diff --git a/src/fork/pty/master/err.rs b/src/fork/pty/master/err.rs index e71fd8f..08e51c8 100644 --- a/src/fork/pty/master/err.rs +++ b/src/fork/pty/master/err.rs @@ -12,7 +12,6 @@ pub enum MasterError { GrantptError, UnlockptError, PtsnameError, - NoFdError, } impl fmt::Display for MasterError { @@ -30,7 +29,6 @@ impl Error for MasterError { MasterError::GrantptError => "the `grantpt` has a error, errnois set appropriately.", MasterError::UnlockptError => "the `grantpt` has a error, errnois set appropriately.", MasterError::PtsnameError => "the `ptsname` has a error", - MasterError::NoFdError => "already closed, no fd", } } diff --git a/src/fork/pty/master/mod.rs b/src/fork/pty/master/mod.rs index a4170f1..7b9bcd6 100644 --- a/src/fork/pty/master/mod.rs +++ b/src/fork/pty/master/mod.rs @@ -3,126 +3,117 @@ mod err; #[cfg(target_os = "macos")] mod ptsname_r_macos; -use descriptor::Descriptor; +use crate::descriptor; pub use self::err::{MasterError, Result}; -use std::ffi::CStr; -use std::io; -use std::os::fd::BorrowedFd; -use std::os::unix::io::RawFd; - -#[derive(Debug, Copy, Clone)] +use std::{ + ffi::CStr, + io, + os::{ + fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd}, + unix::io::RawFd, + }, + sync::Arc, +}; + +#[derive(Debug, Clone)] pub struct Master { - pty: Option, + pty: Arc, } impl Master { pub fn new(path: &CStr) -> Result { - match Self::open(path, libc::O_RDWR, None) { + match descriptor::open(path, libc::O_RDWR, None) { Err(cause) => Err(MasterError::BadDescriptor(cause)), - Ok(fd) => Ok(Master { pty: Some(fd) }), + Ok(fd) => Ok(Master { pty: Arc::new(fd) }), } } /// Extract the raw fd from the underlying object - pub fn raw_fd(&self) -> &Option { - &self.pty + pub fn raw_fd(&self) -> RawFd { + self.pty.as_raw_fd() } /// Borrow the raw fd - pub fn borrow_fd(&self) -> Option> { - // Safety: we only ever close on drop, so this will be - // live the whole time. - self.pty.map(|fd| unsafe { BorrowedFd::borrow_raw(fd) }) + pub fn borrow_fd(&self) -> BorrowedFd<'_> { + self.pty.as_fd() } /// Change UID and GID of slave pty associated with master pty whose /// fd is provided, to the real UID and real GID of the calling thread. pub fn grantpt(&self) -> Result { - if let Some(fd) = self.pty { - unsafe { - match libc::grantpt(fd) { - -1 => Err(MasterError::GrantptError), - c => Ok(c), - } + // Safety: pty is live across the lifetime of this call, + // so the fd is valid. + unsafe { + match libc::grantpt(self.raw_fd()) { + -1 => Err(MasterError::GrantptError), + c => Ok(c), } - } else { - Err(MasterError::NoFdError) } } /// Unlock the slave pty associated with the master to which fd refers. pub fn unlockpt(&self) -> Result { - if let Some(fd) = self.pty { - unsafe { - match libc::unlockpt(fd) { - -1 => Err(MasterError::UnlockptError), - c => Ok(c), - } + // Safety: pty is live across the lifetime of this call, + // so the fd is valid. + unsafe { + match libc::unlockpt(self.raw_fd()) { + -1 => Err(MasterError::UnlockptError), + c => Ok(c), } - } else { - Err(MasterError::NoFdError) } } /// Returns a pointer to a static buffer, which will be overwritten on /// subsequent calls. pub fn ptsname_r(&self, buf: &mut [u8]) -> Result<()> { - if let Some(fd) = self.pty { - // Safety: the vector's memory is valid for the duration - // of the call - unsafe { - let data: *mut u8 = &mut buf[0]; - - #[cfg(any(target_os = "linux", target_os = "android"))] - let result = libc::ptsname_r(fd, data as *mut libc::c_char, buf.len()); - - #[cfg(target_os = "macos")] - let result = ptsname_r_macos::ptsname_r(fd, data as *mut libc::c_char, buf.len()); - - match result { - 0 => Ok(()), - _ => Err(MasterError::PtsnameError), // should probably capture errno - } + // Safety: the vector's memory is valid for the duration + // of the call + unsafe { + let data: *mut u8 = &mut buf[0]; + + #[cfg(any(target_os = "linux", target_os = "android"))] + let result = libc::ptsname_r(self.raw_fd(), data as *mut libc::c_char, buf.len()); + + #[cfg(target_os = "macos")] + let result = ptsname_r_macos::ptsname_r(fd, data as *mut libc::c_char, buf.len()); + + match result { + 0 => Ok(()), + _ => Err(MasterError::PtsnameError), // should probably capture errno } - } else { - Err(MasterError::NoFdError) } } } -unsafe impl Descriptor for Master { - fn take_raw_fd(&mut self) -> Option { - self.pty.take() - } -} - impl io::Read for Master { fn read(&mut self, buf: &mut [u8]) -> io::Result { - if let Some(fd) = self.pty { - unsafe { - match libc::read(fd, buf.as_mut_ptr() as *mut libc::c_void, buf.len()) { - -1 => Ok(0), - len => Ok(len as usize), - } + // Safety: the vector's memory is valid for the duration + // of the call + unsafe { + match libc::read( + self.raw_fd(), + buf.as_mut_ptr() as *mut libc::c_void, + buf.len(), + ) { + -1 => Ok(0), + len => Ok(len as usize), } - } else { - Err(io::Error::other("already closed")) } } } impl io::Write for Master { fn write(&mut self, buf: &[u8]) -> io::Result { - if let Some(fd) = self.pty { - unsafe { - match libc::write(fd, buf.as_ptr() as *const libc::c_void, buf.len()) { - -1 => Err(io::Error::last_os_error()), - ret => Ok(ret as usize), - } + unsafe { + match libc::write( + self.raw_fd(), + buf.as_ptr() as *const libc::c_void, + buf.len(), + ) { + -1 => Err(io::Error::last_os_error()), + ret => Ok(ret as usize), } - } else { - Err(io::Error::other("already closed")) } } diff --git a/src/fork/pty/slave/err.rs b/src/fork/pty/slave/err.rs index 6ddfe95..eec8926 100644 --- a/src/fork/pty/slave/err.rs +++ b/src/fork/pty/slave/err.rs @@ -10,7 +10,6 @@ pub type Result = ::std::result::Result; pub enum SlaveError { BadDescriptor(DescriptorError), Dup2Error, - NoFdError, } impl fmt::Display for SlaveError { @@ -26,7 +25,6 @@ impl Error for SlaveError { match *self { SlaveError::BadDescriptor(_) => "the descriptor as occured an error", SlaveError::Dup2Error => "the `dup2` has a error, errno isset appropriately.", - SlaveError::NoFdError => "the pty has already been closed, no fd", } } diff --git a/src/fork/pty/slave/mod.rs b/src/fork/pty/slave/mod.rs index 4e04b25..3f8a574 100644 --- a/src/fork/pty/slave/mod.rs +++ b/src/fork/pty/slave/mod.rs @@ -1,60 +1,50 @@ mod err; -use descriptor::Descriptor; +use crate::descriptor; pub use self::err::{Result, SlaveError}; -use std::ffi::CStr; -use std::os::fd::BorrowedFd; -use std::os::unix::io::RawFd; + +use std::{ + ffi::CStr, + os::{ + fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd}, + unix::io::RawFd, + }, + sync::Arc, +}; #[derive(Debug, Clone)] pub struct Slave { - pty: Option, + pty: Arc, } impl Slave { /// The constructor function `new` returns the Slave interface. pub fn new(path: &CStr) -> Result { - match Self::open(path, libc::O_RDWR, None) { + match descriptor::open(path, libc::O_RDWR, None) { Err(cause) => Err(SlaveError::BadDescriptor(cause)), - Ok(fd) => Ok(Slave { pty: Some(fd) }), + Ok(fd) => Ok(Slave { pty: Arc::new(fd) }), } } /// Extract the raw fd from the underlying object - pub fn raw_fd(&self) -> &Option { - &self.pty + pub fn raw_fd(&self) -> RawFd { + self.pty.as_raw_fd() } /// Borrow the raw fd - pub fn borrow_fd(&self) -> Option> { - // Safety: we only ever close on drop, so this will be - // live the whole time. - self.pty.map(|fd| unsafe { BorrowedFd::borrow_raw(fd) }) + pub fn borrow_fd(&self) -> BorrowedFd<'_> { + self.pty.as_fd() } pub fn dup2(&self, std: libc::c_int) -> Result { - if let Some(fd) = self.pty { - unsafe { - match libc::dup2(fd, std) { - -1 => Err(SlaveError::Dup2Error), - d => Ok(d), - } + // Safety: pty is live across the lifetime of this call, + // so the fd is valid. + unsafe { + match libc::dup2(self.raw_fd(), std) { + -1 => Err(SlaveError::Dup2Error), + d => Ok(d), } - } else { - Err(SlaveError::NoFdError) } } } - -unsafe impl Descriptor for Slave { - fn take_raw_fd(&mut self) -> Option { - self.pty.take() - } -} - -impl Drop for Slave { - fn drop(&mut self) { - Descriptor::drop(self); - } -} diff --git a/tests/it_protects_against_use_after_free.rs b/tests/it_protects_against_use_after_free.rs new file mode 100644 index 0000000..c8edc86 --- /dev/null +++ b/tests/it_protects_against_use_after_free.rs @@ -0,0 +1,108 @@ +extern crate libc; +extern crate shpool_pty; + +use self::shpool_pty::prelude::*; + +#[test] +fn it_drops_correctly() { + let fork = Fork::from_ptmx().expect("failed to fork"); + + let master = match fork.is_parent() { + Ok(m) => m, + Err(_) => { + // Child process + unsafe { libc::_exit(0) }; + } + }; + + let fd = master.raw_fd(); + + // Check if fd is valid + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } != -1, + "FD should be valid" + ); + + let master_clone = master.clone(); + assert_eq!(master.raw_fd(), master_clone.raw_fd()); + + drop(master); + // FD should still be valid because master_clone exists + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } != -1, + "FD should still be valid after first drop" + ); + + drop(master_clone); + // FD should still be valid because fork still exists and holds a master + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } != -1, + "FD should still be valid because fork exists" + ); + + drop(fork); + // Now it should be closed + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1, + "FD should be closed after last drop" + ); +} + +#[test] +fn fork_drop_does_not_close_master() { + let fork = Fork::from_ptmx().expect("failed to fork"); + + let master = match fork.is_parent() { + Ok(m) => m, + Err(_) => { + // Child process + unsafe { libc::_exit(0) }; + } + }; + + let fd = master.raw_fd(); + + drop(fork); + // Master should still be valid + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } != -1, + "FD should still be valid after fork drop" + ); + + drop(master); + // Now it should be closed + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1, + "FD should be closed after master drop" + ); +} + +#[test] +fn slave_cloning_and_dropping() { + let master = + Master::new(&std::ffi::CString::new("/dev/ptmx").unwrap()).expect("failed to open ptmx"); + master.grantpt().expect("failed to grantpt"); + master.unlockpt().expect("failed to unlockpt"); + + let mut buf = [0u8; 128]; + master.ptsname_r(&mut buf).expect("failed to get ptsname"); + let name = std::ffi::CStr::from_bytes_until_nul(&buf).expect("failed to parse ptsname"); + + let slave = Slave::new(name).expect("failed to open slave"); + let fd = slave.raw_fd(); + + let slave_clone = slave.clone(); + assert_eq!(slave.raw_fd(), slave_clone.raw_fd()); + + drop(slave); + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } != -1, + "Slave FD should still be valid after first drop" + ); + + drop(slave_clone); + assert!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1, + "Slave FD should be closed after last drop" + ); +}