Rollup merge of #143090 - ChrisDenton:workaround1, r=tgross35

Workaround for memory unsafety in third party DLLs

Resolves #143078

Note that we can't make any guarantees if third parties intercept OS functions and don't implement them according to the documentation. However, I think it's practical to attempt mitigations when issues are encountered in the wild and the mitigation itself isn't too invasive.

Author: dianqk

library/std/src/sys/fs/windows/remove_dir_all.rs

@@ -33,7 +33,7 @@ use core::sync::atomic::{Atomic, AtomicU32, Ordering};
 
 use super::{AsRawHandle, DirBuff, File, FromRawHandle};
 use crate::sys::c;
-use crate::sys::pal::api::WinError;
+use crate::sys::pal::api::{UnicodeStrRef, WinError, unicode_str};
 use crate::thread;
 
 // The maximum number of times to spin when waiting for deletes to complete.
@@ -74,7 +74,7 @@ unsafe fn nt_open_file(
 /// `options` will be OR'd with `FILE_OPEN_REPARSE_POINT`.
 fn open_link_no_reparse(
     parent: &File,
-    path: &[u16],
+    path: UnicodeStrRef<'_>,
     access: u32,
     options: u32,
 ) -> Result<Option<File>, WinError> {
@@ -90,9 +90,8 @@ fn open_link_no_reparse(
     static ATTRIBUTES: Atomic<u32> = AtomicU32::new(c::OBJ_DONT_REPARSE);
 
     let result = unsafe {
-        let mut path_str = c::UNICODE_STRING::from_ref(path);
         let mut object = c::OBJECT_ATTRIBUTES {
-            ObjectName: &mut path_str,
+            ObjectName: path.as_ptr(),
             RootDirectory: parent.as_raw_handle(),
             Attributes: ATTRIBUTES.load(Ordering::Relaxed),
             ..c::OBJECT_ATTRIBUTES::with_length()
@@ -129,7 +128,7 @@ fn open_link_no_reparse(
     }
 }
 
-fn open_dir(parent: &File, name: &[u16]) -> Result<Option<File>, WinError> {
+fn open_dir(parent: &File, name: UnicodeStrRef<'_>) -> Result<Option<File>, WinError> {
     // Open the directory for synchronous directory listing.
     open_link_no_reparse(
         parent,
@@ -140,7 +139,7 @@ fn open_dir(parent: &File, name: &[u16]) -> Result<Option<File>, WinError> {
     )
 }
 
-fn delete(parent: &File, name: &[u16]) -> Result<(), WinError> {
+fn delete(parent: &File, name: UnicodeStrRef<'_>) -> Result<(), WinError> {
     // Note that the `delete` function consumes the opened file to ensure it's
     // dropped immediately. See module comments for why this is important.
     match open_link_no_reparse(parent, name, c::DELETE, 0) {
@@ -179,16 +178,17 @@ pub fn remove_dir_all_iterative(dir: File) -> Result<(), WinError> {
     'outer: while let Some(dir) = dirlist.pop() {
         let more_data = dir.fill_dir_buff(&mut buffer, restart)?;
         for (name, is_directory) in buffer.iter() {
+            let name = unicode_str!(&name);
             if is_directory {
-                let Some(subdir) = open_dir(&dir, &name)? else { continue };
+                let Some(subdir) = open_dir(&dir, name)? else { continue };
                 dirlist.push(dir);
                 dirlist.push(subdir);
                 continue 'outer;
             } else {
                 // Attempt to delete, retrying on sharing violation errors as these
                 // can often be very temporary. E.g. if something takes just a
                 // bit longer than expected to release a file handle.
-                retry(|| delete(&dir, &name), WinError::SHARING_VIOLATION)?;
+                retry(|| delete(&dir, name), WinError::SHARING_VIOLATION)?;
             }
         }
         if more_data {
@@ -197,7 +197,8 @@ pub fn remove_dir_all_iterative(dir: File) -> Result<(), WinError> {
         } else {
             // Attempt to delete, retrying on not empty errors because we may
             // need to wait some time for files to be removed from the filesystem.
-            retry(|| delete(&dir, &[]), WinError::DIR_NOT_EMPTY)?;
+            let name = unicode_str!("");
+            retry(|| delete(&dir, name), WinError::DIR_NOT_EMPTY)?;
             restart = true;
         }
     }

library/std/src/sys/pal/windows/api.rs

@@ -30,6 +30,7 @@
 //! should go in sys/pal/windows/mod.rs rather than here. See `IoResult` as an example.
 
 use core::ffi::c_void;
+use core::marker::PhantomData;
 
 use super::c;
 
@@ -291,3 +292,75 @@ impl WinError {
     pub const TIMEOUT: Self = Self::new(c::ERROR_TIMEOUT);
     // tidy-alphabetical-end
 }
+
+/// A wrapper around a UNICODE_STRING that is equivalent to `&[u16]`.
+///
+/// It is preferable to use the `unicode_str!` macro as that contains mitigations for  #143078.
+///
+/// If the MaximumLength field of the underlying UNICODE_STRING is greater than
+/// the Length field then you can test if the string is null terminated by inspecting
+/// the u16 directly after the string. You cannot otherwise depend on nul termination.
+#[derive(Copy, Clone)]
+pub struct UnicodeStrRef<'a> {
+    s: c::UNICODE_STRING,
+    lifetime: PhantomData<&'a [u16]>,
+}
+
+static EMPTY_STRING_NULL_TERMINATED: &[u16] = &[0];
+
+impl UnicodeStrRef<'_> {
+    const fn new(slice: &[u16], is_null_terminated: bool) -> Self {
+        let (len, max_len, ptr) = if slice.is_empty() {
+            (0, 2, EMPTY_STRING_NULL_TERMINATED.as_ptr().cast_mut())
+        } else {
+            let len = slice.len() - (is_null_terminated as usize);
+            (len * 2, size_of_val(slice), slice.as_ptr().cast_mut())
+        };
+        Self {
+            s: c::UNICODE_STRING { Length: len as _, MaximumLength: max_len as _, Buffer: ptr },
+            lifetime: PhantomData,
+        }
+    }
+
+    pub const fn from_slice_with_nul(slice: &[u16]) -> Self {
+        if !slice.is_empty() {
+            debug_assert!(slice[slice.len() - 1] == 0);
+        }
+        Self::new(slice, true)
+    }
+
+    pub const fn from_slice(slice: &[u16]) -> Self {
+        Self::new(slice, false)
+    }
+
+    /// Returns a pointer to the underlying UNICODE_STRING
+    pub const fn as_ptr(&self) -> *const c::UNICODE_STRING {
+        &self.s
+    }
+}
+
+/// Create a UnicodeStringRef from a literal str or a u16 array.
+///
+/// To mitigate #143078, when using a literal str the created UNICODE_STRING
+/// will be nul terminated. The MaximumLength field of the UNICODE_STRING will
+/// be set greater than the Length field to indicate that a nul may be present.
+///
+/// If using a u16 array, the array is used exactly as provided and you cannot
+/// count on the string being nul terminated.
+/// This should generally be used for strings that come from the OS.
+///
+/// **NOTE:** we lack a UNICODE_STRING builder type as we don't currently have
+/// a use for it. If needing to dynamically build a UNICODE_STRING, the builder
+/// should try to ensure there's a nul one past the end of the string.
+pub macro unicode_str {
+    ($str:literal) => {const {
+        crate::sys::pal::windows::api::UnicodeStrRef::from_slice_with_nul(
+            crate::sys::pal::windows::api::wide_str!($str),
+        )
+    }},
+    ($array:expr) => {
+        crate::sys::pal::windows::api::UnicodeStrRef::from_slice(
+            $array,
+        )
+    }
+}

library/std/src/sys/pal/windows/c.rs

@@ -37,13 +37,6 @@ pub fn nt_success(status: NTSTATUS) -> bool {
     status >= 0
 }
 
-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 _ }
-    }
-}
-
 impl OBJECT_ATTRIBUTES {
     pub fn with_length() -> Self {
         Self {

library/std/src/sys/pal/windows/pipe.rs

@@ -1,9 +1,8 @@
 use crate::io::{self, BorrowedCursor, IoSlice, IoSliceMut};
 use crate::ops::Neg;
 use crate::os::windows::prelude::*;
-use crate::sys::api::utf16;
-use crate::sys::c;
 use crate::sys::handle::Handle;
+use crate::sys::{api, c};
 use crate::sys_common::{FromInner, IntoInner};
 use crate::{mem, ptr};
 
@@ -73,8 +72,8 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res
         // Open a handle to the pipe filesystem (`\??\PIPE\`).
         // This will be used when creating a new annon pipe.
         let pipe_fs = {
-            let path = c::UNICODE_STRING::from_ref(utf16!(r"\??\PIPE\"));
-            object_attributes.ObjectName = &path;
+            let path = api::unicode_str!(r"\??\PIPE\");
+            object_attributes.ObjectName = path.as_ptr();
             let mut pipe_fs = ptr::null_mut();
             let status = c::NtOpenFile(
                 &mut pipe_fs,
@@ -93,8 +92,12 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res
 
         // From now on we're using handles instead of paths to create and open pipes.
         // So set the `ObjectName` to a zero length string.
+        // As a (perhaps overzealous) mitigation for #143078, we use the null pointer
+        // for empty.Buffer instead of unicode_str!("").
+        // There's no difference to the OS itself but it's possible that third party
+        // DLLs which hook in to processes could be relying on the exact form of this string.
         let empty = c::UNICODE_STRING::default();
-        object_attributes.ObjectName = ∅
+        object_attributes.ObjectName = &raw const empty;
 
         // Create our side of the pipe for async access.
         let ours = {