Unsound usages of unsafe implementation about c_void

[llooFlashooll]

Hi, I am scanning this crate in the latest version using my own static analyzer tool.

Unsafe pointer conversion is found at:

pub fn userauth_keyboard_interactive<P: KeyboardInteractivePrompt>(
        &self,
        username: &str,
        prompter: &mut P,
    ) -> Result<(), Error> {
        // hold on to your hats, this is a bit involved.
        // The keyboard interactive callback is a bit tricksy, and we want to wrap the
        // raw C types with something a bit safer and more ergonomic.
        // Since the interface is defined in terms of a simple function pointer, wrapping
        // is a bit awkward.
        //
        // The session struct has an abstrakt pointer reserved for
        // the user of the embedding application, and that pointer is passed to the
        // prompt callback. We can use this to store a pointer to some state so that
        // we can manage the conversion.
        //
        // The prompts and responses are defined to be UTF-8, but we use from_utf8_lossy
        // to avoid panics in case the server isn't conformant for whatever reason.
        extern "C" fn prompt<P: KeyboardInteractivePrompt>(
            username: *const c_char,
            username_len: c_int,
            instruction: *const c_char,
            instruction_len: c_int,
            num_prompts: c_int,
            prompts: *const raw::LIBSSH2_USERAUTH_KBDINT_PROMPT,
            responses: *mut raw::LIBSSH2_USERAUTH_KBDINT_RESPONSE,
            abstrakt: *mut *mut c_void,
        ) {
            use std::panic::{catch_unwind, AssertUnwindSafe};
            // Catch panics; we can't let them unwind to C code.
            // There's not much to be done with them though because the
            // signature of the callback doesn't allow reporting an error.
            let _ = catch_unwind(AssertUnwindSafe(|| {
                if prompts.is_null() || responses.is_null() || num_prompts < 0 {
                    return;
                }

                let prompter = unsafe { &mut **(abstrakt as *mut *mut P) };

                let username = if !username.is_null() && username_len >= 0 {
                    let username = unsafe {
                        slice::from_raw_parts(username as *const u8, username_len as usize)
                    };
                    String::from_utf8_lossy(username)
                } else {
                    Cow::Borrowed("")
                };

                let instruction = if !instruction.is_null() && instruction_len >= 0 {
                    let instruction = unsafe {
                        slice::from_raw_parts(instruction as *const u8, instruction_len as usize)
                    };
                    String::from_utf8_lossy(instruction)
                } else {
                    Cow::Borrowed("")
                };

                let prompts = unsafe { slice::from_raw_parts(prompts, num_prompts as usize) };
                let responses =
                    unsafe { slice::from_raw_parts_mut(responses, num_prompts as usize) };

                let prompts: Vec<Prompt> = prompts
                    .iter()
                    .map(|item| {
                        let data = unsafe {
                            slice::from_raw_parts(item.text as *const u8, item.length as usize)
                        };
                        Prompt {
                            text: String::from_utf8_lossy(data),
                            echo: item.echo != 0,
                        }
                    })
                    .collect();

                // libssh2 wants to be able to free(3) the response strings, so allocate
                // storage and copy the responses into appropriately owned memory.
                // We can't simply call strdup(3) here because the rust string types
                // are not NUL terminated.
                fn strdup_string(s: &str) -> *mut c_char {
                    let len = s.len();
                    let ptr = unsafe { libc::malloc(len + 1) as *mut c_char };
                    if !ptr.is_null() {
                        unsafe {
                            ::std::ptr::copy_nonoverlapping(
                                s.as_bytes().as_ptr() as *const c_char,
                                ptr,
                                len,
                            );
                            *ptr.offset(len as isize) = 0;
                        }
                    }
                    ptr
                }

                for (i, response) in (*prompter)
                    .prompt(&username, &instruction, &prompts)
                    .into_iter()
                    .take(prompts.len())
                    .enumerate()
                {
                    let ptr = strdup_string(&response);
                    if !ptr.is_null() {
                        responses[i].length = response.len() as c_uint;
                    } else {
                        responses[i].length = 0;
                    }
                    responses[i].text = ptr;
                }
            }));
        }

        let username = CString::new(username)?;
        let username = username.as_bytes();
        let inner = self.inner();
        unsafe {
            with_abstract(inner.raw, prompter as *mut P as *mut c_void, || {
                inner.rc(raw::libssh2_userauth_keyboard_interactive_ex(
                    inner.raw,
                    username.as_ptr() as *const _,
                    username.len() as c_uint,
                    Some(prompt::<P>),
                ))
            })
        }
    }

This unsound implementation would create memory issues such as overflow, underflow, or misalignment. The attacker can manipulate the argument prompter associated with the c_void pointer with an unexpected type or layout, which can lead to an out-of-bounds memory access bug. The c_void pointer is passed through the FFI (libssh2_userauth_keyboard_interactive_ex) and later recovered inside the callback, which can further corrupt the C/C++ code.

This would cause undefined behaviors in Rust. Adversaries can manipulate the associated argument to cause memory safety bugs. I am reporting this issue for your attention.

[yodaldevoid]

Can you point to a precise cast or series of casts that is unsound?

I don't see how an attacker would be able to create an "unexpected type or layout" without first breaking safety and soundness guarantees further up the chain.

As for the c_void pointer passed through the callback, there is no issue there. It is common practice to pass data through a void* in C, and the expectation is that the library will do nothing with it other than pass it along to the callback. We can't do anything if the library doesn't stick its promised functionality.

Without further information I can't see an issue here.