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

Commit ba54d9d

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 81df1ff commit ba54d9d

19 files changed

Lines changed: 320 additions & 251 deletions

File tree

‎crates/host_env/src/ctypes.rs‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ use libloading::Library;
3333
use libloading::os::unix::Library as UnixLibrary;
3434
#[cfg(any(unix, windows))]
3535
use parking_lot::{Mutex, RwLock};
36-
use rustpython_wtf8::Wtf8;
37-
use rustpython_wtf8::Wtf8Buf;
36+
use rustpython_wtf8::{Wtf8, Wtf8Buf};
3837
#[cfg(any(unix, windows))]
3938
use std::{collections::HashMap, ffi::OsStr, sync::OnceLock};
4039
use widestring::WideCStr;
@@ -1091,10 +1090,15 @@ pub fn utf16z_bytes(s: &Wtf8) -> Vec<u8> {
10911090
.collect()
10921091
}
10931092

1093+
/// Return a NUL terminated copy of `bytes`.
1094+
///
1095+
/// The input may contain interior NULs.
10941096
pub fn null_terminated_bytes(bytes: &[u8]) -> Vec<u8> {
1095-
let mut buffer = bytes.to_vec();
1096-
buffer.push(0);
1097-
buffer
1097+
if bytes.last() == Some(&0) {
1098+
bytes.to_vec()
1099+
} else {
1100+
bytes.iter().copied().chain(Some(0)).collect()
1101+
}
10981102
}
10991103

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

‎crates/host_env/src/fileutils.rs‎

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ pub mod windows {
2121
use libc::{S_IFCHR, S_IFDIR, S_IFMT};
2222
use std::ffi::OsStr;
2323
use std::os::windows::io::AsRawHandle;
24+
use std::path::Path;
2425
use std::sync::OnceLock;
2526
use windows_sys::Win32::Foundation::{
2627
ERROR_INVALID_HANDLE, ERROR_NOT_SUPPORTED, FILETIME, FreeLibrary, SetLastError,
@@ -67,21 +68,15 @@ pub mod windows {
6768
impl StatStruct {
6869
// update_st_mode_from_path in cpython
6970
pub fn update_st_mode_from_path(&mut self, path: &OsStr, attr: u32) {
70-
if attr & FILE_ATTRIBUTE_DIRECTORY == 0 {
71-
let file_extension = path
72-
.to_wide()
73-
.split(|&c| c == '.' as u16)
74-
.next_back()
75-
.and_then(|s| String::from_utf16(s).ok());
76-
77-
if let Some(file_extension) = file_extension
78-
&& (file_extension.eq_ignore_ascii_case("exe")
79-
|| file_extension.eq_ignore_ascii_case("bat")
80-
|| file_extension.eq_ignore_ascii_case("cmd")
81-
|| file_extension.eq_ignore_ascii_case("com"))
82-
{
83-
self.st_mode |= 0o111;
84-
}
71+
if attr & FILE_ATTRIBUTE_DIRECTORY == 0
72+
&& let Some(file_extension) =
73+
Path::new(path).extension().and_then(|ext| ext.to_str())
74+
&& (file_extension.eq_ignore_ascii_case("exe")
75+
|| file_extension.eq_ignore_ascii_case("bat")
76+
|| file_extension.eq_ignore_ascii_case("cmd")
77+
|| file_extension.eq_ignore_ascii_case("com"))
78+
{
79+
self.st_mode |= 0o111;
8580
}
8681
}
8782
}
@@ -329,7 +324,7 @@ pub mod windows {
329324
})
330325
.ok_or_else(|| std::io::Error::from_raw_os_error(ERROR_NOT_SUPPORTED as _))?;
331326

332-
let file_name = file_name.to_wide_with_nul();
327+
let file_name = file_name.to_wide_with_nul()?;
333328
let file_info_buffer_size = core::mem::size_of::<FILE_STAT_BASIC_INFORMATION>() as u32;
334329
let mut file_info_buffer = core::mem::MaybeUninit::<FILE_STAT_BASIC_INFORMATION>::uninit();
335330
unsafe {

‎crates/host_env/src/nt.rs‎

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,9 @@ pub enum ReadConsoleError {
221221
}
222222

223223
pub fn access(path: &Path, mode: u8) -> bool {
224-
let wide = path.as_os_str().to_wide_with_nul();
224+
let Ok(wide) = path.as_os_str().to_wide_with_nul() else {
225+
return false;
226+
};
225227
let attr = unsafe { GetFileAttributesW(wide.as_ptr()) };
226228
attr != INVALID_FILE_ATTRIBUTES
227229
&& (mode & 2 == 0
@@ -237,7 +239,7 @@ pub fn remove(path: &Path) -> io::Result<()> {
237239
IO_REPARSE_TAG_MOUNT_POINT, IO_REPARSE_TAG_SYMLINK,
238240
};
239241

240-
let wide_path = path.as_os_str().to_wide_with_nul();
242+
let wide_path = path.as_os_str().to_wide_with_nul()?;
241243
let attrs = unsafe { GetFileAttributesW(wide_path.as_ptr()) };
242244

243245
let mut is_directory = false;
@@ -384,7 +386,7 @@ pub fn fchmod(fd: i32, mode: u32, write_bit: u32) -> io::Result<()> {
384386
}
385387

386388
pub fn win32_lchmod(path: &OsStr, mode: u32, write_bit: u32) -> io::Result<()> {
387-
let wide = path.to_wide_with_nul();
389+
let wide = path.to_wide_with_nul()?;
388390
let attr = unsafe { GetFileAttributesW(wide.as_ptr()) }.check_ne(INVALID_FILE_ATTRIBUTES)?;
389391
let new_attr = if mode & write_bit != 0 {
390392
attr & !FILE_ATTRIBUTE_READONLY
@@ -417,7 +419,7 @@ pub fn chmod_follow(path: &widestring::WideCStr, mode: u32, write_bit: u32) -> i
417419
}
418420

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

423425
let handle = unsafe { FindFirstFileW(wide_path.as_ptr(), &mut find_data) }.check_valid()?;
@@ -449,7 +451,7 @@ pub fn path_isdevdrive(path: &Path) -> io::Result<bool> {
449451
reserved: u32,
450452
}
451453

452-
let wide_path = path.as_os_str().to_wide_with_nul();
454+
let wide_path = path.as_os_str().to_wide_with_nul()?;
453455
let mut volume = [0u16; MAX_PATH as usize];
454456
unsafe { GetVolumePathNameW(wide_path.as_ptr(), volume.as_mut_ptr(), volume.len() as _) }
455457
.check_win32_bool()?;
@@ -616,7 +618,7 @@ fn win32_xstat_attributes_from_dir(
616618
BY_HANDLE_FILE_INFORMATION, FILE_ATTRIBUTE_REPARSE_POINT,
617619
};
618620

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

622624
let handle = unsafe { FindFirstFileW(wide.as_ptr(), &mut find_data) }.check_valid()?;
@@ -654,7 +656,7 @@ fn win32_xstat_slow_impl(path: &OsStr, traverse: bool) -> io::Result<StatStruct>
654656
},
655657
};
656658

657-
let wide: Vec<u16> = path.to_wide_with_nul();
659+
let wide: Vec<u16> = path.to_wide_with_nul()?;
658660
let access = FILE_READ_ATTRIBUTES;
659661
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
660662
if !traverse {
@@ -925,7 +927,9 @@ pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> bool {
925927
if !matches!(tested_type, TestType::RegularFile | TestType::Directory) {
926928
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
927929
}
928-
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+
};
929933
let handle = unsafe {
930934
CreateFileW(
931935
wide_path.as_ptr(),
@@ -991,7 +995,9 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
991995
}
992996
}
993997

994-
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+
};
9951001
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
9961002
if !follow_links {
9971003
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
@@ -1049,7 +1055,9 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
10491055
}
10501056

10511057
pub fn path_exists_via_open(path: &Path, follow_links: bool) -> bool {
1052-
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+
};
10531061
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
10541062
if !follow_links {
10551063
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
@@ -1271,7 +1279,10 @@ pub fn readlink(path: &Path) -> Result<OsString, ReadlinkError> {
12711279
IO_REPARSE_TAG_MOUNT_POINT, IO_REPARSE_TAG_SYMLINK,
12721280
};
12731281

1274-
let wide_path = path.as_os_str().to_wide_with_nul();
1282+
let wide_path = path
1283+
.as_os_str()
1284+
.to_wide_with_nul()
1285+
.map_err(ReadlinkError::Io)?;
12751286
let handle = unsafe {
12761287
CreateFileW(
12771288
wide_path.as_ptr(),
@@ -1372,7 +1383,7 @@ pub fn kill(pid: u32, sig: u32) -> io::Result<()> {
13721383
pub fn getfinalpathname(path: &Path) -> io::Result<OsString> {
13731384
use windows_sys::Win32::Storage::FileSystem::{GetFinalPathNameByHandleW, VOLUME_NAME_DOS};
13741385

1375-
let wide = path.as_os_str().to_wide_with_nul();
1386+
let wide = path.as_os_str().to_wide_with_nul()?;
13761387
let handle = unsafe {
13771388
CreateFileW(
13781389
wide.as_ptr(),
@@ -1410,7 +1421,7 @@ pub fn getfinalpathname(path: &Path) -> io::Result<OsString> {
14101421
}
14111422

14121423
pub fn getfullpathname(path: &Path) -> io::Result<OsString> {
1413-
let wide = path.as_os_str().to_wide_with_nul();
1424+
let wide = path.as_os_str().to_wide_with_nul()?;
14141425
let mut buffer = vec![0u16; MAX_PATH as usize];
14151426
let mut ret = unsafe {
14161427
windows_sys::Win32::Storage::FileSystem::GetFullPathNameW(
@@ -1438,7 +1449,7 @@ pub fn getfullpathname(path: &Path) -> io::Result<OsString> {
14381449
}
14391450

14401451
pub fn getvolumepathname(path: &Path) -> io::Result<OsString> {
1441-
let wide = path.as_os_str().to_wide_with_nul();
1452+
let wide = path.as_os_str().to_wide_with_nul()?;
14421453
let buflen = core::cmp::max(wide.len(), MAX_PATH as usize);
14431454
let mut buffer = vec![0u16; buflen];
14441455
unsafe {
@@ -1455,7 +1466,7 @@ pub fn getvolumepathname(path: &Path) -> io::Result<OsString> {
14551466
pub fn getdiskusage(path: &Path) -> io::Result<(u64, u64)> {
14561467
use windows_sys::Win32::Storage::FileSystem::GetDiskFreeSpaceExW;
14571468

1458-
let wide = path.as_os_str().to_wide_with_nul();
1469+
let wide = path.as_os_str().to_wide_with_nul()?;
14591470
let mut free_to_me = 0u64;
14601471
let mut total = 0u64;
14611472
let mut free = 0u64;
@@ -1587,7 +1598,7 @@ pub fn listvolumes() -> io::Result<Vec<OsString>> {
15871598
}
15881599

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

@@ -1679,6 +1690,7 @@ pub fn getppid() -> u32 {
16791690

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

‎crates/host_env/src/overlapped.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ use std::{
1515
sync::{Mutex, OnceLock},
1616
};
1717

18-
use crate::windows::{CheckWin32Bool, CheckWin32Handle};
18+
use crate::windows::{CheckWin32Bool, CheckWin32Handle, ToWideString};
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,7 @@ 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).to_wide_with_nul()?;
10601062
parse_address_v4_wide(&host_wide, port)
10611063
}
10621064

@@ -1066,7 +1068,7 @@ pub fn parse_address_v6(
10661068
flowinfo: u32,
10671069
scope_id: u32,
10681070
) -> io::Result<(Vec<u8>, i32)> {
1069-
let host_wide: Vec<u16> = host.encode_utf16().chain([0]).collect();
1071+
let host_wide: Vec<u16> = Wtf8::new(host).to_wide_with_nul()?;
10701072
parse_address_v6_wide(&host_wide, port, flowinfo, scope_id)
10711073
}
10721074

@@ -1083,6 +1085,7 @@ pub fn parse_address_v6_wide(
10831085

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

1088+
// SAFETY: host_wide is nul capped and doesn't have interior nuls
10861089
let ret = unsafe {
10871090
WSAStringToAddressW(
10881091
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(),

‎crates/host_env/src/windows.rs‎

Lines changed: 17 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@ use rustpython_wtf8::Wtf8;
22
use std::{
33
ffi::{OsStr, OsString},
44
io,
5-
os::windows::ffi::{OsStrExt, OsStringExt},
5+
os::windows::ffi::OsStringExt,
66
};
7+
use widestring::WideCString;
78
use windows_sys::{
89
Win32::{
910
Foundation::{
@@ -397,54 +398,32 @@ pub fn multi_byte_to_wide(
397398
}
398399
}
399400

401+
/// [`OsStr`] to [`WideCString`] for Windows FFI.
402+
///
403+
/// Prefer using this trait when encoding bytes to pass to Windows. Interior NULs are memory safe
404+
/// but possibly a security hazard for FFI.
405+
///
406+
/// https://github.com/python/cpython/issues/111656
400407
pub trait ToWideString {
401-
fn to_wide(&self) -> Vec<u16>;
402-
fn to_wide_with_nul(&self) -> Vec<u16>;
403-
fn to_wide_cstring(&self) -> widestring::WideCString {
404-
widestring::WideCString::from_vec_truncate(self.to_wide())
408+
fn to_wide_with_nul(&self) -> Result<Vec<u16>, io::Error> {
409+
self.to_wide_cstring().map(WideCString::into_vec_with_nul)
405410
}
411+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error>;
406412
}
407413

408414
impl<T> ToWideString for T
409415
where
410416
T: AsRef<OsStr>,
411417
{
412-
fn to_wide(&self) -> Vec<u16> {
413-
self.as_ref().encode_wide().collect()
414-
}
415-
fn to_wide_with_nul(&self) -> Vec<u16> {
416-
self.as_ref().encode_wide().chain(Some(0)).collect()
417-
}
418-
}
419-
420-
impl ToWideString for OsStr {
421-
fn to_wide(&self) -> Vec<u16> {
422-
self.encode_wide().collect()
423-
}
424-
fn to_wide_with_nul(&self) -> Vec<u16> {
425-
self.encode_wide().chain(Some(0)).collect()
418+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error> {
419+
WideCString::from_os_str(self).map_err(|_| io::Error::other("embedded null character"))
426420
}
427421
}
428422

429423
impl ToWideString for Wtf8 {
430-
fn to_wide(&self) -> Vec<u16> {
431-
self.encode_wide().collect()
432-
}
433-
fn to_wide_with_nul(&self) -> Vec<u16> {
434-
self.encode_wide().chain(Some(0)).collect()
435-
}
436-
}
437-
438-
pub trait FromWideString
439-
where
440-
Self: Sized,
441-
{
442-
fn from_wides_until_nul(wide: &[u16]) -> Self;
443-
}
444-
445-
impl FromWideString for OsString {
446-
fn from_wides_until_nul(wide: &[u16]) -> Self {
447-
let len = wide.iter().take_while(|&&c| c != 0).count();
448-
Self::from_wide(&wide[..len])
424+
fn to_wide_cstring(&self) -> Result<WideCString, io::Error> {
425+
let mut buf = Vec::with_capacity(self.len() * 2 + 2);
426+
buf.extend(self.encode_wide());
427+
WideCString::from_vec(buf).map_err(|_| io::Error::other("embedded null character"))
449428
}
450429
}

0 commit comments

Comments
 (0)