Design: Arithmetic Overflow in the wrapper, how do we deal with it?

[a4lg]

Some wrappers inside the windows crate takes one or more slices as arguments.

What I concern is, on current windows crate, slice.len() is converted to another type without checking validity against the target type. As a result, it sometimes causes arithmetic overflow.

For instance, MultiByteToWideChar Win32 API uses int as the length type.

Quoting crates/libs/windows/src/Windows/Win32/Globalization/mod.rs:

#[inline]
pub unsafe fn MultiByteToWideChar(codepage: u32, dwflags: MULTI_BYTE_TO_WIDE_CHAR_FLAGS, lpmultibytestr: &[u8], lpwidecharstr: ::core::option::Option<&mut [u16]>) -> i32 {
    ::windows::imp::link ! ( "kernel32.dll""system" fn MultiByteToWideChar ( codepage : u32 , dwflags : MULTI_BYTE_TO_WIDE_CHAR_FLAGS , lpmultibytestr : :: windows::core::PCSTR , cbmultibyte : i32 , lpwidecharstr : :: windows::core::PWSTR , cchwidechar : i32 ) -> i32 );
    MultiByteToWideChar(codepage, dwflags, ::core::mem::transmute(lpmultibytestr.as_ptr()), lpmultibytestr.len() as _, ::core::mem::transmute(lpwidecharstr.as_deref().map_or(::core::ptr::null(), |slice| slice.as_ptr())), lpwidecharstr.as_deref().map_or(0, |slice| slice.len() as _))
}

OK, if we overflow i32, that would be a problem.

Following test code intentionally exploits this and try to pass 0x1_0000_0003-byte sized buffer to MultiByteToWideChar. It is handled as 3-byte sized buffer in the wrapper and the result is corrupted.

For windows-sys crate, I don't find any issues. The user must handle such conditions to use the API safely. However, in windows crate, slice length type is hidden inside the wrapper and there's no safety measures.

I'm not sure what to do but I can say that some decision making will be required.

// Run this code on the 64-bit environment!

use windows::Win32::Globalization::*;
use windows::Win32::Foundation::*;

fn main() {
    // Japanese Shift_JIS (Microsoft variant)
    const CODE_PAGE: u32 = 932;
    // Your current ANSI code page (if 932 does not work)
    // const CODE_PAGE: u32 = CP_ACP;

    // If the arithmetic overflow occurs, this pattern is truncated to the first 3 bytes
    // because the final buffer size is 0x1_0000_0003.
    // Note 1:
    // \x83\x5c in Shift_JIS (code page 932) is Katakana SO (U+30BD).
    // Note 2:
    // corrupt \x83 in Shift_JIS (code page 932) is confirmed to be converted to
    // a middle dot (U+30FB), in this case it means a corrupted byte.
    let repeating_pattern: &[u8; 7] = b"AB\x83\x5cCDE";

    // Single repeating pattern
    let buffer_repeating_pattern: Vec<u16>;
    unsafe {
        let len = MultiByteToWideChar(CODE_PAGE, MULTI_BYTE_TO_WIDE_CHAR_FLAGS(0), repeating_pattern.as_slice(), None);
        if len == 0 {
            eprintln!("MultiByteToWideChar 1:1 error: {:?}", GetLastError());
            std::process::exit(1);
        }
        let mut tmp_buffer: Vec<u16> = core::iter::repeat(0u16).take(usize::try_from(len).unwrap()).collect();
        let len = MultiByteToWideChar(CODE_PAGE, MULTI_BYTE_TO_WIDE_CHAR_FLAGS(0), repeating_pattern.as_slice(), Some(tmp_buffer.as_mut_slice()));
        if len == 0 {
            eprintln!("MultiByteToWideChar 1:2 error: {:?}", GetLastError());
            std::process::exit(1);
        }
        buffer_repeating_pattern = tmp_buffer;
    }

    // Repeated until the buffer is larger than 4GiB.
    let mut buffer = Vec::<u8>::new();
    while buffer.len() < 0x1_0000_0000 {
        buffer.extend_from_slice(repeating_pattern);
    }
    let buffer_overflow_repeated: Vec<u16>;
    unsafe {
        // FAILURE EXPECTED HERE BECAUSE OF AN ARITHMETIC OVERFLOW CALLING THE WIN32 API.
        let len = MultiByteToWideChar(CODE_PAGE, MULTI_BYTE_TO_WIDE_CHAR_FLAGS(0), buffer.as_slice(), None);
        if len == 0 {
            eprintln!("MultiByteToWideChar 2:1 error: {:?}", GetLastError());
            std::process::exit(1);
        }
        let mut tmp_buffer: Vec<u16> = core::iter::repeat(0u16).take(usize::try_from(len).unwrap()).collect();
        let len = MultiByteToWideChar(CODE_PAGE, MULTI_BYTE_TO_WIDE_CHAR_FLAGS(0), buffer.as_slice(), Some(tmp_buffer.as_mut_slice()));
        if len == 0 {
            eprintln!("MultiByteToWideChar 2:2 error: {:?}", GetLastError());
            std::process::exit(1);
        }
        buffer_overflow_repeated = tmp_buffer;
    }

    let s_repeating_pattern = String::from_utf16_lossy(&buffer_repeating_pattern.as_slice());
    let s_overflow_repeated = String::from_utf16_lossy(&buffer_overflow_repeated.as_slice());

    println!("Repeating pattern:\n\t{s_repeating_pattern:?}");
    println!("Overflowing pattern (repeated the first pattern until the string exceeds 4GiB):\n\t{s_overflow_repeated:?}");
}

Additional Keywords: (potential) security, integer overflow

[kennykerr]

Yes, I think we should probably use TryFrom and panic in cases where it is impractical to propagate the error.

[ahcodedthat]

It might be better to wrap the slice in a newtype that checks the maximum length and returns an error of its own if the slice is too large. Something like:

pub struct WSlice<'a, T> {
    pub(crate) slice: *const T,
    pub(crate) len: i32,
    _phantom_slice: PhantomData<&'a [T]>,
}

impl<'a, T> TryFrom<&'a [T]> for WSlice<'a, T> {
    type Error = SliceTooLongError;

    fn try_from(slice: &'a [T]) -> Result<Self, Self::Error> {
        let len = i32::try_from(slice.len()).map_err(|_| SliceTooLongError)?;
        let slice = slice.as_ptr();
        Ok(Self { slice, len, _phantom_slice: PhantomData })
    }
}

Then, change APIs that currently take slice references to take &WSlice<T> instead of &[T].

This has the upside of no panics and no misbehavior, but the downside that it's more difficult to use.

---

Note that there are some cases where it's acceptable to just use i32::MAX if the slice is too long. For example, GetClipboardFormatName can't produce a clipboard format name longer than i32::MAX characters, and unless I'm mistaken clipboard format names can't be longer than that anyway, so if the slice is longer than that, nothing bad will happen if i32::MAX is passed to GetClipboardFormatName instead of the real slice length.

[a4lg]

My first idea was to fix this is to use regular slices and change the return type to Result<i32, SOME_NEW_ERROR_TYPE> but @ahcodedthat's idea looks less breaking and easier to understand what have happened.

Yes, creating safe error somewhere is required but there's many of possible choices without making panics ...and that's why I'm not sure what to do (because no matter what we change, that would be a breaking change; I don't support making a panic in such easy consequences but the fact that the fix will break the compatibility suffers me).

[a4lg]

Ping.

[kennykerr]

I was thinking of turning these into E_INVALIDARG and just returning through the same Error propagation path. Just haven't had time to rough it in but hope to do so soon.

[a4lg]

Thanks for your response!

I understand that the work to avoid this issue wouldn't be easy and will take much time (and most notably, compatibility breaking). As long as this issue is not forgotten, I'm happy with it.

[kennykerr]

Here's a partial fix: #2699