rust-lang / rust-lang/ssh2-rs

Unsound usages of unsafe implementation about c_void

Open
#357 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
566
Forks
163
Avg merge
4h 51m
Merged PRs (30d)
2

Description

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at userauth_keyboard_interactive and its nested prompt callback, then trace with_abstract and the libssh2_userauth_keyboard_interactive_ex FFI call. Review the c_void conversion and callback arguments for the reported memory-safety risks; done means the callback no longer permits invalid pointer interpretation or out-of-bounds access, with appropriate regression coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.