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

Commit 08865bf

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 a9c2c52 commit 08865bf

18 files changed

Lines changed: 301 additions & 157 deletions

File tree

‎crates/host_env/src/ctypes.rs‎

Lines changed: 55 additions & 29 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;
@@ -523,21 +526,49 @@ pub fn encode_wtf8_to_wchar_padded(s: &Wtf8, size: usize) -> Vec<u8> {
523526
wchar_bytes
524527
}
525528

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-
}
529+
/// Encode buffer as a wide string but yield bytes instead of u16.
530+
///
531+
/// The resulting bytes buffer is suitable for FFI. It is NUL capped without interior
532+
/// NULs.
533+
pub fn wchar_null_terminated_bytes(s: &Wtf8) -> impl Iterator<Item = Result<u8, InteriorNulError>> {
534+
let mut iter = s.code_points().map(|cp| cp.to_u32() as WChar);
535+
let mut pending: Option<array::IntoIter<_, _>> = None;
536+
let mut complete = false;
537+
538+
iter::from_fn(move || {
539+
if let Some(pending) = pending.as_mut()
540+
&& let Some(next) = pending.next()
541+
{
542+
return Some(Ok(next));
543+
}
544+
545+
match iter.next() {
546+
Some(0) => {
547+
core::hint::cold_path();
548+
complete = true;
549+
550+
for next in iter.by_ref() {
551+
if next != 0 {
552+
return Some(Err(InteriorNulError));
553+
}
554+
}
555+
556+
pending = Some((0 as WChar).to_ne_bytes().into_iter());
557+
}
558+
Some(next) => {
559+
pending = Some(next.to_ne_bytes().into_iter());
560+
}
561+
None if !complete => {
562+
complete = true;
563+
pending = Some((0 as WChar).to_ne_bytes().into_iter());
564+
}
565+
None => {
566+
return None;
567+
}
568+
}
534569

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) }
570+
pending.as_mut().unwrap().next().map(Result::Ok)
571+
})
541572
}
542573

543574
pub enum IntegerValue {
@@ -1107,14 +1138,9 @@ pub fn simple_storage_value_to_bytes_endian(
11071138
}
11081139
}
11091140

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
1141+
#[inline]
1142+
pub fn null_terminated_bytes(bytes: &[u8]) -> Result<Vec<u8>, alloc::ffi::NulError> {
1143+
CString::new(bytes).map(CString::into_bytes_with_nul)
11181144
}
11191145

11201146
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: 35 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -217,13 +217,13 @@ pub enum ReadConsoleError {
217217
BufferTooSmall { available: usize, required: usize },
218218
}
219219

220-
pub fn access(path: &Path, mode: u8) -> bool {
221-
let wide = path.as_os_str().to_wide_with_nul();
220+
pub fn access(path: &Path, mode: u8) -> Result<bool, io::Error> {
221+
let wide = path.as_os_str().to_wide_with_nul()?;
222222
let attr = unsafe { GetFileAttributesW(wide.as_ptr()) };
223-
attr != INVALID_FILE_ATTRIBUTES
223+
Ok(attr != INVALID_FILE_ATTRIBUTES
224224
&& (mode & 2 == 0
225225
|| attr & FILE_ATTRIBUTE_READONLY == 0
226-
|| attr & windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_DIRECTORY != 0)
226+
|| attr & windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_DIRECTORY != 0))
227227
}
228228

229229
pub fn remove(path: &Path) -> io::Result<()> {
@@ -882,7 +882,7 @@ pub fn win32_xstat(path: &OsStr, traverse: bool) -> io::Result<StatStruct> {
882882
Ok(result)
883883
}
884884

885-
pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> bool {
885+
pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> Result<bool, io::Error> {
886886
match get_file_information_by_name(
887887
path.as_os_str(),
888888
FILE_INFO_BY_NAME_CLASS::FileStatBasicByNameInfo,
@@ -906,14 +906,14 @@ pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> bool {
906906
& windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT)
907907
== 0
908908
{
909-
return result;
909+
return Ok(result);
910910
}
911911
}
912912
Err(err) => {
913913
if let Some(code) = err.raw_os_error()
914914
&& file_info_error_is_trustworthy(code as u32)
915915
{
916-
return false;
916+
return Ok(false);
917917
}
918918
}
919919
}
@@ -922,7 +922,7 @@ pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> bool {
922922
if !matches!(tested_type, TestType::RegularFile | TestType::Directory) {
923923
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
924924
}
925-
let wide_path = path.as_os_str().to_wide_with_nul();
925+
let wide_path = path.as_os_str().to_wide_with_nul()?;
926926
let handle = unsafe {
927927
CreateFileW(
928928
wide_path.as_ptr(),
@@ -937,7 +937,7 @@ pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> bool {
937937
if handle != INVALID_HANDLE_VALUE {
938938
let result = test_file_type_by_handle(handle, tested_type, false);
939939
unsafe { CloseHandle(handle) };
940-
return result;
940+
return Ok(result);
941941
}
942942

943943
match unsafe { GetLastError() } {
@@ -962,10 +962,10 @@ pub fn test_file_type_by_name(path: &Path, tested_type: TestType) -> bool {
962962
_ => {}
963963
}
964964

965-
false
965+
Ok(false)
966966
}
967967

968-
pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
968+
pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> Result<bool, io::Error> {
969969
match get_file_information_by_name(
970970
path.as_os_str(),
971971
FILE_INFO_BY_NAME_CLASS::FileStatBasicByNameInfo,
@@ -976,19 +976,19 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
976976
== 0
977977
|| (!follow_links && is_reparse_tag_name_surrogate(info.ReparseTag))
978978
{
979-
return true;
979+
return Ok(true);
980980
}
981981
}
982982
Err(err) => {
983983
if let Some(code) = err.raw_os_error()
984984
&& file_info_error_is_trustworthy(code as u32)
985985
{
986-
return false;
986+
return (false);
987987
}
988988
}
989989
}
990990

991-
let wide_path = path.as_os_str().to_wide_with_nul();
991+
let wide_path = path.as_os_str().to_wide_with_nul()?;
992992
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
993993
if !follow_links {
994994
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
@@ -1007,13 +1007,13 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
10071007
if handle != INVALID_HANDLE_VALUE {
10081008
if follow_links {
10091009
unsafe { CloseHandle(handle) };
1010-
return true;
1010+
return Ok(true);
10111011
}
10121012
let is_regular_reparse_point =
10131013
test_file_type_by_handle(handle, TestType::RegularReparsePoint, false);
10141014
unsafe { CloseHandle(handle) };
10151015
if !is_regular_reparse_point {
1016-
return true;
1016+
return Ok(true);
10171017
}
10181018
let handle = unsafe {
10191019
CreateFileW(
@@ -1028,7 +1028,7 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
10281028
};
10291029
if handle != INVALID_HANDLE_VALUE {
10301030
unsafe { CloseHandle(handle) };
1031-
return true;
1031+
return Ok(true);
10321032
}
10331033
}
10341034

@@ -1037,16 +1037,16 @@ pub fn test_file_exists_by_name(path: &Path, follow_links: bool) -> bool {
10371037
| windows_sys::Win32::Foundation::ERROR_SHARING_VIOLATION
10381038
| windows_sys::Win32::Foundation::ERROR_CANT_ACCESS_FILE
10391039
| windows_sys::Win32::Foundation::ERROR_INVALID_PARAMETER => {
1040-
return win32_xstat(path.as_os_str(), follow_links).is_ok();
1040+
return Ok(win32_xstat(path.as_os_str(), follow_links).is_ok());
10411041
}
10421042
_ => {}
10431043
}
10441044

1045-
false
1045+
Ok(false)
10461046
}
10471047

1048-
pub fn path_exists_via_open(path: &Path, follow_links: bool) -> bool {
1049-
let wide_path = path.as_os_str().to_wide_with_nul();
1048+
pub fn path_exists_via_open(path: &Path, follow_links: bool) -> Result<bool, io::Error> {
1049+
let wide_path = path.as_os_str().to_wide_with_nul()?;
10501050
let mut flags = FILE_FLAG_BACKUP_SEMANTICS;
10511051
if !follow_links {
10521052
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
@@ -1065,13 +1065,13 @@ pub fn path_exists_via_open(path: &Path, follow_links: bool) -> bool {
10651065
if handle != INVALID_HANDLE_VALUE {
10661066
if follow_links {
10671067
unsafe { CloseHandle(handle) };
1068-
return true;
1068+
return Ok(true);
10691069
}
10701070
let is_regular_reparse_point =
10711071
test_file_type_by_handle(handle, TestType::RegularReparsePoint, false);
10721072
unsafe { CloseHandle(handle) };
10731073
if !is_regular_reparse_point {
1074-
return true;
1074+
return Ok(true);
10751075
}
10761076
let handle = unsafe {
10771077
CreateFileW(
@@ -1086,10 +1086,10 @@ pub fn path_exists_via_open(path: &Path, follow_links: bool) -> bool {
10861086
};
10871087
if handle != INVALID_HANDLE_VALUE {
10881088
unsafe { CloseHandle(handle) };
1089-
return true;
1089+
return Ok(true);
10901090
}
10911091
}
1092-
false
1092+
Ok(false)
10931093
}
10941094

10951095
pub fn fd_exists(fd: crate::crt_fd::Borrowed<'_>) -> bool {
@@ -1270,7 +1270,10 @@ pub fn readlink(path: &Path) -> Result<OsString, ReadlinkError> {
12701270
IO_REPARSE_TAG_MOUNT_POINT, IO_REPARSE_TAG_SYMLINK,
12711271
};
12721272

1273-
let wide_path = path.as_os_str().to_wide_with_nul();
1273+
let wide_path = path
1274+
.as_os_str()
1275+
.to_wide_with_nul()
1276+
.map_err(ReadlinkError::Io)?;
12741277
let handle = unsafe {
12751278
CreateFileW(
12761279
wide_path.as_ptr(),
@@ -1369,7 +1372,7 @@ pub fn kill(pid: u32, sig: u32) -> io::Result<()> {
13691372
pub fn getfinalpathname(path: &Path) -> io::Result<OsString> {
13701373
use windows_sys::Win32::Storage::FileSystem::{GetFinalPathNameByHandleW, VOLUME_NAME_DOS};
13711374

1372-
let wide = path.as_os_str().to_wide_with_nul();
1375+
let wide = path.as_os_str().to_wide_with_nul()?;
13731376
let handle = unsafe {
13741377
CreateFileW(
13751378
wide.as_ptr(),
@@ -1407,7 +1410,7 @@ pub fn getfinalpathname(path: &Path) -> io::Result<OsString> {
14071410
}
14081411

14091412
pub fn getfullpathname(path: &Path) -> io::Result<OsString> {
1410-
let wide = path.as_os_str().to_wide_with_nul();
1413+
let wide = path.as_os_str().to_wide_with_nul()?;
14111414
let mut buffer = vec![0u16; MAX_PATH as usize];
14121415
let mut ret = unsafe {
14131416
windows_sys::Win32::Storage::FileSystem::GetFullPathNameW(
@@ -1435,7 +1438,7 @@ pub fn getfullpathname(path: &Path) -> io::Result<OsString> {
14351438
}
14361439

14371440
pub fn getvolumepathname(path: &Path) -> io::Result<OsString> {
1438-
let wide = path.as_os_str().to_wide_with_nul();
1441+
let wide = path.as_os_str().to_wide_with_nul()?;
14391442
let buflen = core::cmp::max(wide.len(), MAX_PATH as usize);
14401443
let mut buffer = vec![0u16; buflen];
14411444
unsafe {
@@ -1452,7 +1455,7 @@ pub fn getvolumepathname(path: &Path) -> io::Result<OsString> {
14521455
pub fn getdiskusage(path: &Path) -> io::Result<(u64, u64)> {
14531456
use windows_sys::Win32::Storage::FileSystem::GetDiskFreeSpaceExW;
14541457

1455-
let wide = path.as_os_str().to_wide_with_nul();
1458+
let wide = path.as_os_str().to_wide_with_nul()?;
14561459
let mut free_to_me = 0u64;
14571460
let mut total = 0u64;
14581461
let mut free = 0u64;
@@ -1584,7 +1587,7 @@ pub fn listvolumes() -> io::Result<Vec<OsString>> {
15841587
}
15851588

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

@@ -1676,6 +1679,7 @@ pub fn getppid() -> u32 {
16761679

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

0 commit comments

Comments
 (0)