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

Unsoundness: Incorrect safety invariant checks in UnixSocketAddr::from_raw() #17

Closed
Manishearth opened this issue Oct 11, 2023 · 1 comment · Fixed by #18
Closed

Unsoundness: Incorrect safety invariant checks in UnixSocketAddr::from_raw() #17

Manishearth opened this issue Oct 11, 2023 · 1 comment · Fixed by #18

Comments

@Manishearth
Copy link
Contributor

We perform a bunch of invariant checks in UnixSocketAddr::from_raw()

uds/src/addr.rs

Lines 670 to 686 in a596894

if addr.is_null() && len == 0 {
Ok(Self::new_unspecified())
} else if addr.is_null() {
Err(io::Error::new(ErrorKind::InvalidInput, "addr is NULL"))
} else if len < path_offset() {
Err(io::Error::new(ErrorKind::InvalidInput, "address length is too low"))
} else if len > path_offset() + mem::size_of_val(&copy.addr.sun_path) as socklen_t {
Err(io::Error::new(ErrorKind::InvalidInput, TOO_LONG_DESC))
} else if (&*addr).sa_family != AF_UNIX as sa_family_t {
Err(io::Error::new(ErrorKind::InvalidData, "not an unix socket address"))
} else {
let addr = addr as *const sockaddr_un;
let sun_path_ptr = (&*addr).sun_path.as_ptr();
let sun_path = slice::from_raw_parts(sun_path_ptr, len as usize);
copy.addr.sun_path.copy_from_slice(sun_path);
copy.len = len;
Ok(copy)

Specificlaly, we check that path_offset() < len < path_offset() + sizeof(sun_path)

We then call slice::from_raw_parts(sun_path_ptr, len as usize);

This seems incorrect. path_offset is the offset of sun_path in sockaddr_un. The bounds check we need is 0 < len < sizeof(sun_path), since we are indexing directly into sun_path_ptr, not sockaddr_un.

tormol added a commit that referenced this issue Oct 15, 2023
It would do out-of-bounds reads for addresses with length equal to
sizeof(sun_path)-path_offset(), and panic for any other lengths.

It had not been tested, and due to that second part
I assume it has never been used.

Fixes #17.
@tormol
Copy link
Owner

tormol commented Oct 15, 2023

len is supposed to be the length of the whole address including path_offset(), so the slice is too long.
But the function is completely broken, as copy_from_slice() will panic if the slice length is not equal to the length of sun_path!
That means out-of-bounds read will only happen for addresses with a specific, rather long length.
I've apparently never tested it, and since it fails for any reasonable-length address I really doubt anyone else has used it.

@tormol tormol mentioned this issue Oct 15, 2023
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