rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.67k stars 12.75k forks source link

`bind` may incorrectly create a dual-stack socket on some platforms #130668

Open Zoxc opened 1 month ago

Zoxc commented 1 month ago

UdpSocket::bind and TcpListener::bind can on some platforms incorrectly create a dual-stack socket. It seems very unlikely that these are intended to create dual-stack sockets as the API and documentation make no mention of it. There's also no alternate API that allow for the creation of IPv6 wildcard sockets. This means that if these API were intended to create dual-stack sockets, the standard library would be missing essential IPv6 functionality.

Additional there are good reasons to not have or add an API for dual-stack sockets in the standard library:

Users not needing portability and wanting dual-stack sockets, may want to use a 3rd party crate (for example socket2) to create dual-stack sockets.

This bug have some rather bad consequences:

This affects (at least):

Not affected:

the8472 commented 1 month ago

You should list under which circumstances.

Zoxc commented 1 month ago

This applies when binding to Ipv6Addr::UNSPECIFIED. For example TcpListener::bind((Ipv6Addr::UNSPECIFIED, port)).

cuviper commented 1 month ago

The relevant socket option is IPV6_V6ONLY, which is off (and thus dual-stack) by default on many platforms, as RFC 3493 section 5.3 describes. Linux does have a system wide control for this, sysctl net.ipv6.bindv6only, but the default there is still 0. Windows seems to be the exception in enabling this by default.

Rust did expose this option long ago, but not in a useful way: #33052

cuviper commented 1 month ago

Personally, I don't think it's valid to characterize the current behavior as incorrect, but I can understand that it may be surprising. IMO, the way forward would be to design an explicit API like the old set_only_v6, but actually doing its job. Maybe we could add a net::BindOptions type similar to fs::OpenOptions?

the8472 commented 1 month ago

A general unbound socket type that can be configured before being bound and turned into a more specific type is probably better than an options builder. the socket configuration API is vast and has a lot of niche platform-specific/special-socket-type stuff.

Zoxc commented 1 month ago

@cuviper What the underlying platform APIs look like is not relevant other than the constraints the place on std's API. They do all support IPv6 wildcard sockets however, so that can't be used as an argument for why std does not always create IPv6 wildcard sockets. What matters here is the intent of the bind API.

I find it rather unlikely that std was intended to not support IPv6 wildcard and dual-stack wildcard servers like you suggest. I expect std to properly support IPv6 and while the API design does, the actual platform implementations are lacking. Adding new APIs instead of actually fixing the implementation is a terrible solution and leaves the current API as a security and correctness trap.

himikof commented 1 month ago

A general unbound socket type that can be configured before being bound and turned into a more specific type is probably better than an options builder. the socket configuration API is vast and has a lot of niche platform-specific/special-socket-type stuff.

The socket2 crate which follows this paradigm was already mentioned. This is literally the first example in its documentation:

use std::net::{SocketAddr, TcpListener};
use socket2::{Socket, Domain, Type};

// Create a TCP listener bound to two addresses.
let socket = Socket::new(Domain::IPV6, Type::STREAM, None)?;

socket.set_only_v6(false)?;
let address: SocketAddr = "[::1]:12345".parse().unwrap();
socket.bind(&address.into())?;
socket.listen(128)?;

let listener: TcpListener = socket.into();

But I think that having a simple way to explicitly set only_v6 without resorting to such low-level code would be an appropriate addition to the standard library regardless of having a flexible socket2-like type in it (which I personally think is very useful, too).

Maybe we could add a net::BindOptions type similar to fs::OpenOptions?

Actually, the TcpStream::connect/TcpStream::connect_timeout have the same fundamental problem. So maybe net::BindOptions/net::ConnectOptions? Or net::SocketCreationOptions?

What matters here is the intent of the bind API.

Yes, and the intent/semantics of the common bind API are (in this part) described in RFC 3493, sections 3.7 and 5.3. The RFC authors were very explicit that they intend the dual-stack sockets to be both mandatory and the default in the implementations:

Applications may use AF_INET6 sockets to open TCP connections to IPv4 nodes, or send UDP packets to IPv4 nodes, by simply encoding the destination's IPv4 address as an IPv4-mapped IPv6 address, and passing that address, within a sockaddr_in6 structure, in the connect() or sendto() call.

As stated in section <3.7 Compatibility with IPv4 Nodes>, AF_INET6 sockets may be used for both IPv4 and IPv6 communications. Some applications may want to restrict their use of an AF_INET6 socket to IPv6 communications only. For these applications the IPV6_V6ONLY socket option is defined.

Unfortunately, the current situation is indeed not consistent between platforms, and the default cannot be relied on. So an explicit toggle is needed both to ensure dual-stack sockets are used and are not used.

If such only_v6 toggle is available in the standard library, a separate question arises: what the default value should be if the user did specify nothing. There are 3 obvious options: off (following the RFC), on (as suggested in this issue), or following the platform defaults (as it is currently). But I'm also pretty sure that changing the existing TcpListener::bind API would be prohibited by the API stability requirements, so any change here would result in creation of new APIs anyway.

Zoxc commented 1 month ago

Yes, and the intent/semantics of the common bind API are (in this part) described in RFC 3493

RFC 3493 is irrelevant. It does not specify std. It does not even specify any Rust API. std does not try to follow it either, otherwise some variant of IPV6_V6ONLY would have been in the initial design.

This is simply an implementation bug in std.

cuviper commented 1 month ago

RFC 3493 is irrelevant. It does not specify std.

IPv6 is not an invention of Rust std -- the RFC is relevant to expectations and interoperability at large.

If such only_v6 toggle is available in the standard library, a separate question arises: what the default value should be if the user did specify nothing. There are 3 obvious options: off (following the RFC), on (as suggested in this issue), or following the platform defaults (as it is currently).

I think following the platform default makes sense, not only because it's the status quo, but it's also hard to achieve that otherwise -- especially with local system choices like the Linux sysctl that you would have to check at runtime.

Zoxc commented 1 month ago

IPv6 is not an invention of Rust std -- the RFC is relevant to expectations and interoperability at large.

RFC 3493 also not specify IPv6. C users of Windows and OpenBSD would also have different expectations.

I think following the platform default makes sense

Only Linux seems to have a concept of a platform default. Regardless std's bind does not aim to follow your definition of a platform default. I don't see a strong use case for adding such an API, but it should diverge from bind by returning the actual bound IP protocols.