Skip to content

Commit ec9ad96

Browse files
joshuamegnauth54joshuamegnauth54
committed
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
1 parent 045a6d5 commit ec9ad96

6 files changed

Lines changed: 133 additions & 67 deletions

File tree

‎crates/host_env/src/ctypes.rs‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
use alloc::borrow::Cow;
2+
#[cfg(unix)]
3+
use alloc::ffi::CString;
24
use core::ffi::{
35
CStr, c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar, c_uint,
46
c_ulong, c_ulonglong, c_ushort, c_void,
@@ -32,8 +34,8 @@ use libloading::Library;
3234
use libloading::os::unix::Library as UnixLibrary;
3335
#[cfg(any(unix, windows))]
3436
use parking_lot::{Mutex, RwLock};
35-
use rustpython_wtf8::Wtf8;
3637
use rustpython_wtf8::Wtf8Buf;
38+
use rustpython_wtf8::{InteriorNulError, Wtf8};
3739
#[cfg(any(unix, windows))]
3840
use std::{collections::HashMap, ffi::OsStr, sync::OnceLock};
3941

@@ -383,7 +385,7 @@ pub fn dlopen_mode(load_flags: Option<i32>) -> i32 {
383385

384386
#[cfg(target_os = "macos")]
385387
pub fn dyld_shared_cache_contains_path(path: &str) -> Result<bool, alloc::ffi::NulError> {
386-
let c_path = alloc::ffi::CString::new(path)?;
388+
let c_path = CString::new(path)?;
387389

388390
unsafe extern "C" {
389391
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<u8> {
523525
wchar_bytes
524526
}
525527

526-
pub fn wchar_null_terminated_bytes(s: &Wtf8) -> Vec<u8> {
527-
let wchars: Vec<WChar> = s
528-
.code_points()
529-
.map(|cp| cp.to_u32() as WChar)
530-
.chain(core::iter::once(0))
531-
.collect();
532-
vec_into_bytes(wchars)
528+
/// Encode `s` into a NUL terminated wide string without any interior NULs.
529+
///
530+
/// The result of this function is suitable for Windows FFI.
531+
pub fn wchar_nul_terminated_bytes(s: &Wtf8) -> Result<Vec<u8>, InteriorNulError> {
532+
let v: Vec<u16> = s.encode_wide_ffi().collect::<Result<_, _>>()?;
533+
// SAFETY: u16 is POD.
534+
Ok(unsafe { vec_into_bytes(v) })
533535
}
534536

535-
pub fn vec_into_bytes<T>(vec: Vec<T>) -> Vec<u8> {
536-
let len = vec.len() * core::mem::size_of::<T>();
537-
let cap = vec.capacity() * core::mem::size_of::<T>();
538-
let ptr = vec.as_ptr() as *mut u8;
539-
core::mem::forget(vec);
540-
unsafe { Vec::from_raw_parts(ptr, len, cap) }
537+
// Reinterpret Vec<T> into Vec<u8>.
538+
//
539+
// # Safety:
540+
// T must be a plain old data type. No pointers. No destructors. If this function ever needs to be
541+
// public, switch to using bytemuck instead.
542+
unsafe fn vec_into_bytes<T: Copy>(vec: Vec<T>) -> Vec<u8> {
543+
assert!(size_of::<T>() != 0);
544+
let (ptr, len, cap) = vec.into_raw_parts();
545+
unsafe { Vec::from_raw_parts(ptr.cast::<u8>(), len * size_of::<T>(), cap * size_of::<T>()) }
541546
}
542547

543548
pub enum IntegerValue {
@@ -1122,14 +1127,16 @@ pub fn simple_storage_value_to_bytes_endian(
11221127
}
11231128
}
11241129

1125-
pub fn utf16z_bytes(s: &Wtf8) -> Vec<u8> {
1126-
vec_into_bytes::<u16>(s.encode_wide().chain(core::iter::once(0)).collect())
1130+
#[inline]
1131+
pub fn utf16z_bytes(s: &Wtf8) -> Result<Vec<u8>, InteriorNulError> {
1132+
let v: Vec<u16> = s.encode_wide_ffi().collect::<Result<_, _>>()?;
1133+
// SAFETY: u16 is POD.
1134+
Ok(unsafe { vec_into_bytes::<u16>(v) })
11271135
}
11281136

1129-
pub fn null_terminated_bytes(bytes: &[u8]) -> Vec<u8> {
1130-
let mut buffer = bytes.to_vec();
1131-
buffer.push(0);
1132-
buffer
1137+
#[inline]
1138+
pub fn null_terminated_bytes(bytes: &[u8]) -> Result<Vec<u8>, alloc::ffi::NulError> {
1139+
CString::new(bytes).map(CString::into_bytes_with_nul)
11331140
}
11341141

11351142
pub fn decode_type_code(type_code: &str, bytes: &[u8]) -> DecodedValue {

‎crates/host_env/src/fileutils.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ pub mod windows {
2727
use alloc::ffi::CString;
2828
use libc::{S_IFCHR, S_IFDIR, S_IFMT};
2929
use std::ffi::{OsStr, OsString};
30+
use std::io;
3031
use std::os::windows::io::AsRawHandle;
3132
use std::sync::OnceLock;
3233
use windows_sys::Win32::Foundation::{
@@ -72,10 +73,14 @@ pub mod windows {
7273

7374
impl StatStruct {
7475
// update_st_mode_from_path in cpython
75-
pub fn update_st_mode_from_path(&mut self, path: &OsStr, attr: u32) {
76+
pub fn update_st_mode_from_path(
77+
&mut self,
78+
path: &OsStr,
79+
attr: u32,
80+
) -> Result<(), io::Error> {
7681
if attr & FILE_ATTRIBUTE_DIRECTORY == 0 {
7782
let file_extension = path
78-
.to_wide()
83+
.to_wide()?
7984
.split(|&c| c == '.' as u16)
8085
.next_back()
8186
.and_then(|s| String::from_utf16(s).ok());

‎crates/host_env/src/windows.rs‎

Lines changed: 22 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ use rustpython_wtf8::Wtf8;
22
use std::{
33
ffi::{OsStr, OsString},
44
io,
5-
os::windows::ffi::{OsStrExt, OsStringExt},
65
};
6+
use widestring::{WideCString, error::NulError};
77
use windows_sys::Win32::{
88
Foundation::{
99
E_POINTER, ERROR_INSUFFICIENT_BUFFER, ERROR_INVALID_FLAGS, ERROR_NO_UNICODE_TRANSLATION,
@@ -394,54 +394,42 @@ pub fn multi_byte_to_wide(
394394
}
395395
}
396396

397+
/// [`OsStr`] to [`WideCString`] for Windows FFI.
398+
///
399+
/// Prefer using this trait when encoding bytes to pass to Windows. Interior NULs are memory safe
400+
/// but possibly a security hazard for FFI.
401+
///
402+
/// https://github.com/python/cpython/issues/111656
397403
pub trait ToWideString {
398-
fn to_wide(&self) -> Vec<u16>;
399-
fn to_wide_with_nul(&self) -> Vec<u16>;
400-
fn to_wide_cstring(&self) -> widestring::WideCString {
401-
widestring::WideCString::from_vec_truncate(self.to_wide())
402-
}
404+
fn to_wide(&self) -> Result<Vec<u16>, io::Error>;
405+
fn to_wide_with_nul(&self) -> Result<Vec<u16>, io::Error>;
406+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error>;
403407
}
404408

405409
impl<T> ToWideString for T
406410
where
407411
T: AsRef<OsStr>,
408412
{
409-
fn to_wide(&self) -> Vec<u16> {
410-
self.as_ref().encode_wide().collect()
413+
fn to_wide(&self) -> Result<Vec<u16>, io::Error> {
414+
WideCString::from_os_str(self)
415+
.map(WideCString::into_vec)
416+
.map_err(io::Error)
411417
}
412-
fn to_wide_with_nul(&self) -> Vec<u16> {
413-
self.as_ref().encode_wide().chain(Some(0)).collect()
418+
fn to_wide_with_nul(&self) -> Result<Vec<u16>, io::Error> {
419+
WideCString::from_os_str(self)
420+
.map(WideCString::into_vec_with_nul)
421+
.map_err(io::Error::other)
414422
}
415-
}
416-
417-
impl ToWideString for OsStr {
418-
fn to_wide(&self) -> Vec<u16> {
419-
self.encode_wide().collect()
420-
}
421-
fn to_wide_with_nul(&self) -> Vec<u16> {
422-
self.encode_wide().chain(Some(0)).collect()
423+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error> {
424+
WideCString::from_os_str(self).map_err(io::Error::other)
423425
}
424426
}
425427

426428
impl ToWideString for Wtf8 {
427-
fn to_wide(&self) -> Vec<u16> {
429+
fn to_wide(&self) -> Result<Vec<u16>, io::Error> {
428430
self.encode_wide().collect()
429431
}
430-
fn to_wide_with_nul(&self) -> Vec<u16> {
432+
fn to_wide_with_nul(&self) -> Result<Vec<u16>, io::Error> {
431433
self.encode_wide().chain(Some(0)).collect()
432434
}
433435
}
434-
435-
pub trait FromWideString
436-
where
437-
Self: Sized,
438-
{
439-
fn from_wides_until_nul(wide: &[u16]) -> Self;
440-
}
441-
442-
impl FromWideString for OsString {
443-
fn from_wides_until_nul(wide: &[u16]) -> Self {
444-
let len = wide.iter().take_while(|&&c| c != 0).count();
445-
Self::from_wide(&wide[..len])
446-
}
447-
}

‎crates/vm/src/stdlib/_ctypes/base.rs‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use core::fmt::Debug;
1616
use crossbeam_utils::atomic::AtomicCell;
1717
use num_traits::{Signed, ToPrimitive};
1818
use rustpython_common::lock::PyRwLock;
19-
use rustpython_common::wtf8::Wtf8;
19+
use rustpython_common::wtf8::{InteriorNulError, Wtf8};
2020
use rustpython_host_env::ctypes::{
2121
CTypeParamKind, FfiArg, FfiType, FfiValue, char_array_assignment_bytes, char_array_field_value,
2222
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 {
365365
pub(super) fn ensure_z_null_terminated(
366366
bytes: &PyBytes,
367367
vm: &VirtualMachine,
368-
) -> (PyObjectRef, usize) {
369-
let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes());
368+
) -> Result<(PyObjectRef, usize), alloc::ffi::NulError> {
369+
let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes())?;
370370
let ptr = buffer.as_ptr() as usize;
371371
let kept_alive: PyObjectRef = vm.ctx.new_bytes(buffer).into();
372-
(kept_alive, ptr)
372+
Ok((kept_alive, ptr))
373373
}
374374

375375
/// Convert str to null-terminated wchar_t buffer. Returns (PyBytes holder, pointer).
376-
pub(super) fn str_to_wchar_bytes(s: &Wtf8, vm: &VirtualMachine) -> (PyObjectRef, usize) {
377-
let bytes = rustpython_host_env::ctypes::wchar_null_terminated_bytes(s);
376+
pub(super) fn str_to_wchar_bytes(
377+
s: &Wtf8,
378+
vm: &VirtualMachine,
379+
) -> Result<(PyObjectRef, usize), InteriorNulError> {
380+
let bytes = rustpython_host_env::ctypes::wchar_nul_terminated_bytes(s)?;
378381
let ptr = bytes.as_ptr() as usize;
379382
let holder: PyObjectRef = vm.ctx.new_bytes(bytes).into();
380-
(holder, ptr)
383+
Ok((holder, ptr))
381384
}
382385

383386
/// PyCData - base type for all ctypes data types
@@ -957,7 +960,7 @@ impl PyCData {
957960
if field_type_code.as_deref() == Some("z")
958961
&& let Some(bytes_val) = value.downcast_ref::<PyBytes>()
959962
{
960-
let (kept_alive, ptr) = ensure_z_null_terminated(bytes_val, vm);
963+
let (kept_alive, ptr) = ensure_z_null_terminated(bytes_val, vm)?;
961964
let result =
962965
rustpython_host_env::ctypes::pointer_to_sized_bytes_endian(ptr, size, needs_swap);
963966
self.write_bytes_at_offset(offset, &result);

‎crates/vm/src/stdlib/_ctypes/function.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ fn conv_param(value: &PyObject, vm: &VirtualMachine) -> PyResult<Argument> {
155155

156156
// 4. Python str -> wide string pointer (like PyUnicode_AsWideCharString)
157157
if let Some(s) = value.downcast_ref::<PyStr>() {
158-
let wide_bytes = rustpython_host_env::ctypes::utf16z_bytes(s.as_wtf8());
158+
let wide_bytes = rustpython_host_env::ctypes::utf16z_bytes(s.as_wtf8())?;
159159
let keep = vm.ctx.new_bytes(wide_bytes);
160160
let addr = keep.as_bytes().as_ptr() as usize;
161161
return Ok(Argument {
@@ -168,7 +168,7 @@ fn conv_param(value: &PyObject, vm: &VirtualMachine) -> PyResult<Argument> {
168168
// 9. Python bytes -> null-terminated buffer pointer
169169
// Need to ensure null termination like c_char_p
170170
if let Some(bytes) = value.downcast_ref::<PyBytes>() {
171-
let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes());
171+
let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes())?;
172172
let keep = vm.ctx.new_bytes(buffer);
173173
let addr = keep.as_bytes().as_ptr() as usize;
174174
return Ok(Argument {

‎crates/wtf8/src/lib.rs‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -912,6 +912,15 @@ impl Wtf8 {
912912
}
913913
}
914914

915+
/// Converts the WTF-8 string to potentially ill-formed UTF-8 while checking for interior
916+
/// NULs.
917+
pub fn encode_wide_ffi(&self) -> EncodeWideForFfi<'_> {
918+
EncodeWideForFfi {
919+
iter: self.encode_wide(),
920+
complete: false,
921+
}
922+
}
923+
915924
pub const fn chunks(&self) -> Wtf8Chunks<'_> {
916925
Wtf8Chunks { wtf8: self }
917926
}
@@ -1494,6 +1503,60 @@ impl Iterator for EncodeWide<'_> {
14941503

14951504
impl FusedIterator for EncodeWide<'_> {}
14961505

1506+
pub struct EncodeWideForFfi<'s> {
1507+
iter: EncodeWide<'s>,
1508+
complete: bool,
1509+
}
1510+
1511+
impl Iterator for EncodeWideForFfi<'_> {
1512+
type Item = Result<u16, InteriorNulError>;
1513+
1514+
fn next(&mut self) -> Option<Self::Item> {
1515+
if self.complete {
1516+
return None;
1517+
}
1518+
1519+
match self.iter.next() {
1520+
Some(v) if v != 0 => Some(Ok(v)),
1521+
Some(v) => {
1522+
core::hint::cold_path();
1523+
1524+
// Scan if the rest of the buffer is NUL. This is technically incorrect as any
1525+
// interior NUL should fail, but this implementation is defensive in case NULs
1526+
// are filled in anywhere else.
1527+
while let Some(next) = self.iter.next() {
1528+
if next != 0 {
1529+
self.complete = true;
1530+
return Some(Err(InteriorNulError));
1531+
}
1532+
}
1533+
1534+
// If there aren't any valid u16s past the current NUL, then the original
1535+
// buffer is NUL capped and therefore valid.
1536+
self.complete = true;
1537+
Some(Ok(0))
1538+
}
1539+
None => {
1540+
self.complete = true;
1541+
Some(Ok(0))
1542+
}
1543+
}
1544+
}
1545+
}
1546+
1547+
impl FusedIterator for EncodeWideForFfi<'_> {}
1548+
1549+
#[derive(Debug)]
1550+
pub struct InteriorNulError;
1551+
1552+
impl core::error::Error for InteriorNulError {}
1553+
1554+
impl fmt::Display for InteriorNulError {
1555+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
1556+
write!(f, "interior NULs are invalid for FFI")
1557+
}
1558+
}
1559+
14971560
pub struct Wtf8Chunks<'a> {
14981561
wtf8: &'a Wtf8,
14991562
}

0 commit comments

Comments
 (0)