Rollup merge of #109960 - thomcc:symlink-junction-buffer-overrun, r=ChrisDenton

Fix buffer overrun in bootstrap and (test-only) symlink_junction

I don't think these can be hit in practice, due to their inputs being valid paths. It's also not security-sensitive code, but just... bad vibes.

I think this is still not really the right way to do this (in terms of path correctness), but is no worse than it was.

r? `@ChrisDenton`

Author: matthiaskrgr

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

@@ -1403,24 +1403,40 @@ fn symlink_junction_inner(original: &Path, junction: &Path) -> io::Result<()> {
     opts.custom_flags(c::FILE_FLAG_OPEN_REPARSE_POINT | c::FILE_FLAG_BACKUP_SEMANTICS);
     let f = File::open(junction, &opts)?;
     let h = f.as_inner().as_raw_handle();
-
     unsafe {
         let mut data = Align8([MaybeUninit::<u8>::uninit(); c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE]);
         let data_ptr = data.0.as_mut_ptr();
+        let data_end = data_ptr.add(c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE);
         let db = data_ptr.cast::<c::REPARSE_MOUNTPOINT_DATA_BUFFER>();
         // Zero the header to ensure it's fully initialized, including reserved parameters.
         *db = mem::zeroed();
-        let buf = ptr::addr_of_mut!((*db).ReparseTarget).cast::<c::WCHAR>();
-        let mut i = 0;
+        let reparse_target_slice = {
+            let buf_start = ptr::addr_of_mut!((*db).ReparseTarget).cast::<c::WCHAR>();
+            // Compute offset in bytes and then divide so that we round down
+            // rather than hit any UB (admittedly this arithmetic should work
+            // out so that this isn't necessary)
+            let buf_len_bytes = usize::try_from(data_end.byte_offset_from(buf_start)).unwrap();
+            let buf_len_wchars = buf_len_bytes / core::mem::size_of::<c::WCHAR>();
+            core::slice::from_raw_parts_mut(buf_start, buf_len_wchars)
+        };
+
         // FIXME: this conversion is very hacky
-        let v = br"\??\";
-        let v = v.iter().map(|x| *x as u16);
-        for c in v.chain(original.as_os_str().encode_wide()) {
-            *buf.add(i) = c;
+        let iter = br"\??\"
+            .iter()
+            .map(|x| *x as u16)
+            .chain(original.as_os_str().encode_wide())
+            .chain(core::iter::once(0));
+        let mut i = 0;
+        for c in iter {
+            if i >= reparse_target_slice.len() {
+                return Err(crate::io::const_io_error!(
+                    crate::io::ErrorKind::InvalidFilename,
+                    "Input filename is too long"
+                ));
+            }
+            reparse_target_slice[i] = c;
             i += 1;
         }
-        *buf.add(i) = 0;
-        i += 1;
         (*db).ReparseTag = c::IO_REPARSE_TAG_MOUNT_POINT;
         (*db).ReparseTargetMaximumLength = (i * 2) as c::WORD;
         (*db).ReparseTargetLength = ((i - 1) * 2) as c::WORD;

src/bootstrap/Cargo.lock

@@ -45,6 +45,7 @@ dependencies = [
  "hex",
  "ignore",
  "is-terminal",
+ "junction",
  "libc",
  "object",
  "once_cell",
@@ -349,6 +350,16 @@ version = "1.0.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "112c678d4050afce233f4f2852bb2eb519230b3cf12f33585275537d7e41578d"
 
+[[package]]
+name = "junction"
+version = "1.0.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ca39ef0d69b18e6a2fd14c2f0a1d593200f4a4ed949b240b5917ab51fac754cb"
+dependencies = [
+ "scopeguard",
+ "winapi",
+]
+
 [[package]]
 name = "lazy_static"
 version = "1.4.0"

src/bootstrap/Cargo.toml

@@ -61,6 +61,9 @@ sysinfo = { version = "0.26.0", optional = true }
 [target.'cfg(not(target_os = "solaris"))'.dependencies]
 fd-lock = "3.0.8"
 
+[target.'cfg(windows)'.dependencies.junction]
+version = "1.0.0"
+
 [target.'cfg(windows)'.dependencies.windows]
 version = "0.46.0"
 features = [

src/bootstrap/util.rs

@@ -146,106 +146,9 @@ pub fn symlink_dir(config: &Config, src: &Path, dest: &Path) -> io::Result<()> {
         fs::symlink(src, dest)
     }
 
-    // Creating a directory junction on windows involves dealing with reparse
-    // points and the DeviceIoControl function, and this code is a skeleton of
-    // what can be found here:
-    //
-    // http://www.flexhex.com/docs/articles/hard-links.phtml
     #[cfg(windows)]
     fn symlink_dir_inner(target: &Path, junction: &Path) -> io::Result<()> {
-        use std::ffi::OsStr;
-        use std::os::windows::ffi::OsStrExt;
-
-        use windows::{
-            core::PCWSTR,
-            Win32::Foundation::{CloseHandle, HANDLE},
-            Win32::Storage::FileSystem::{
-                CreateFileW, FILE_ACCESS_FLAGS, FILE_FLAG_BACKUP_SEMANTICS,
-                FILE_FLAG_OPEN_REPARSE_POINT, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE,
-                MAXIMUM_REPARSE_DATA_BUFFER_SIZE, OPEN_EXISTING,
-            },
-            Win32::System::Ioctl::FSCTL_SET_REPARSE_POINT,
-            Win32::System::SystemServices::{GENERIC_WRITE, IO_REPARSE_TAG_MOUNT_POINT},
-            Win32::System::IO::DeviceIoControl,
-        };
-
-        #[allow(non_snake_case)]
-        #[repr(C)]
-        struct REPARSE_MOUNTPOINT_DATA_BUFFER {
-            ReparseTag: u32,
-            ReparseDataLength: u32,
-            Reserved: u16,
-            ReparseTargetLength: u16,
-            ReparseTargetMaximumLength: u16,
-            Reserved1: u16,
-            ReparseTarget: u16,
-        }
-
-        fn to_u16s<S: AsRef<OsStr>>(s: S) -> io::Result<Vec<u16>> {
-            Ok(s.as_ref().encode_wide().chain(Some(0)).collect())
-        }
-
-        // We're using low-level APIs to create the junction, and these are more
-        // picky about paths. For example, forward slashes cannot be used as a
-        // path separator, so we should try to canonicalize the path first.
-        let target = fs::canonicalize(target)?;
-
-        fs::create_dir(junction)?;
-
-        let path = to_u16s(junction)?;
-
-        let h = unsafe {
-            CreateFileW(
-                PCWSTR(path.as_ptr()),
-                FILE_ACCESS_FLAGS(GENERIC_WRITE),
-                FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                None,
-                OPEN_EXISTING,
-                FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS,
-                HANDLE::default(),
-            )
-        }
-        .map_err(|_| io::Error::last_os_error())?;
-
-        unsafe {
-            #[repr(C, align(8))]
-            struct Align8<T>(T);
-            let mut data = Align8([0u8; MAXIMUM_REPARSE_DATA_BUFFER_SIZE as usize]);
-            let db = data.0.as_mut_ptr() as *mut REPARSE_MOUNTPOINT_DATA_BUFFER;
-            let buf = core::ptr::addr_of_mut!((*db).ReparseTarget) as *mut u16;
-            let mut i = 0;
-            // FIXME: this conversion is very hacky
-            let v = br"\??\";
-            let v = v.iter().map(|x| *x as u16);
-            for c in v.chain(target.as_os_str().encode_wide().skip(4)) {
-                *buf.offset(i) = c;
-                i += 1;
-            }
-            *buf.offset(i) = 0;
-            i += 1;
-
-            (*db).ReparseTag = IO_REPARSE_TAG_MOUNT_POINT;
-            (*db).ReparseTargetMaximumLength = (i * 2) as u16;
-            (*db).ReparseTargetLength = ((i - 1) * 2) as u16;
-            (*db).ReparseDataLength = ((*db).ReparseTargetLength + 12) as u32;
-
-            let mut ret = 0u32;
-            DeviceIoControl(
-                h,
-                FSCTL_SET_REPARSE_POINT,
-                Some(db.cast()),
-                (*db).ReparseDataLength + 8,
-                None,
-                0,
-                Some(&mut ret),
-                None,
-            )
-            .ok()
-            .map_err(|_| io::Error::last_os_error())?;
-        }
-
-        unsafe { CloseHandle(h) };
-        Ok(())
+        junction::create(&target, &junction)
     }
 }