Description
Third-party software can inject a DLL into firefox.exe that detours calls to NtOpenFile to monitor the use of this system call. Some of these third-party DLLs can assume that in any NtOpenFile call, ObjectAttributes->ObjectName.Buffer is a valid pointer to dereference, and more generally that it is a null-terminated wide string. They do not check ObjectAttributes->ObjectName.Length nor ObjectAttributes->ObjectName.MaxLength. Rust's implementation of remove_dir_all on Windows makes direct calls to NtOpenFile that violate this third-party assumption, resulting in Firefox crashes in Bug 1973947.
While one could argue that this is an incorrect assumption to have in the first place, it turns out that this assumption does not usually lead to crashes. This is because the assumption seems valid for the UNICODE_STRING values that usually flow into NtOpenFile, for example through win32 API calls, because these calls happen to always provide null-terminated wide strings in Buffer. For example they may rely on RtlInitUnicodeString to wrap null-terminated wide strings as UNICODE_STRING values. RtlInitUnicodeString(&unicode_string, wide_string) will typically set unicode_string.Buffer to wide_string, unicode_string.Length to wcslen(wide_string) and unicode_string.MaxLength to wcslen(wide_string) + 2 (accounting for the null wide char).
Rust's implementation of remove_dir_all appears to be the only violation for this assumption when firefox.exe runs. Would it be possible to enforce this assumption, to mimic the way usual calls to NtOpenFile behave, thereby preventing these crashes? In particular, the code below generates a UNICODE_STRING where Length is 0, MaxLength is 0 and Buffer is 2:
pub fn remove_dir_all_iterative(dir: File) -> Result<(), WinError> {
// ...
retry(|| delete(&dir, &[]), WinError::DIR_NOT_EMPTY)?;
// ...
}This can be reproduced in a standalone binary. The code below prints 0 0 2:
type PWSTR = *mut u16;
#[repr(C)]
#[derive(Clone, Copy)]
struct UNICODE_STRING {
pub Length: u16,
pub MaximumLength: u16,
pub Buffer: PWSTR,
}
impl UNICODE_STRING {
pub fn from_ref(slice: &[u16]) -> Self {
let len = size_of_val(slice);
Self { Length: len as _, MaximumLength: len as _, Buffer: slice.as_ptr() as _ }
}
}
fn main() {
let str = UNICODE_STRING::from_ref(&[]);
println!("{} {} {}", str.Length, str.MaximumLength, str.Buffer as u64);
}This explains why the Firefox crashes we receive are caused by derefencing 2 as a pointer.