summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG.md4
-rw-r--r--src/sys/socket/addr.rs157
-rw-r--r--src/sys/socket/mod.rs8
-rw-r--r--test/sys/test_socket.rs13
4 files changed, 122 insertions, 60 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index eb1ec569..0d3e513a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -74,6 +74,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
diff --git a/src/sys/socket/addr.rs b/src/sys/socket/addr.rs
index ca77ad41..38dd4190 100644
--- a/src/sys/socket/addr.rs
+++ b/src/sys/socket/addr.rs
@@ -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.
@@ -537,7 +564,7 @@ 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));
}
@@ -545,7 +572,7 @@ impl UnixAddr {
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))
}
})?
}
@@ -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));
}
@@ -574,28 +601,39 @@ 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))
+ }
+ }
+
+ /// 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 || sun.sun_path[path_len] == 0
+ pub(crate) unsafe fn from_raw_parts(sun: libc::sockaddr_un, mut path_len: usize) -> UnixAddr {
+ if let UnixAddrKind::Pathname(_) = UnixAddrKind::get(&sun, path_len) {
+ if sun.sun_path[path_len - 1] != 0 {
+ assert_eq!(sun.sun_path[path_len], 0);
+ path_len += 1
+ }
}
+ UnixAddr { sun, path_len }
}
- fn sun_path(&self) -> &[u8] {
- unsafe { slice::from_raw_parts(self.0.sun_path.as_ptr() as *const u8, self.1) }
+ 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,
}
}
@@ -605,31 +643,55 @@ 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
+ }
+}
+
+#[cfg(any(target_os = "android", target_os = "linux"))]
+fn fmt_abstract(abs: &[u8], f: &mut fmt::Formatter) -> fmt::Result {
+ use fmt::Write;
+ f.write_str("@\"")?;
+ for &b in abs {
+ use fmt::Display;
+ char::from(b).escape_default().fmt(f)?;
+ }
+ f.write_char('"')?;
+ Ok(())
}
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) => fmt_abstract(name, f),
}
}
}
impl PartialEq for UnixAddr {
fn eq(&self, other: &UnixAddr) -> bool {
- self.sun_path() == other.sun_path()
+ self.kind() == other.kind()
}
}
@@ -637,7 +699,7 @@ 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)
}
}
@@ -805,12 +867,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)) => (
@@ -1376,11 +1438,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);
}
}
diff --git a/src/sys/socket/mod.rs b/src/sys/socket/mod.rs
index d4ff7690..96328842 100644
--- a/src/sys/socket/mod.rs
+++ b/src/sys/socket/mod.rs
@@ -1833,10 +1833,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 => {
diff --git a/test/sys/test_socket.rs b/test/sys/test_socket.rs
index 2b0bc831..6b998cb6 100644
--- a/test/sys/test_socket.rs
+++ b/test/sys/test_socket.rs
@@ -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));
}
@@ -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));
@@ -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));
}
@@ -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]
@@ -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]
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-05 17:59:21 +0000