From ec9ad96b9842b70cfa0fa46ec6ea49eba205b3ff Mon Sep 17 00:00:00 2001 From: Josh Megnauth Date: Wed, 8 Jul 2026 21:47:59 -0400 Subject: [PATCH] ffi: No interior NULs (part 1) Interior NULs is a security hazard for C-style strings. A NUL byte truncates a string which can lead the caller and callee to see two different strings. It can cause path traversal attacks where a path in Python looks complete but it is interpreted differently through FFI. RustPython needs to handle this for its C-API as well as raw libc or Windows calls. Both Rust's standard library as well as Rustix handle interior NULs for us. Finally, this PR is non-exhaustive. I will have to rely heavily on CodeRabbit to help lint it to ensure that interior NUL checks are only introduced for FFI and not outside of it. **AI disclosure:** I relied on AI to ensure I'm solving this problem correctly. Mainly, I used it to check if the FFI functions I'm modifying need to handle interior NULs. **Sources:** * https://owasp.org/www-community/attacks/Embedding_Null_Code * python/cpython#11656 Assisted-by: Codex --- crates/host_env/src/ctypes.rs | 49 ++++++++++-------- crates/host_env/src/fileutils.rs | 9 +++- crates/host_env/src/windows.rs | 56 +++++++++------------ crates/vm/src/stdlib/_ctypes/base.rs | 19 ++++--- crates/vm/src/stdlib/_ctypes/function.rs | 4 +- crates/wtf8/src/lib.rs | 63 ++++++++++++++++++++++++ 6 files changed, 133 insertions(+), 67 deletions(-) diff --git a/crates/host_env/src/ctypes.rs b/crates/host_env/src/ctypes.rs index 9bfd41c2818..4ce7b0c4c32 100644 --- a/crates/host_env/src/ctypes.rs +++ b/crates/host_env/src/ctypes.rs @@ -1,4 +1,6 @@ use alloc::borrow::Cow; +#[cfg(unix)] +use alloc::ffi::CString; use core::ffi::{ CStr, c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar, c_uint, c_ulong, c_ulonglong, c_ushort, c_void, @@ -32,8 +34,8 @@ use libloading::Library; use libloading::os::unix::Library as UnixLibrary; #[cfg(any(unix, windows))] use parking_lot::{Mutex, RwLock}; -use rustpython_wtf8::Wtf8; use rustpython_wtf8::Wtf8Buf; +use rustpython_wtf8::{InteriorNulError, Wtf8}; #[cfg(any(unix, windows))] use std::{collections::HashMap, ffi::OsStr, sync::OnceLock}; @@ -383,7 +385,7 @@ pub fn dlopen_mode(load_flags: Option) -> i32 { #[cfg(target_os = "macos")] pub fn dyld_shared_cache_contains_path(path: &str) -> Result { - let c_path = alloc::ffi::CString::new(path)?; + let c_path = CString::new(path)?; unsafe extern "C" { fn _dyld_shared_cache_contains_path(path: *const c_char) -> bool; @@ -523,21 +525,24 @@ pub fn encode_wtf8_to_wchar_padded(s: &Wtf8, size: usize) -> Vec { wchar_bytes } -pub fn wchar_null_terminated_bytes(s: &Wtf8) -> Vec { - let wchars: Vec = s - .code_points() - .map(|cp| cp.to_u32() as WChar) - .chain(core::iter::once(0)) - .collect(); - vec_into_bytes(wchars) +/// Encode `s` into a NUL terminated wide string without any interior NULs. +/// +/// The result of this function is suitable for Windows FFI. +pub fn wchar_nul_terminated_bytes(s: &Wtf8) -> Result, InteriorNulError> { + let v: Vec = s.encode_wide_ffi().collect::>()?; + // SAFETY: u16 is POD. + Ok(unsafe { vec_into_bytes(v) }) } -pub fn vec_into_bytes(vec: Vec) -> Vec { - let len = vec.len() * core::mem::size_of::(); - let cap = vec.capacity() * core::mem::size_of::(); - let ptr = vec.as_ptr() as *mut u8; - core::mem::forget(vec); - unsafe { Vec::from_raw_parts(ptr, len, cap) } +// Reinterpret Vec into Vec. +// +// # Safety: +// T must be a plain old data type. No pointers. No destructors. If this function ever needs to be +// public, switch to using bytemuck instead. +unsafe fn vec_into_bytes(vec: Vec) -> Vec { + assert!(size_of::() != 0); + let (ptr, len, cap) = vec.into_raw_parts(); + unsafe { Vec::from_raw_parts(ptr.cast::(), len * size_of::(), cap * size_of::()) } } pub enum IntegerValue { @@ -1122,14 +1127,16 @@ pub fn simple_storage_value_to_bytes_endian( } } -pub fn utf16z_bytes(s: &Wtf8) -> Vec { - vec_into_bytes::(s.encode_wide().chain(core::iter::once(0)).collect()) +#[inline] +pub fn utf16z_bytes(s: &Wtf8) -> Result, InteriorNulError> { + let v: Vec = s.encode_wide_ffi().collect::>()?; + // SAFETY: u16 is POD. + Ok(unsafe { vec_into_bytes::(v) }) } -pub fn null_terminated_bytes(bytes: &[u8]) -> Vec { - let mut buffer = bytes.to_vec(); - buffer.push(0); - buffer +#[inline] +pub fn null_terminated_bytes(bytes: &[u8]) -> Result, alloc::ffi::NulError> { + CString::new(bytes).map(CString::into_bytes_with_nul) } pub fn decode_type_code(type_code: &str, bytes: &[u8]) -> DecodedValue { diff --git a/crates/host_env/src/fileutils.rs b/crates/host_env/src/fileutils.rs index c9e366e3dd5..38435c72629 100644 --- a/crates/host_env/src/fileutils.rs +++ b/crates/host_env/src/fileutils.rs @@ -27,6 +27,7 @@ pub mod windows { use alloc::ffi::CString; use libc::{S_IFCHR, S_IFDIR, S_IFMT}; use std::ffi::{OsStr, OsString}; + use std::io; use std::os::windows::io::AsRawHandle; use std::sync::OnceLock; use windows_sys::Win32::Foundation::{ @@ -72,10 +73,14 @@ pub mod windows { impl StatStruct { // update_st_mode_from_path in cpython - pub fn update_st_mode_from_path(&mut self, path: &OsStr, attr: u32) { + pub fn update_st_mode_from_path( + &mut self, + path: &OsStr, + attr: u32, + ) -> Result<(), io::Error> { if attr & FILE_ATTRIBUTE_DIRECTORY == 0 { let file_extension = path - .to_wide() + .to_wide()? .split(|&c| c == '.' as u16) .next_back() .and_then(|s| String::from_utf16(s).ok()); diff --git a/crates/host_env/src/windows.rs b/crates/host_env/src/windows.rs index bde8d679737..92c1c6af8aa 100644 --- a/crates/host_env/src/windows.rs +++ b/crates/host_env/src/windows.rs @@ -2,8 +2,8 @@ use rustpython_wtf8::Wtf8; use std::{ ffi::{OsStr, OsString}, io, - os::windows::ffi::{OsStrExt, OsStringExt}, }; +use widestring::{WideCString, error::NulError}; use windows_sys::Win32::{ Foundation::{ E_POINTER, ERROR_INSUFFICIENT_BUFFER, ERROR_INVALID_FLAGS, ERROR_NO_UNICODE_TRANSLATION, @@ -394,54 +394,42 @@ pub fn multi_byte_to_wide( } } +/// [`OsStr`] to [`WideCString`] for Windows FFI. +/// +/// Prefer using this trait when encoding bytes to pass to Windows. Interior NULs are memory safe +/// but possibly a security hazard for FFI. +/// +/// https://github.com/python/cpython/issues/111656 pub trait ToWideString { - fn to_wide(&self) -> Vec; - fn to_wide_with_nul(&self) -> Vec; - fn to_wide_cstring(&self) -> widestring::WideCString { - widestring::WideCString::from_vec_truncate(self.to_wide()) - } + fn to_wide(&self) -> Result, io::Error>; + fn to_wide_with_nul(&self) -> Result, io::Error>; + fn to_wide_cstring(&self) -> Result; } impl ToWideString for T where T: AsRef, { - fn to_wide(&self) -> Vec { - self.as_ref().encode_wide().collect() + fn to_wide(&self) -> Result, io::Error> { + WideCString::from_os_str(self) + .map(WideCString::into_vec) + .map_err(io::Error) } - fn to_wide_with_nul(&self) -> Vec { - self.as_ref().encode_wide().chain(Some(0)).collect() + fn to_wide_with_nul(&self) -> Result, io::Error> { + WideCString::from_os_str(self) + .map(WideCString::into_vec_with_nul) + .map_err(io::Error::other) } -} - -impl ToWideString for OsStr { - fn to_wide(&self) -> Vec { - self.encode_wide().collect() - } - fn to_wide_with_nul(&self) -> Vec { - self.encode_wide().chain(Some(0)).collect() + fn to_wide_cstring(&self) -> Result { + WideCString::from_os_str(self).map_err(io::Error::other) } } impl ToWideString for Wtf8 { - fn to_wide(&self) -> Vec { + fn to_wide(&self) -> Result, io::Error> { self.encode_wide().collect() } - fn to_wide_with_nul(&self) -> Vec { + fn to_wide_with_nul(&self) -> Result, io::Error> { self.encode_wide().chain(Some(0)).collect() } } - -pub trait FromWideString -where - Self: Sized, -{ - fn from_wides_until_nul(wide: &[u16]) -> Self; -} - -impl FromWideString for OsString { - fn from_wides_until_nul(wide: &[u16]) -> Self { - let len = wide.iter().take_while(|&&c| c != 0).count(); - Self::from_wide(&wide[..len]) - } -} diff --git a/crates/vm/src/stdlib/_ctypes/base.rs b/crates/vm/src/stdlib/_ctypes/base.rs index 62856c4cef8..9668211929e 100644 --- a/crates/vm/src/stdlib/_ctypes/base.rs +++ b/crates/vm/src/stdlib/_ctypes/base.rs @@ -16,7 +16,7 @@ use core::fmt::Debug; use crossbeam_utils::atomic::AtomicCell; use num_traits::{Signed, ToPrimitive}; use rustpython_common::lock::PyRwLock; -use rustpython_common::wtf8::Wtf8; +use rustpython_common::wtf8::{InteriorNulError, Wtf8}; use rustpython_host_env::ctypes::{ CTypeParamKind, FfiArg, FfiType, FfiValue, char_array_assignment_bytes, char_array_field_value, ffi_arg_from_value, ffi_type_for_layout, wchar_array_field_value, write_cow_bytes_at_offset, @@ -365,19 +365,22 @@ pub(super) static CDATA_BUFFER_METHODS: BufferMethods = BufferMethods { pub(super) fn ensure_z_null_terminated( bytes: &PyBytes, vm: &VirtualMachine, -) -> (PyObjectRef, usize) { - let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes()); +) -> Result<(PyObjectRef, usize), alloc::ffi::NulError> { + let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes())?; let ptr = buffer.as_ptr() as usize; let kept_alive: PyObjectRef = vm.ctx.new_bytes(buffer).into(); - (kept_alive, ptr) + Ok((kept_alive, ptr)) } /// Convert str to null-terminated wchar_t buffer. Returns (PyBytes holder, pointer). -pub(super) fn str_to_wchar_bytes(s: &Wtf8, vm: &VirtualMachine) -> (PyObjectRef, usize) { - let bytes = rustpython_host_env::ctypes::wchar_null_terminated_bytes(s); +pub(super) fn str_to_wchar_bytes( + s: &Wtf8, + vm: &VirtualMachine, +) -> Result<(PyObjectRef, usize), InteriorNulError> { + let bytes = rustpython_host_env::ctypes::wchar_nul_terminated_bytes(s)?; let ptr = bytes.as_ptr() as usize; let holder: PyObjectRef = vm.ctx.new_bytes(bytes).into(); - (holder, ptr) + Ok((holder, ptr)) } /// PyCData - base type for all ctypes data types @@ -957,7 +960,7 @@ impl PyCData { if field_type_code.as_deref() == Some("z") && let Some(bytes_val) = value.downcast_ref::() { - let (kept_alive, ptr) = ensure_z_null_terminated(bytes_val, vm); + let (kept_alive, ptr) = ensure_z_null_terminated(bytes_val, vm)?; let result = rustpython_host_env::ctypes::pointer_to_sized_bytes_endian(ptr, size, needs_swap); self.write_bytes_at_offset(offset, &result); diff --git a/crates/vm/src/stdlib/_ctypes/function.rs b/crates/vm/src/stdlib/_ctypes/function.rs index 2cf3eda13e1..96f882505f3 100644 --- a/crates/vm/src/stdlib/_ctypes/function.rs +++ b/crates/vm/src/stdlib/_ctypes/function.rs @@ -155,7 +155,7 @@ fn conv_param(value: &PyObject, vm: &VirtualMachine) -> PyResult { // 4. Python str -> wide string pointer (like PyUnicode_AsWideCharString) if let Some(s) = value.downcast_ref::() { - let wide_bytes = rustpython_host_env::ctypes::utf16z_bytes(s.as_wtf8()); + let wide_bytes = rustpython_host_env::ctypes::utf16z_bytes(s.as_wtf8())?; let keep = vm.ctx.new_bytes(wide_bytes); let addr = keep.as_bytes().as_ptr() as usize; return Ok(Argument { @@ -168,7 +168,7 @@ fn conv_param(value: &PyObject, vm: &VirtualMachine) -> PyResult { // 9. Python bytes -> null-terminated buffer pointer // Need to ensure null termination like c_char_p if let Some(bytes) = value.downcast_ref::() { - let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes()); + let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes())?; let keep = vm.ctx.new_bytes(buffer); let addr = keep.as_bytes().as_ptr() as usize; return Ok(Argument { diff --git a/crates/wtf8/src/lib.rs b/crates/wtf8/src/lib.rs index 772a2879944..4a80467afc1 100644 --- a/crates/wtf8/src/lib.rs +++ b/crates/wtf8/src/lib.rs @@ -912,6 +912,15 @@ impl Wtf8 { } } + /// Converts the WTF-8 string to potentially ill-formed UTF-8 while checking for interior + /// NULs. + pub fn encode_wide_ffi(&self) -> EncodeWideForFfi<'_> { + EncodeWideForFfi { + iter: self.encode_wide(), + complete: false, + } + } + pub const fn chunks(&self) -> Wtf8Chunks<'_> { Wtf8Chunks { wtf8: self } } @@ -1494,6 +1503,60 @@ impl Iterator for EncodeWide<'_> { impl FusedIterator for EncodeWide<'_> {} +pub struct EncodeWideForFfi<'s> { + iter: EncodeWide<'s>, + complete: bool, +} + +impl Iterator for EncodeWideForFfi<'_> { + type Item = Result; + + fn next(&mut self) -> Option { + if self.complete { + return None; + } + + match self.iter.next() { + Some(v) if v != 0 => Some(Ok(v)), + Some(v) => { + core::hint::cold_path(); + + // Scan if the rest of the buffer is NUL. This is technically incorrect as any + // interior NUL should fail, but this implementation is defensive in case NULs + // are filled in anywhere else. + while let Some(next) = self.iter.next() { + if next != 0 { + self.complete = true; + return Some(Err(InteriorNulError)); + } + } + + // If there aren't any valid u16s past the current NUL, then the original + // buffer is NUL capped and therefore valid. + self.complete = true; + Some(Ok(0)) + } + None => { + self.complete = true; + Some(Ok(0)) + } + } + } +} + +impl FusedIterator for EncodeWideForFfi<'_> {} + +#[derive(Debug)] +pub struct InteriorNulError; + +impl core::error::Error for InteriorNulError {} + +impl fmt::Display for InteriorNulError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "interior NULs are invalid for FFI") + } +} + pub struct Wtf8Chunks<'a> { wtf8: &'a Wtf8, }