Skip to content

Commit b3816bb

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 492e41c commit b3816bb

16 files changed

Lines changed: 221 additions & 115 deletions

File tree

‎crates/host_env/src/ctypes.rs‎

Lines changed: 6 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use alloc::borrow::Cow;
1+
use alloc::{borrow::Cow, ffi::CString};
22
use core::ffi::{
33
CStr, c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar, c_uint,
44
c_ulong, c_ulonglong, c_ushort, c_void,
@@ -32,8 +32,7 @@ use libloading::Library;
3232
use libloading::os::unix::Library as UnixLibrary;
3333
#[cfg(any(unix, windows))]
3434
use parking_lot::{Mutex, RwLock};
35-
use rustpython_wtf8::Wtf8;
36-
use rustpython_wtf8::Wtf8Buf;
35+
use rustpython_wtf8::{Wtf8, Wtf8Buf};
3736
#[cfg(any(unix, windows))]
3837
use std::{collections::HashMap, ffi::OsStr, sync::OnceLock};
3938

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

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

388387
unsafe extern "C" {
389388
fn _dyld_shared_cache_contains_path(path: *const c_char) -> bool;
@@ -523,23 +522,6 @@ pub fn encode_wtf8_to_wchar_padded(s: &Wtf8, size: usize) -> Vec<u8> {
523522
wchar_bytes
524523
}
525524

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)
533-
}
534-
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) }
541-
}
542-
543525
pub enum IntegerValue {
544526
Signed(i64),
545527
Unsigned(u64),
@@ -1107,14 +1089,9 @@ pub fn simple_storage_value_to_bytes_endian(
11071089
}
11081090
}
11091091

1110-
pub fn utf16z_bytes(s: &Wtf8) -> Vec<u8> {
1111-
vec_into_bytes::<u16>(s.encode_wide().chain(core::iter::once(0)).collect())
1112-
}
1113-
1114-
pub fn null_terminated_bytes(bytes: &[u8]) -> Vec<u8> {
1115-
let mut buffer = bytes.to_vec();
1116-
buffer.push(0);
1117-
buffer
1092+
#[inline]
1093+
pub fn null_terminated_bytes(bytes: &[u8]) -> Result<Vec<u8>, alloc::ffi::NulError> {
1094+
CString::new(bytes).map(CString::into_bytes_with_nul)
11181095
}
11191096

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

‎crates/host_env/src/fileutils.rs‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ 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;
32+
use std::path::Path;
3133
use std::sync::OnceLock;
3234
use windows_sys::Win32::Foundation::{
3335
ERROR_INVALID_HANDLE, ERROR_NOT_SUPPORTED, FILETIME, FreeLibrary, SetLastError,
@@ -74,13 +76,8 @@ pub mod windows {
7476
// update_st_mode_from_path in cpython
7577
pub fn update_st_mode_from_path(&mut self, path: &OsStr, attr: u32) {
7678
if attr & FILE_ATTRIBUTE_DIRECTORY == 0 {
77-
let file_extension = path
78-
.to_wide()
79-
.split(|&c| c == '.' as u16)
80-
.next_back()
81-
.and_then(|s| String::from_utf16(s).ok());
82-
83-
if let Some(file_extension) = file_extension
79+
if let Some(file_extension) =
80+
Path::new(path).extension().and_then(|ext| ext.to_str())
8481
&& (file_extension.eq_ignore_ascii_case("exe")
8582
|| file_extension.eq_ignore_ascii_case("bat")
8683
|| file_extension.eq_ignore_ascii_case("cmd")

‎crates/host_env/src/nt.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1676,6 +1676,7 @@ pub fn getppid() -> u32 {
16761676

16771677
pub fn path_skip_root(path: &widestring::WideCStr) -> Option<usize> {
16781678
let mut end: *const u16 = core::ptr::null();
1679+
// SAFETY: `path` is a valid pointer to a nul terminated wide string without interior nuls.
16791680
let hr = unsafe { windows_sys::Win32::UI::Shell::PathCchSkipRoot(path.as_ptr(), &mut end) };
16801681
if hr >= 0 {
16811682
assert!(!end.is_null());

‎crates/host_env/src/overlapped.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,6 +1028,7 @@ pub fn parse_address_v4_wide(host_wide: &[u16], port: u16) -> io::Result<(Vec<u8
10281028

10291029
let mut addr_len = core::mem::size_of::<SOCKADDR_IN>() as i32;
10301030

1031+
// SAFETY: host_wide is nul capped and doesn't have interior nuls
10311032
let ret = unsafe {
10321033
WSAStringToAddressW(
10331034
host_wide.as_ptr(),
@@ -1056,7 +1057,10 @@ pub fn parse_address_v4_wide(host_wide: &[u16], port: u16) -> io::Result<(Vec<u8
10561057
}
10571058

10581059
pub fn parse_address_v4(host: &str, port: u16) -> io::Result<(Vec<u8>, i32)> {
1059-
let host_wide: Vec<u16> = host.encode_utf16().chain([0]).collect();
1060+
let host_wide: Vec<u16> = Wtf8::new(host)
1061+
.encode_wide_ffi()
1062+
.collect::<Result<_, _>>()
1063+
.map_err(io::Error::other)?;
10601064
parse_address_v4_wide(&host_wide, port)
10611065
}
10621066

@@ -1066,7 +1070,10 @@ pub fn parse_address_v6(
10661070
flowinfo: u32,
10671071
scope_id: u32,
10681072
) -> io::Result<(Vec<u8>, i32)> {
1069-
let host_wide: Vec<u16> = host.encode_utf16().chain([0]).collect();
1073+
let host_wide: Vec<u16> = Wtf8::new(host)
1074+
.encode_wide_ffi()
1075+
.collect::<Result<_, _>>()
1076+
.map_err(io::Error::other)?;
10701077
parse_address_v6_wide(&host_wide, port, flowinfo, scope_id)
10711078
}
10721079

@@ -1083,6 +1090,7 @@ pub fn parse_address_v6_wide(
10831090

10841091
let mut addr_len = core::mem::size_of::<SOCKADDR_IN6>() as i32;
10851092

1093+
// SAFETY: host_wide is nul capped and doesn't have interior nuls
10861094
let ret = unsafe {
10871095
WSAStringToAddressW(
10881096
host_wide.as_ptr(),

‎crates/host_env/src/winapi.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1140,6 +1140,12 @@ pub fn lc_map_string_ex(
11401140
src: &[u16],
11411141
) -> io::Result<Vec<u16>> {
11421142
let src_len = src.len() as i32;
1143+
// SAFETY:
1144+
// * locale does not have interior NULs and ends with a NUL. This is guaranteed by
1145+
// WideCStr.
1146+
// * src CAN have interior NULs and DOES NOT need to end with a NUL. However, the length must be
1147+
// passed into LCMapStringEx. If the length is NOT passed in, Windows calculates the length
1148+
// and interior NULs are not allowed.
11431149
let dest_size = unsafe {
11441150
windows_sys::Win32::Globalization::LCMapStringEx(
11451151
locale.as_ptr(),

‎crates/host_env/src/windows.rs‎

Lines changed: 19 additions & 37 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,36 @@ 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_with_nul(&self) -> Result<Vec<u16>, io::Error>;
405+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error>;
403406
}
404407

405408
impl<T> ToWideString for T
406409
where
407410
T: AsRef<OsStr>,
408411
{
409-
fn to_wide(&self) -> Vec<u16> {
410-
self.as_ref().encode_wide().collect()
411-
}
412-
fn to_wide_with_nul(&self) -> Vec<u16> {
413-
self.as_ref().encode_wide().chain(Some(0)).collect()
414-
}
415-
}
416-
417-
impl ToWideString for OsStr {
418-
fn to_wide(&self) -> Vec<u16> {
419-
self.encode_wide().collect()
412+
fn to_wide_with_nul(&self) -> Result<Vec<u16>, io::Error> {
413+
WideCString::from_os_str(self)
414+
.map(WideCString::into_vec_with_nul)
415+
.map_err(io::Error::other)
420416
}
421-
fn to_wide_with_nul(&self) -> Vec<u16> {
422-
self.encode_wide().chain(Some(0)).collect()
417+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error> {
418+
WideCString::from_os_str(self).map_err(io::Error::other)
423419
}
424420
}
425421

426422
impl ToWideString for Wtf8 {
427-
fn to_wide(&self) -> Vec<u16> {
428-
self.encode_wide().collect()
429-
}
430-
fn to_wide_with_nul(&self) -> Vec<u16> {
431-
self.encode_wide().chain(Some(0)).collect()
423+
fn to_wide_with_nul(&self) -> Result<Vec<u16>, io::Error> {
424+
self.encode_wide_ffi().collect().map_err(io::Error::other)
432425
}
433-
}
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])
426+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error> {
427+
WideCString::from_
446428
}
447429
}

‎crates/stdlib/src/overlapped.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,8 @@ mod _overlapped {
210210
// IPv4: (host, port)
211211
let host: PyStrRef = addr_obj[0].clone().try_into_value(vm)?;
212212
let port: u16 = addr_obj[1].clone().try_to_value(vm)?;
213-
let host_wide: Vec<u16> = host.as_wtf8().encode_wide().chain([0]).collect();
213+
let host_wide: Vec<u16> =
214+
host.as_wtf8().encode_wide_ffi().collect::<Result<_, _>>()?;
214215
host_overlapped::parse_address_v4_wide(&host_wide, port)
215216
.map_err(|err| set_from_windows_err(err.raw_os_error().unwrap_or(0) as u32, vm))
216217
}
@@ -220,7 +221,8 @@ mod _overlapped {
220221
let port: u16 = addr_obj[1].clone().try_to_value(vm)?;
221222
let flowinfo: u32 = addr_obj[2].clone().try_to_value(vm)?;
222223
let scope_id: u32 = addr_obj[3].clone().try_to_value(vm)?;
223-
let host_wide: Vec<u16> = host.as_wtf8().encode_wide().chain([0]).collect();
224+
let host_wide: Vec<u16> =
225+
host.as_wtf8().encode_wide_ffi().collect::<Result<_, _>>()?;
224226
host_overlapped::parse_address_v6_wide(&host_wide, port, flowinfo, scope_id)
225227
.map_err(|err| set_from_windows_err(err.raw_os_error().unwrap_or(0) as u32, vm))
226228
}

‎crates/vm/src/exceptions.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1215,6 +1215,12 @@ impl ToPyException for alloc::ffi::NulError {
12151215
}
12161216
}
12171217

1218+
impl ToPyException for rustpython_common::wtf8::InteriorNulError {
1219+
fn to_pyexception(&self, vm: &VirtualMachine) -> PyBaseExceptionRef {
1220+
cstring_error(vm)
1221+
}
1222+
}
1223+
12181224
#[cfg(windows)]
12191225
impl<C> ToPyException for widestring::error::ContainsNul<C> {
12201226
fn to_pyexception(&self, vm: &VirtualMachine) -> PyBaseExceptionRef {

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use super::StgInfo;
22
use super::base::{CDATA_BUFFER_METHODS, PyCData};
33
use crate::common::lock::LazyLock;
4+
use crate::convert::ToPyException;
45
use crate::sliceable::SaturatedSliceIter;
56
use crate::{
67
AsObject, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject, VirtualMachine,
@@ -638,7 +639,8 @@ impl PyCArray {
638639
let (ptr_val, converted) = if value.is(&vm.ctx.none) {
639640
(0usize, None)
640641
} else if let Some(bytes) = value.downcast_ref::<PyBytes>() {
641-
let (kept_alive, ptr) = super::base::ensure_z_null_terminated(bytes, vm);
642+
let (kept_alive, ptr) = super::base::ensure_z_null_terminated(bytes, vm)
643+
.map_err(|e| e.to_pyexception(vm))?;
642644
zelf.0.keep_alive(index, kept_alive);
643645
(ptr, Some(value.to_owned()))
644646
} else if let Ok(int_val) = value.try_index(vm) {
@@ -665,7 +667,8 @@ impl PyCArray {
665667
let (ptr_val, converted) = if value.is(&vm.ctx.none) {
666668
(0usize, None)
667669
} else if let Some(s) = value.downcast_ref::<PyStr>() {
668-
let (holder, ptr) = super::base::str_to_wchar_bytes(s.as_wtf8(), vm);
670+
let (holder, ptr) = super::base::str_to_wchar_bytes(s.as_wtf8(), vm)
671+
.map_err(|e| e.to_pyexception(vm))?;
669672
(ptr, Some(holder))
670673
} else if let Ok(int_val) = value.try_index(vm) {
671674
(int_val.as_bigint().to_usize().unwrap_or(0), None)

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

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use crate::builtins::{
44
PyBytes, PyDict, PyList, PyMemoryView, PyStr, PyTuple, PyType, PyTypeRef, PyUtf8Str,
55
};
66
use crate::class::StaticType;
7-
use crate::convert::ToPyObject;
7+
use crate::convert::{ToPyException, ToPyObject};
88
use crate::function::{ArgBytesLike, OptionalArg, PySetterValue};
99
use crate::protocol::{BufferMethods, PyBuffer};
1010
use crate::types::{Constructor, GetDescriptor, Representable};
@@ -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
CTypeLayout, char_array_assignment_bytes, char_array_field_value, wchar_array_field_value,
2222
write_cow_bytes_at_offset,
@@ -380,24 +380,32 @@ pub(super) static CDATA_BUFFER_METHODS: BufferMethods = BufferMethods {
380380
retain: |_| {},
381381
};
382382

383-
/// Ensure PyBytes data is null-terminated. Returns (kept_alive_obj, pointer).
384-
/// The caller must keep the returned object alive to keep the pointer valid.
383+
/// Ensure PyBytes data is null-terminated without interior nulls.
384+
///
385+
/// Returns (kept_alive_obj, pointer). The caller must keep the returned object alive to keep
386+
/// the pointer valid.
385387
pub(super) fn ensure_z_null_terminated(
386388
bytes: &PyBytes,
387389
vm: &VirtualMachine,
388-
) -> (PyObjectRef, usize) {
389-
let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes());
390+
) -> Result<(PyObjectRef, usize), InteriorNulError> {
391+
let buffer = rustpython_host_env::ctypes::null_terminated_bytes(bytes.as_bytes())
392+
.map_err(|_| InteriorNulError)?;
390393
let ptr = buffer.as_ptr() as usize;
391394
let kept_alive: PyObjectRef = vm.ctx.new_bytes(buffer).into();
392-
(kept_alive, ptr)
395+
Ok((kept_alive, ptr))
393396
}
394397

395398
/// Convert str to null-terminated wchar_t buffer. Returns (PyBytes holder, pointer).
396-
pub(super) fn str_to_wchar_bytes(s: &Wtf8, vm: &VirtualMachine) -> (PyObjectRef, usize) {
397-
let bytes = rustpython_host_env::ctypes::wchar_null_terminated_bytes(s);
399+
pub(super) fn str_to_wchar_bytes(
400+
s: &Wtf8,
401+
vm: &VirtualMachine,
402+
) -> Result<(PyObjectRef, usize), InteriorNulError> {
403+
let bytes = s
404+
.encode_wide_to_bytes_ffi()
405+
.collect::<Result<Vec<_>, _>>()?;
398406
let ptr = bytes.as_ptr() as usize;
399407
let holder: PyObjectRef = vm.ctx.new_bytes(bytes).into();
400-
(holder, ptr)
408+
Ok((holder, ptr))
401409
}
402410

403411
/// PyCData - base type for all ctypes data types
@@ -977,7 +985,8 @@ impl PyCData {
977985
if field_type_code.as_deref() == Some("z")
978986
&& let Some(bytes_val) = value.downcast_ref::<PyBytes>()
979987
{
980-
let (kept_alive, ptr) = ensure_z_null_terminated(bytes_val, vm);
988+
let (kept_alive, ptr) =
989+
ensure_z_null_terminated(bytes_val, vm).map_err(|e| e.to_pyexception(vm))?;
981990
let result =
982991
rustpython_host_env::ctypes::pointer_to_sized_bytes_endian(ptr, size, needs_swap);
983992
self.write_bytes_at_offset(offset, &result);
@@ -1610,7 +1619,8 @@ impl PyCField {
16101619
"Z" => {
16111620
// c_wchar_p: store pointer to null-terminated wchar_t buffer
16121621
if let Some(s) = value.downcast_ref::<PyStr>() {
1613-
let (holder, ptr) = str_to_wchar_bytes(s.as_wtf8(), vm);
1622+
let (holder, ptr) =
1623+
str_to_wchar_bytes(s.as_wtf8(), vm).map_err(|e| e.to_pyexception(vm))?;
16141624
return Ok((
16151625
rustpython_host_env::ctypes::pointer_to_sized_bytes(ptr, size),
16161626
Some(holder),

0 commit comments

Comments
 (0)