Skip to content

Commit

Permalink
Rework UnixAddr to fix soundness issues
Browse files Browse the repository at this point in the history
  • Loading branch information
coolreader18 committed Aug 25, 2021
1 parent dab7332 commit 460842a
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 61 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ This project adheres to [Semantic Versioning](https://semver.org/).
- Minimum supported Rust version is now 1.46.0.
([#1492](https://github.com/nix-rust/nix/pull/1492))

- Rework `UnixAddr` to encapsulate internals better in order to fix soundness
issues. No longer allows creating a `UnixAddr` from a raw `sockaddr_un`.
([#1496](https://github.com/nix-rust/nix/pull/1496))

### Fixed

- Added more errno definitions for better backwards compatibility with
Expand Down
141 changes: 92 additions & 49 deletions src/sys/socket/addr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,15 +515,42 @@ impl fmt::Display for Ipv6Addr {
}

/// A wrapper around `sockaddr_un`.
///
/// This also tracks the length of `sun_path` address (excluding
/// a terminating null), because it may not be null-terminated. For example,
/// unconnected and Linux abstract sockets are never null-terminated, and POSIX
/// does not require that `sun_len` include the terminating null even for normal
/// sockets. Note that the actual sockaddr length is greater by
/// `offset_of!(libc::sockaddr_un, sun_path)`
#[derive(Clone, Copy, Debug)]
pub struct UnixAddr(pub libc::sockaddr_un, pub usize);
pub struct UnixAddr {
// INVARIANT: sun & path_len are valid as defined by docs for from_raw_parts
sun: libc::sockaddr_un,
path_len: usize
}

// linux man page unix(7) says there are 3 kinds of unix socket:
// pathname: addrlen = offsetof(struct sockaddr_un, sun_path) + strlen(sun_path) + 1
// unnamed: addrlen = sizeof(sa_family_t)
// abstract: addren > sizeof(sa_family_t), name = sun_path[..(addrlen - sizeof(sa_family_t))]
//
// what we call path_len = addrlen - offsetof(struct sockaddr_un, sun_path)
#[derive(PartialEq, Eq, Hash)]
enum UnixAddrKind<'a> {
Pathname(&'a Path),
Unnamed,
#[cfg(any(target_os = "android", target_os = "linux"))]
Abstract(&'a [u8]),
}
impl<'a> UnixAddrKind<'a> {
/// Safety: sun & path_len must be valid
unsafe fn get(sun: &'a libc::sockaddr_un, path_len: usize) -> Self {
if path_len == 0 {
return Self::Unnamed;
}
#[cfg(any(target_os = "android", target_os = "linux"))]
if sun.sun_path[0] == 0 {
let name = slice::from_raw_parts(sun.sun_path.as_ptr().add(1) as *const u8, path_len - 1);
return Self::Abstract(name);
}
let pathname = slice::from_raw_parts(sun.sun_path.as_ptr() as *const u8, path_len - 1);
Self::Pathname(Path::new(OsStr::from_bytes(pathname)))
}
}


impl UnixAddr {
/// Create a new sockaddr_un representing a filesystem path.
Expand All @@ -537,15 +564,15 @@ impl UnixAddr {

let bytes = cstr.to_bytes();

if bytes.len() > ret.sun_path.len() {
if bytes.len() >= ret.sun_path.len() {
return Err(Error::from(Errno::ENAMETOOLONG));
}

ptr::copy_nonoverlapping(bytes.as_ptr(),
ret.sun_path.as_mut_ptr() as *mut u8,
bytes.len());

Ok(UnixAddr(ret, bytes.len()))
Ok(UnixAddr::from_raw_parts(ret, bytes.len() + 1))
}
})?
}
Expand All @@ -564,7 +591,7 @@ impl UnixAddr {
.. mem::zeroed()
};

if path.len() + 1 > ret.sun_path.len() {
if path.len() >= ret.sun_path.len() {
return Err(Error::from(Errno::ENAMETOOLONG));
}

Expand All @@ -574,28 +601,33 @@ impl UnixAddr {
ret.sun_path.as_mut_ptr().offset(1) as *mut u8,
path.len());

Ok(UnixAddr(ret, path.len() + 1))
Ok(UnixAddr::from_raw_parts(ret, path.len() + 1))
}
}

fn sun_path(&self) -> &[u8] {
unsafe { slice::from_raw_parts(self.0.sun_path.as_ptr() as *const u8, self.1) }
/// Create a UnixAddr from a raw `sockaddr_un` struct and a size. `path_len` is the "addrlen"
/// of this address, but minus `offsetof(struct sockaddr_un, sun_path)`. Basically the length
/// of the data in `sun_path`.
///
/// # Safety
/// This pair of sockaddr_un & path_len must be a valid unix addr, which means:
/// - path_len <= sockaddr_un.sun_path.len()
/// - if this is a unix addr with a pathname, sun.sun_path is a nul-terminated fs path and
/// sun.sun_path[path_len - 1] == 0
pub(crate) unsafe fn from_raw_parts(sun: libc::sockaddr_un, path_len: usize) -> UnixAddr {
UnixAddr { sun, path_len }
}

fn kind(&self) -> UnixAddrKind<'_> {
// SAFETY: our sockaddr is always valid because of the invariant on the struct
unsafe { UnixAddrKind::get(&self.sun, self.path_len) }
}

/// If this address represents a filesystem path, return that path.
pub fn path(&self) -> Option<&Path> {
if self.1 == 0 || self.0.sun_path[0] == 0 {
// unnamed or abstract
None
} else {
let p = self.sun_path();
// POSIX only requires that `sun_len` be at least long enough to
// contain the pathname, and it need not be null-terminated. So we
// need to create a string that is the shorter of the
// null-terminated length or the full length.
let ptr = &self.0.sun_path as *const libc::c_char;
let reallen = unsafe { libc::strnlen(ptr, p.len()) };
Some(Path::new(<OsStr as OsStrExt>::from_bytes(&p[..reallen])))
match self.kind() {
UnixAddrKind::Pathname(path) => Some(path),
_ => None
}
}

Expand All @@ -605,39 +637,53 @@ impl UnixAddr {
/// leading null byte. `None` is returned for unnamed or path-backed sockets.
#[cfg(any(target_os = "android", target_os = "linux"))]
pub fn as_abstract(&self) -> Option<&[u8]> {
if self.1 >= 1 && self.0.sun_path[0] == 0 {
Some(&self.sun_path()[1..])
} else {
// unnamed or filesystem path
None
match self.kind() {
UnixAddrKind::Abstract(name) => Some(name),
_ => None
}
}

/// Returns the addrlen of this socket - `offsetof(struct sockaddr_un, sun_path)`
#[inline]
pub fn path_len(&self) -> usize {
self.path_len
}
/// Returns a pointer to the raw `sockaddr_un` struct
#[inline]
pub fn as_ptr(&self) -> *const libc::sockaddr_un {
&self.sun
}
/// Returns a mutable pointer to the raw `sockaddr_un` struct
#[inline]
pub fn as_mut_ptr(&mut self) -> *mut libc::sockaddr_un {
&mut self.sun
}
}

impl fmt::Display for UnixAddr {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
if self.1 == 0 {
f.write_str("<unbound UNIX socket>")
} else if let Some(path) = self.path() {
path.display().fmt(f)
} else {
let display = String::from_utf8_lossy(&self.sun_path()[1..]);
write!(f, "@{}", display)
match self.kind() {
UnixAddrKind::Pathname(path) => path.display().fmt(f),
UnixAddrKind::Unnamed => f.pad("<unbound UNIX socket>"),
#[cfg(any(target_os = "android", target_os = "linux"))]
UnixAddrKind::Abstract(name) => {
write!(f, "@{}", String::from_utf8_lossy(name))
}
}
}
}

impl PartialEq for UnixAddr {
fn eq(&self, other: &UnixAddr) -> bool {
self.sun_path() == other.sun_path()
self.kind() == other.kind()
}
}

impl Eq for UnixAddr {}

impl Hash for UnixAddr {
fn hash<H: Hasher>(&self, s: &mut H) {
( self.0.sun_family, self.sun_path() ).hash(s)
self.kind().hash(s)
}
}

Expand Down Expand Up @@ -805,12 +851,12 @@ impl SockAddr {
},
mem::size_of_val(addr) as libc::socklen_t
),
SockAddr::Unix(UnixAddr(ref addr, len)) => (
SockAddr::Unix(UnixAddr { ref sun, path_len }) => (
// This cast is always allowed in C
unsafe {
&*(addr as *const libc::sockaddr_un as *const libc::sockaddr)
&*(sun as *const libc::sockaddr_un as *const libc::sockaddr)
},
(len + offset_of!(libc::sockaddr_un, sun_path)) as libc::socklen_t
(path_len + offset_of!(libc::sockaddr_un, sun_path)) as libc::socklen_t
),
#[cfg(any(target_os = "android", target_os = "linux"))]
SockAddr::Netlink(NetlinkAddr(ref sa)) => (
Expand Down Expand Up @@ -1376,11 +1422,8 @@ mod tests {
let name = String::from("nix\0abstract\0test");
let addr = UnixAddr::new_abstract(name.as_bytes()).unwrap();

let sun_path1 = addr.sun_path();
let sun_path2 = [0u8, 110, 105, 120, 0, 97, 98, 115, 116, 114, 97, 99, 116, 0, 116, 101, 115, 116];
assert_eq!(sun_path1.len(), sun_path2.len());
for i in 0..sun_path1.len() {
assert_eq!(sun_path1[i], sun_path2[i]);
}
let sun_path1 = unsafe { &(*addr.as_ptr()).sun_path[..addr.path_len()] };
let sun_path2 = [0, 110, 105, 120, 0, 97, 98, 115, 116, 114, 97, 99, 116, 0, 116, 101, 115, 116];
assert_eq!(sun_path1, sun_path2);
}
}
10 changes: 5 additions & 5 deletions src/sys/socket/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1774,7 +1774,7 @@ pub fn getsockname(fd: RawFd) -> Result<SockAddr> {
/// In C this would usually be done by casting. The `len` argument
/// should be the number of bytes in the `sockaddr_storage` that are actually
/// allocated and valid. It must be at least as large as all the useful parts
/// of the structure. Note that in the case of a `sockaddr_un`, `len` need not
/// of the structure. Note that in the case of a `sockaddr_un`, `len` should
/// include the terminating null.
pub fn sockaddr_storage_to_addr(
addr: &sockaddr_storage,
Expand Down Expand Up @@ -1802,10 +1802,10 @@ pub fn sockaddr_storage_to_addr(
}
libc::AF_UNIX => {
let pathlen = len - offset_of!(sockaddr_un, sun_path);
let sun = unsafe {
*(addr as *const _ as *const sockaddr_un)
};
Ok(SockAddr::Unix(UnixAddr(sun, pathlen)))
unsafe {
let sun = *(addr as *const _ as *const sockaddr_un);
Ok(SockAddr::Unix(UnixAddr::from_raw_parts(sun, pathlen)))
}
}
#[cfg(any(target_os = "android", target_os = "linux"))]
libc::AF_PACKET => {
Expand Down
13 changes: 6 additions & 7 deletions test/sys/test_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ pub fn test_path_to_sock_addr() {
let addr = UnixAddr::new(actual).unwrap();

let expect: &[c_char] = unsafe {
slice::from_raw_parts(path.as_bytes().as_ptr() as *const c_char, path.len())
slice::from_raw_parts(path.as_ptr() as *const c_char, path.len())
};
assert_eq!(&addr.0.sun_path[..8], expect);
assert_eq!(unsafe { &(*addr.as_ptr()).sun_path[..8] }, expect);

assert_eq!(addr.path(), Some(actual));
}
Expand All @@ -133,7 +133,7 @@ pub fn test_addr_equality_path() {
let addr1 = UnixAddr::new(actual).unwrap();
let mut addr2 = addr1;

addr2.0.sun_path[10] = 127;
unsafe { (*addr2.as_mut_ptr()).sun_path[10] = 127 };

assert_eq!(addr1, addr2);
assert_eq!(calculate_hash(&addr1), calculate_hash(&addr2));
Expand All @@ -157,7 +157,7 @@ pub fn test_addr_equality_abstract() {
assert_eq!(addr1, addr2);
assert_eq!(calculate_hash(&addr1), calculate_hash(&addr2));

addr2.0.sun_path[17] = 127;
unsafe { (*addr2.as_mut_ptr()).sun_path[17] = 127 };
assert_ne!(addr1, addr2);
assert_ne!(calculate_hash(&addr1), calculate_hash(&addr2));
}
Expand All @@ -180,7 +180,7 @@ pub fn test_abstract_uds_addr() {
assert_eq!(addr.path(), None);

// Internally, name is null-prefixed (abstract namespace)
assert_eq!(addr.0.sun_path[0], 0);
assert_eq!(unsafe { (*addr.as_ptr()).sun_path[0] }, 0);
}

#[test]
Expand All @@ -194,8 +194,7 @@ pub fn test_getsockname() {
.expect("socket failed");
let sockaddr = SockAddr::new_unix(&sockname).unwrap();
bind(sock, &sockaddr).expect("bind failed");
assert_eq!(sockaddr.to_string(),
getsockname(sock).expect("getsockname failed").to_string());
assert_eq!(sockaddr, getsockname(sock).expect("getsockname failed"));
}

#[test]
Expand Down

0 comments on commit 460842a

Please sign in to comment.