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

Bug in UNIX Domain Socket connect handling when addrlen is smaller than sockaddr_un size #154

Open
Stone749990226 opened this issue Dec 5, 2024 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@Stone749990226
Copy link

Description:

There is a bug in the handling of UNIX domain sockets in the connect method. According to the Linux man page for UNIX domain sockets ([man7.org - unix.7](https://man7.org/linux/man-pages/man7/unix.7.html)), the sockaddr_un structure can be passed with a length smaller than the full size of the structure. For example, when using the connect system call in SSH, a length of 24 bytes is passed instead of the full size of 110 bytes for sockaddr_un.

However, the current implementation in the connect method assumes that the addrlen parameter must always be equal to sizeof(sockaddr_un). This can cause issues if the length is smaller than the structure size.

Code Context:

In the current implementation, specifically in the UNIX socket handling part, the addrlen is not properly validated against the actual length of the sockaddr_un structure:

fn connect(
    &self,
    socket_addr: *const ctypes::sockaddr,
    addrlen: ctypes::socklen_t,
) -> LinuxResult {
    match self {
        Socket::Unix(socket) => {
            if socket_addr.is_null() {
                return Err(LinuxError::EFAULT);
            }
            // Validation for addrlen being equal to sizeof(sockaddr_un) is commented out:
            // if addrlen != size_of::<ctypes::sockaddr_un>() as _ {
            //     return Err(LinuxError::EINVAL);
            // }
            Ok(socket
                .lock()
                .connect(addrun_convert(socket_addr as *const ctypes::sockaddr_un))?)
        }
    }
}

Problem:

  • Linux's behavior: The Linux man page explicitly states that the length of the sockaddr_un structure can be smaller than its full size, depending on the actual address provided. For instance, SSH uses a length of 24 bytes instead of the full 110 bytes.
  • Current code: The code currently assumes that addrlen must match the full size of sockaddr_un, but this is not always true, leading to potential misbehavior when the length is smaller.

Proposed Solution:

  • Modify the validation logic to allow for cases where addrlen is smaller than sizeof(sockaddr_un): Modify the validation logic to check that addrlen is at least offsetof(struct sockaddr_un, sun_path) + strlen(sun_path) + 1, instead of checking for the full size of sockaddr_un. offsetof(struct sockaddr_un, sun_path) returns 2, which is the offset of sun_path within the structure. strlen(sun_path) is the actual length of the provided path, excluding the trailing null character (\0). The + 1 is to account for the null-terminator (\0), which marks the end of the string.

This will align the implementation with Linux's expected behavior for UNIX domain sockets.

Reference:


Additional Notes:

  • This change is essential for correct behavior when interacting with UNIX domain sockets where the provided address length is smaller than the full sockaddr_un structure size, as specified in the man pages.
  • Please verify if similar issues exist in other parts of the codebase where addrlen is used with sockaddr_un.

@Stone749990226 Stone749990226 self-assigned this Dec 5, 2024
@Stone749990226 Stone749990226 added the bug Something isn't working label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant