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

Fix UB in the SO_TYPE sockopt #1821

merged 1 commit into from Nov 29, 2022

Conversation

asomers
Copy link
Member

@asomers asomers commented Sep 17, 2022

When reading a value into an enum from getsockopt, we must validate it. Failing to do so can lead to UB for example with SOCK_PACKET on Linux.

Perform the validation in GetSockOpt::get. Currently SockType is the only type that requires validation.

Fixes #1819

@@ -121,6 +121,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.

@asomers
Copy link
Member Author

asomers commented Sep 17, 2022

I fixed the formatting. And rather than figure out how to determine whether a Fuchsia process can create raw sockets, I'm just skipping that test on Fuchsia.

@asomers
Copy link
Member Author

asomers commented Sep 17, 2022

@ahcodedthat does this patch solve your original problem?

@ahcodedthat
Copy link

Yes, that works. The example program now just fails cleanly with EINVAL instead of segfaulting. 👍

I suggest mentioning in the documentation for nix::sys::socket::sockopt::SockType that it will fail with EINVAL if the socket type is unknown.

It might also be wise to use a different error code, since the getsockopt system call can fail with EINVAL for other reasons too. ESOCKTNOSUPPORT seems best, but it doesn't exist on cfg(target_os = "haiku"), so consider using EPROTONOSUPPORT instead.

When reading a value into an enum from getsockopt, we must validate it.
Failing to do so can lead to UB for example with SOCK_PACKET on Linux.

Perform the validation in GetSockOpt::get.  Currently SockType is the
only type that requires validation.

Fixes nix-rust#1819
@asomers
Copy link
Member Author

asomers commented Nov 29, 2022

@rtzoeller I stupidly forgot about this PR when I made the 0.26.0 release a few minutes ago.

Copy link
Collaborator

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

Sometimes it's important to stress test cargo release by releasing multiple times in quick succession. Glad you caught it now.

@rtzoeller
Copy link
Collaborator

bors r+

@bors bors bot merged commit 749bf75 into nix-rust:master Nov 29, 2022
@asomers asomers deleted the SO_TYPE.2 branch November 29, 2022 06:27
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 this pull request may close these issues.

impl GetSockOpt for nix::sys::socket::sockopt::SockType causes UB with unknown socket type
3 participants