Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible Buffer-Overflow when sending control messages with sendmmsg #2176

Open
Jan561 opened this issue Nov 4, 2023 · 2 comments
Open

Possible Buffer-Overflow when sending control messages with sendmmsg #2176

Jan561 opened this issue Nov 4, 2023 · 2 comments

Comments

@Jan561
Copy link
Contributor

Jan561 commented Nov 4, 2023

pub fn sendmmsg<'a, C, S>(
    // elided ...
    data: &'a mut MultiHeaders<S>,
    cmsgs: C,
) 
where
    C: AsRef<[ControlMessage<'a>]>,
{

    for mmsghdr in data.items.iter_mut().enumerate() {
        let p = &mut mmsghdr.msg_hdr;

        let mut pmhdr: *mut cmsghdr = unsafe { CMSG_FIRSTHDR(p) };
        for cmsg in cmsgs.as_ref() {
            assert_ne!(pmhdr, ptr::null_mut());

            // This is unsound: it relies on the caller having initialized the control
            // message buffer inside `data` with enough space for holding all
            // control messages inside `cmsgs`, i.e. having used the `cmsg_space!`
            // macro properly in conjunction with `MultiHeaders::preallocate`. Since
            // the macro, the constructor of `MultiHeaders` and this function are
            // all safe to call, unsafe code must not rely on this invariant to be
            // upheld by the caller.
            //
            // Even the above assertion won't catch this bug, as there might
            // be enough space left to send *a* control message, different from `cmsg`.
            //
            // Original comment:
            //
            // > Safe because we know that pmhdr is valid, and we initialized it with
            // > sufficient space

            unsafe { cmsg.encode_into(pmhdr) };

            pmhdr = unsafe { CMSG_NXTHDR(p, pmhdr) };
        }
    }

    // ...
}

The security impact of this issue is probably negligible, as

  • the control message buffer inside data must have a non-zero length and the majority of the users of this function just want to reduce the amount of syscalls and don't need control messages,
  • an attacker would have to be able to influence which control messages are being sent, which is a very odd and unlikely scenario, or the program must be buggy in first place and initialize the control message buffer too small.
@asomers
Copy link
Member

asomers commented Nov 4, 2023

sendmmsg always makes me want to quit my programming job and pursue a second career as a swineherd. Before going further, could you please tell me if this bug was introduced by #2120 ?

@Jan561
Copy link
Contributor Author

Jan561 commented Nov 4, 2023

No, this bug already existed before

@Jan561 Jan561 mentioned this issue Nov 23, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants