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

Fix UB in the SO_TYPE sockopt #1821

Merged
merged 1 commit into from Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -7,6 +7,9 @@ This project adheres to [Semantic Versioning](https://semver.org/).
### Added
### Changed
### Fixed
- Fix UB with `sys::socket::sockopt::SockType` using `SOCK_PACKET`.
([#1821](https://github.com/nix-rust/nix/pull/1821))

### Removed

## [0.26.0] - 2022-11-29
Expand Down
20 changes: 19 additions & 1 deletion src/sys/socket/mod.rs
Expand Up @@ -12,7 +12,7 @@ use libc::{
self, c_int, c_void, iovec, size_t, socklen_t, CMSG_DATA, CMSG_FIRSTHDR,
CMSG_LEN, CMSG_NXTHDR,
};
use std::convert::TryInto;
use std::convert::{TryFrom, TryInto};
use std::io::{IoSlice, IoSliceMut};
#[cfg(feature = "net")]
use std::net;
Expand Down Expand Up @@ -107,6 +107,24 @@ pub enum SockType {
#[cfg(not(any(target_os = "haiku")))]
Rdm = libc::SOCK_RDM,
}
// The TryFrom impl could've been derived using libc_enum!. But for
// backwards-compatibility with Nix-0.25.0 we manually implement it, so as to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asomers I'm interpreting this as driving a patch, is that the intent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean so we can build patch releases? Yes, I think that's worthwhile. But even for new releases too I think it's annoying to force people to change SockType::Stream to SockType::SOCK_STREAM without great reason.

// keep the old variant names.
impl TryFrom<i32> for SockType {
type Error = crate::Error;

fn try_from(x: i32) -> Result<Self> {
match x {
libc::SOCK_STREAM => Ok(Self::Stream),
libc::SOCK_DGRAM => Ok(Self::Datagram),
libc::SOCK_SEQPACKET => Ok(Self::SeqPacket),
libc::SOCK_RAW => Ok(Self::Raw),
#[cfg(not(any(target_os = "haiku")))]
libc::SOCK_RDM => Ok(Self::Rdm),
_ => Err(Errno::EINVAL)
}
}
}

/// Constants used in [`socket`](fn.socket.html) and [`socketpair`](fn.socketpair.html)
/// to specify the protocol to use.
Expand Down
13 changes: 10 additions & 3 deletions src/sys/socket/sockopt.rs
Expand Up @@ -6,7 +6,10 @@ use crate::Result;
use cfg_if::cfg_if;
use libc::{self, c_int, c_void, socklen_t};
use std::ffi::{OsStr, OsString};
use std::mem::{self, MaybeUninit};
use std::{
convert::TryFrom,
mem::{self, MaybeUninit}
};
#[cfg(target_family = "unix")]
use std::os::unix::ffi::OsStrExt;
use std::os::unix::io::RawFd;
Expand Down Expand Up @@ -102,7 +105,10 @@ macro_rules! getsockopt_impl {
);
Errno::result(res)?;

Ok(getter.assume_init())
match <$ty>::try_from(getter.assume_init()) {
Err(_) => Err(Errno::EINVAL),
Ok(r) => Ok(r)
}
}
}
}
Expand Down Expand Up @@ -629,7 +635,8 @@ sockopt_impl!(
GetOnly,
libc::SOL_SOCKET,
libc::SO_TYPE,
super::SockType
super::SockType,
GetStruct<i32>
);
sockopt_impl!(
/// Returns a value indicating whether or not this socket has been marked to
Expand Down
27 changes: 27 additions & 0 deletions test/sys/test_sockopt.rs
Expand Up @@ -151,6 +151,33 @@ fn test_so_tcp_maxseg() {
close(ssock).unwrap();
}

#[test]
fn test_so_type() {
let sockfd = socket(
AddressFamily::Inet,
SockType::Stream,
SockFlag::empty(),
None,
)
.unwrap();

assert_eq!(Ok(SockType::Stream), getsockopt(sockfd, sockopt::SockType));
}

/// getsockopt(_, sockopt::SockType) should gracefully handle unknown socket
/// types. Regression test for https://github.com/nix-rust/nix/issues/1819
#[cfg(any(target_os = "android", target_os = "linux",))]
#[test]
fn test_so_type_unknown() {
use nix::errno::Errno;

require_capability!("test_so_type", CAP_NET_RAW);
let sockfd = unsafe { libc::socket(libc::AF_PACKET, libc::SOCK_PACKET, 0) };
assert!(sockfd >= 0, "Error opening socket: {}", nix::Error::last());

assert_eq!(Err(Errno::EINVAL), getsockopt(sockfd, sockopt::SockType));
}

// The CI doesn't supported getsockopt and setsockopt on emulated processors.
// It's believed that a QEMU issue, the tests run ok on a fully emulated system.
// Current CI just run the binary with QEMU but the Kernel remains the same as the host.
Expand Down