Visitar URL original
ffi: No interior NULs (part 1) · RustPython/RustPython@f25dc9f · GitHub
Skip to content

Commit f25dc9f

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 some of 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 with CStrings, so this mostly affects Windows or areas where we have raw bytes that weren't checked by CString. 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. Most of RustPython seems to handle interior NULs already due to CString as well as WideCString. **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:gpt-5.4
1 parent 3290f28 commit f25dc9f

16 files changed

Lines changed: 274 additions & 107 deletions

File tree

‎crates/host_env/src/ctypes.rs‎

Lines changed: 73 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
1-
use alloc::borrow::Cow;
2-
use core::ffi::{
3-
CStr, c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar, c_uint,
4-
c_ulong, c_ulonglong, c_ushort, c_void,
1+
use alloc::{borrow::Cow, ffi::CString};
2+
use core::{
3+
array,
4+
ffi::{
5+
CStr, c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar,
6+
c_uint, c_ulong, c_ulonglong, c_ushort, c_void,
7+
},
8+
iter,
59
};
610
#[cfg(all(
711
any(
@@ -32,8 +36,7 @@ use libloading::Library;
3236
use libloading::os::unix::Library as UnixLibrary;
3337
#[cfg(any(unix, windows))]
3438
use parking_lot::{Mutex, RwLock};
35-
use rustpython_wtf8::Wtf8;
36-
use rustpython_wtf8::Wtf8Buf;
39+
use rustpython_wtf8::{InteriorNulError, Wtf8, Wtf8Buf};
3740
#[cfg(any(unix, windows))]
3841
use std::{collections::HashMap, ffi::OsStr, sync::OnceLock};
3942

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

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

388391
unsafe extern "C" {
389392
fn _dyld_shared_cache_contains_path(path: *const c_char) -> bool;
@@ -537,6 +540,51 @@ pub fn wchar_null_terminated_bytes(s: &Wtf8) -> Vec<u8> {
537540
}
538541
}
539542

543+
/// Encode buffer as a wide string but yield bytes instead of u16.
544+
///
545+
/// The resulting bytes buffer is suitable for FFI. It is NUL capped without interior
546+
/// NULs.
547+
pub fn wchar_ffi_bytes(s: &Wtf8) -> impl Iterator<Item = Result<u8, InteriorNulError>> {
548+
let mut iter = s.code_points().map(|cp| cp.to_u32() as WChar);
549+
let mut pending: Option<array::IntoIter<_, _>> = None;
550+
let mut complete = false;
551+
552+
iter::from_fn(move || {
553+
if let Some(pending) = pending.as_mut()
554+
&& let Some(next) = pending.next()
555+
{
556+
return Some(Ok(next));
557+
}
558+
559+
match iter.next() {
560+
Some(0) => {
561+
core::hint::cold_path();
562+
complete = true;
563+
564+
for next in iter.by_ref() {
565+
if next != 0 {
566+
return Some(Err(InteriorNulError));
567+
}
568+
}
569+
570+
pending = Some((0 as WChar).to_ne_bytes().into_iter());
571+
}
572+
Some(next) => {
573+
pending = Some(next.to_ne_bytes().into_iter());
574+
}
575+
None if !complete => {
576+
complete = true;
577+
pending = Some((0 as WChar).to_ne_bytes().into_iter());
578+
}
579+
None => {
580+
return None;
581+
}
582+
}
583+
584+
pending.as_mut().unwrap().next().map(Result::Ok)
585+
})
586+
}
587+
540588
pub enum IntegerValue {
541589
Signed(i64),
542590
Unsigned(u64),
@@ -1111,10 +1159,25 @@ pub fn utf16z_bytes(s: &Wtf8) -> Vec<u8> {
11111159
.collect()
11121160
}
11131161

1162+
/// Return a NUL terminated copy of `bytes`.
1163+
///
1164+
/// The input may contain interior NULs.
11141165
pub fn null_terminated_bytes(bytes: &[u8]) -> Vec<u8> {
1115-
let mut buffer = bytes.to_vec();
1116-
buffer.push(0);
1117-
buffer
1166+
if bytes.last() == Some(&0) {
1167+
bytes.to_vec()
1168+
} else {
1169+
bytes.iter().copied().chain(Some(0)).collect()
1170+
}
1171+
}
1172+
1173+
/// Return a NUL terminated copy of `bytes` if it doesn't contain interior NULs.
1174+
pub fn c_string_bytes(bytes: &[u8]) -> Result<Vec<u8>, InteriorNulError> {
1175+
if bytes.last() == Some(&0) {
1176+
CString::from_vec_with_nul(bytes.to_vec()).map_err(|_| InteriorNulError)
1177+
} else {
1178+
CString::new(bytes).map_err(|_| InteriorNulError)
1179+
}
1180+
.map(CString::into_bytes_with_nul)
11181181
}
11191182

11201183
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: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -218,12 +218,14 @@ pub enum ReadConsoleError {
218218
}
219219

220220
pub fn access(path: &Path, mode: u8) -> bool {
221-
let wide = path.as_os_str().to_wide_with_nul();
221+
let Ok(wide) = path.as_os_str().to_wide_with_nul() else {
222+
return false;
223+
};
222224
let attr = unsafe { GetFileAttributesW(wide.as_ptr()) };
223-
attr != INVALID_FILE_ATTRIBUTES
225+
Ok(attr != INVALID_FILE_ATTRIBUTES
224226
&& (mode & 2 == 0
225227
|| attr & FILE_ATTRIBUTE_READONLY == 0
226-
|| attr & windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_DIRECTORY != 0)
228+
|| attr & windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_DIRECTORY != 0))
227229
}
228230

229231
pub fn remove(path: &Path) -> io::Result<()> {
@@ -234,7 +236,10 @@ pub fn remove(path: &Path) -> io::Result<()> {
234236
IO_REPARSE_TAG_MOUNT_POINT, IO_REPARSE_TAG_SYMLINK,
235237
};
236238

237-
let wide_path = path.as_os_str().to_wide_with_nul();
239+
let wide_path = path
240+
.as_os_str()
241+
.to_wide_with_nul()
242+
.map_err(io::Error::other)?;
238243
let attrs = unsafe { GetFileAttributesW(wide_path.as_ptr()) };
239244

240245
let mut is_directory = false;
@@ -381,7 +386,7 @@ pub fn fchmod(fd: i32, mode: u32, write_bit: u32) -> io::Result<()> {
381386
}
382387

383388
pub fn win32_lchmod(path: &OsStr, mode: u32, write_bit: u32) -> io::Result<()> {
384-
let wide = path.to_wide_with_nul();
389+
let wide = path.to_wide_with_nul()?;
385390
let attr = unsafe { GetFileAttributesW(wide.as_ptr()) }.check_ne(INVALID_FILE_ATTRIBUTES)?;
386391
let new_attr = if mode & write_bit != 0 {
387392
attr & !FILE_ATTRIBUTE_READONLY
@@ -414,7 +419,7 @@ pub fn chmod_follow(path: &widestring::WideCStr, mode: u32, write_bit: u32) -> i
414419
}
415420

416421
pub fn find_first_file_name(path: &Path) -> io::Result<OsString> {
417-
let wide_path = path.as_os_str().to_wide_with_nul();
422+
let wide_path = path.as_os_str().to_wide_with_nul()?;
418423
let mut find_data: WIN32_FIND_DATAW = unsafe { core::mem::zeroed() };
419424

420425
let handle = unsafe { FindFirstFileW(wide_path.as_ptr(), &mut find_data) }.check_valid()?;
@@ -446,7 +451,7 @@ pub fn path_isdevdrive(path: &Path) -> io::Result<bool> {
446451
reserved: u32,
447452
}
448453

449-
let wide_path = path.as_os_str().to_wide_with_nul();
454+
let wide_path = path.as_os_str().to_wide_with_nul()?;
450455
let mut volume = [0u16; MAX_PATH as usize];
451456
unsafe { GetVolumePathNameW(wide_path.as_ptr(), volume.as_mut_ptr(), volume.len() as _) }
452457
.check_win32_bool()?;
@@ -613,7 +618,7 @@ fn win32_xstat_attributes_from_dir(
613618
BY_HANDLE_FILE_INFORMATION, FILE_ATTRIBUTE_REPARSE_POINT,
614619
};
615620

616-
let wide: Vec<u16> = path.to_wide_with_nul();
621+
let wide: Vec<u16> = path.to_wide_with_nul()?;
617622
let mut find_data: WIN32_FIND_DATAW = unsafe { core::mem::zeroed() };
618623

619624
let handle = unsafe { FindFirstFileW(wide.as_ptr(), &mut find_data) }.check_valid()?;
@@ -651,7 +656,7 @@ fn win32_xstat_slow_impl(path: &OsStr, traverse: bool) -> io::Result<StatStruct>
651656
},
652657
};
653658

654-
let wide: Vec<u16> = path.to_wide_with_nul();
659+
let wide: Vec<u16> = path.to_wide_with_nul()?;
655660
let access = FILE_READ_ATTRIBUTES;
656661
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
657662
if !traverse {
@@ -922,7 +927,9 @@ pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> bool {
922927
if !matches!(tested_type, TestType::RegularFile | TestType::Directory) {
923928
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
924929
}
925-
let wide_path = path.as_os_str().to_wide_with_nul();
930+
let Ok(wide_path) = path.as_os_str().to_wide_with_nul() else {
931+
return false;
932+
};
926933
let handle = unsafe {
927934
CreateFileW(
928935
wide_path.as_ptr(),
@@ -988,7 +995,9 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
988995
}
989996
}
990997

991-
let wide_path = path.as_os_str().to_wide_with_nul();
998+
let Ok(wide_path) = path.as_os_str().to_wide_with_nul() else {
999+
return false;
1000+
};
9921001
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
9931002
if !follow_links {
9941003
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
@@ -1046,7 +1055,9 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
10461055
}
10471056

10481057
pub fn path_exists_via_open(path: &Path, follow_links: bool) -> bool {
1049-
let wide_path = path.as_os_str().to_wide_with_nul();
1058+
let Ok(wide_path) = path.as_os_str().to_wide_with_nul() else {
1059+
return false;
1060+
};
10501061
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
10511062
if !follow_links {
10521063
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
@@ -1270,7 +1281,10 @@ pub fn readlink(path: &Path) -> Result<OsString, ReadlinkError> {
12701281
IO_REPARSE_TAG_MOUNT_POINT, IO_REPARSE_TAG_SYMLINK,
12711282
};
12721283

1273-
let wide_path = path.as_os_str().to_wide_with_nul();
1284+
let wide_path = path
1285+
.as_os_str()
1286+
.to_wide_with_nul()
1287+
.map_err(ReadlinkError::Io)?;
12741288
let handle = unsafe {
12751289
CreateFileW(
12761290
wide_path.as_ptr(),
@@ -1369,7 +1383,7 @@ pub fn kill(pid: u32, sig: u32) -> io::Result<()> {
13691383
pub fn getfinalpathname(path: &Path) -> io::Result<OsString> {
13701384
use windows_sys::Win32::Storage::FileSystem::{GetFinalPathNameByHandleW, VOLUME_NAME_DOS};
13711385

1372-
let wide = path.as_os_str().to_wide_with_nul();
1386+
let wide = path.as_os_str().to_wide_with_nul()?;
13731387
let handle = unsafe {
13741388
CreateFileW(
13751389
wide.as_ptr(),
@@ -1407,7 +1421,7 @@ pub fn getfinalpathname(path: &Path) -> io::Result<OsString> {
14071421
}
14081422

14091423
pub fn getfullpathname(path: &Path) -> io::Result<OsString> {
1410-
let wide = path.as_os_str().to_wide_with_nul();
1424+
let wide = path.as_os_str().to_wide_with_nul()?;
14111425
let mut buffer = vec![0u16; MAX_PATH as usize];
14121426
let mut ret = unsafe {
14131427
windows_sys::Win32::Storage::FileSystem::GetFullPathNameW(
@@ -1435,7 +1449,7 @@ pub fn getfullpathname(path: &Path) -> io::Result<OsString> {
14351449
}
14361450

14371451
pub fn getvolumepathname(path: &Path) -> io::Result<OsString> {
1438-
let wide = path.as_os_str().to_wide_with_nul();
1452+
let wide = path.as_os_str().to_wide_with_nul()?;
14391453
let buflen = core::cmp::max(wide.len(), MAX_PATH as usize);
14401454
let mut buffer = vec![0u16; buflen];
14411455
unsafe {
@@ -1452,7 +1466,7 @@ pub fn getvolumepathname(path: &Path) -> io::Result<OsString> {
14521466
pub fn getdiskusage(path: &Path) -> io::Result<(u64, u64)> {
14531467
use windows_sys::Win32::Storage::FileSystem::GetDiskFreeSpaceExW;
14541468

1455-
let wide = path.as_os_str().to_wide_with_nul();
1469+
let wide = path.as_os_str().to_wide_with_nul()?;
14561470
let mut free_to_me = 0u64;
14571471
let mut total = 0u64;
14581472
let mut free = 0u64;
@@ -1584,7 +1598,7 @@ pub fn listvolumes() -> io::Result<Vec<OsString>> {
15841598
}
15851599

15861600
pub fn listmounts(volume: &Path) -> io::Result<Vec<OsString>> {
1587-
let wide = volume.as_os_str().to_wide_with_nul();
1601+
let wide = volume.as_os_str().to_wide_with_nul()?;
15881602
let mut buflen: u32 = MAX_PATH + 1;
15891603
let mut buffer = vec![0u16; buflen as usize];
15901604

@@ -1676,6 +1690,7 @@ pub fn getppid() -> u32 {
16761690

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

‎crates/host_env/src/overlapped.rs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use std::{
1616
};
1717

1818
use crate::windows::{CheckWin32Bool, CheckWin32Handle};
19+
use rustpython_wtf8::Wtf8;
1920
use windows_sys::Win32::{
2021
Foundation::{CloseHandle, ERROR_IO_PENDING, ERROR_MORE_DATA, ERROR_SUCCESS, HANDLE},
2122
Networking::WinSock::{AF_INET, AF_INET6, SOCKADDR, SOCKADDR_IN, SOCKADDR_IN6},
@@ -1028,6 +1029,7 @@ pub fn parse_address_v4_wide(host_wide: &[u16], port: u16) -> io::Result<(Vec<u8
10281029

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

1032+
// SAFETY: host_wide is nul capped and doesn't have interior nuls
10311033
let ret = unsafe {
10321034
WSAStringToAddressW(
10331035
host_wide.as_ptr(),
@@ -1056,7 +1058,10 @@ pub fn parse_address_v4_wide(host_wide: &[u16], port: u16) -> io::Result<(Vec<u8
10561058
}
10571059

10581060
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();
1061+
let host_wide: Vec<u16> = Wtf8::new(host)
1062+
.encode_wide_ffi()
1063+
.collect::<Result<_, _>>()
1064+
.map_err(io::Error::other)?;
10601065
parse_address_v4_wide(&host_wide, port)
10611066
}
10621067

@@ -1066,7 +1071,10 @@ pub fn parse_address_v6(
10661071
flowinfo: u32,
10671072
scope_id: u32,
10681073
) -> io::Result<(Vec<u8>, i32)> {
1069-
let host_wide: Vec<u16> = host.encode_utf16().chain([0]).collect();
1074+
let host_wide: Vec<u16> = Wtf8::new(host)
1075+
.encode_wide_ffi()
1076+
.collect::<Result<_, _>>()
1077+
.map_err(io::Error::other)?;
10701078
parse_address_v6_wide(&host_wide, port, flowinfo, scope_id)
10711079
}
10721080

@@ -1083,6 +1091,7 @@ pub fn parse_address_v6_wide(
10831091

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

1094+
// SAFETY: host_wide is nul capped and doesn't have interior nuls
10861095
let ret = unsafe {
10871096
WSAStringToAddressW(
10881097
host_wide.as_ptr(),

‎crates/host_env/src/posix_windows.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ fn rename_impl(
7878
.into_vec_with_nul();
7979

8080
// SAFETY:
81-
// * from and to are NUL terminated wide strings
81+
// * from and to are NUL terminated wide strings without interior nuls
8282
let success = unsafe {
8383
// Rust's [`std::fs::rename`] is more complicated than CPython's. Rust attempts to use modern APIs
8484
// where available, such as `FileRenameInfoEx`, which better map to POSIX. CPython simply

‎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(),

0 commit comments

Comments
 (0)