Unsound usages of unsafe implementation about c_void

Inizia da userauth_keyboard_interactive e dal relativo callback di prompt annidato, quindi segui with_abstract e la chiamata FFI a libssh2_userauth_keyboard_interactive_ex. Esamina la conversione di c_void e gli argomenti del callback rispetto ai rischi di sicurezza della memoria segnalati; il lavoro è completato quando il callback non consente più l'interpretazione di puntatori non validi né accessi oltre i limiti, con un'adeguata copertura di regressione.

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.