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

Commit 191b277

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 a handful of Windows functions 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. **Sources:** * https://owasp.org/www-community/attacks/Embedding_Null_Code * python/cpython#11656
1 parent 81df1ff commit 191b277

20 files changed

Lines changed: 475 additions & 520 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 & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ pub fn fstat(fd: crate::crt_fd::Borrowed<'_>) -> std::io::Result<StatStruct> {
1717
#[cfg(windows)]
1818
pub mod windows {
1919
use crate::crt_fd;
20-
use crate::windows::ToWideString;
2120
use libc::{S_IFCHR, S_IFDIR, S_IFMT};
2221
use std::ffi::OsStr;
2322
use std::os::windows::io::AsRawHandle;
23+
use std::path::Path;
2424
use std::sync::OnceLock;
2525
use windows_sys::Win32::Foundation::{
2626
ERROR_INVALID_HANDLE, ERROR_NOT_SUPPORTED, FILETIME, FreeLibrary, SetLastError,
@@ -67,21 +67,15 @@ pub mod windows {
6767
impl StatStruct {
6868
// update_st_mode_from_path in cpython
6969
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-
}
70+
if attr & FILE_ATTRIBUTE_DIRECTORY == 0
71+
&& let Some(file_extension) =
72+
Path::new(path).extension().and_then(|ext| ext.to_str())
73+
&& (file_extension.eq_ignore_ascii_case("exe")
74+
|| file_extension.eq_ignore_ascii_case("bat")
75+
|| file_extension.eq_ignore_ascii_case("cmd")
76+
|| file_extension.eq_ignore_ascii_case("com"))
77+
{
78+
self.st_mode |= 0o111;
8579
}
8680
}
8781
}
@@ -288,7 +282,7 @@ pub mod windows {
288282

289283
// _Py_GetFileInformationByName in cpython
290284
pub fn get_file_information_by_name(
291-
file_name: &OsStr,
285+
file_name: &widestring::WideCStr,
292286
file_information_class: FILE_INFO_BY_NAME_CLASS,
293287
) -> std::io::Result<FILE_STAT_BASIC_INFORMATION> {
294288
static GET_FILE_INFORMATION_BY_NAME: OnceLock<
@@ -329,7 +323,6 @@ pub mod windows {
329323
})
330324
.ok_or_else(|| std::io::Error::from_raw_os_error(ERROR_NOT_SUPPORTED as _))?;
331325

332-
let file_name = file_name.to_wide_with_nul();
333326
let file_info_buffer_size = core::mem::size_of::<FILE_STAT_BASIC_INFORMATION>() as u32;
334327
let mut file_info_buffer = core::mem::MaybeUninit::<FILE_STAT_BASIC_INFORMATION>::uninit();
335328
unsafe {

0 commit comments

Comments
 (0)