swarm: disallow dialing of 0.0.0.0

[marten-seemann]

We just discovered a bug in go-libp2p where our existing logic that prevents self-dials doesn't work.

It turns out that on Linux, it is valid to dial 0.0.0.0: https://en.wikipedia.org/wiki/0.0.0.0#Operating_system_specific_uses. The kernel will then dial localhost.

We should detect that and disallow the dial.

[thomaseizinger]

Thanks for the ping!

The way we handle 0.0.0.0 is that we watch the local system for interface changes and listen on the specific interfaces as they come up, see

rust-libp2p/transports/tcp/src/lib.rs

Lines 384 to 391 in 6f0895f

	if local_addr.ip().is_unspecified() {
	return ListenStream::<T>::new(
	id,
	listener,
	Some(T::new_if_watcher()?),
	self.port_reuse.clone(),
	);
	}

for the specific line.

Under the hood, this uses https://github.com/mxinden/if-watch/.

[marten-seemann]

The issue is not about the listener side, it's about dialing 0.0.0.0.

[thomaseizinger]

It's probably a good idea to filter out 0.0.0.0 advertisements.

My point was towards this in that we never advertise 0.0.0.0 because we translate them to actual addresses of interfaces first.

But I guess what you are saying is that we should still deny a dial for 0.0.0.0 and filter them from identify, etc.

[marten-seemann]

Right. You shouldn't advertise it, but you need to correctly handle all kinds of nonsense that other peers might try to tell you. 0.0.0.0 is one example of such nonsense that caught us by surprise in go-libp2p.

[marten-seemann]

go-libp2p fix: libp2p/go-libp2p#2560

[vnermolaev]

Hi @thomaseizinger

I want to address this; my initial thought goes as follows.

Immediately after https://github.com/libp2p/rust-libp2p/blob/master/swarm/src/lib.rs#L438

I want to add the following filtering:

let dial_address_errors = dial_opts
            .get_addresses()
            .iter()
            .map(|address| {
               // Collect errors per address.
                address
                    .iter()
                    .filter_map(|protocol| match protocol {
                        Protocol::Ip4(ip) if ip.is_unspecified() => Some(AddressError::Ip4(ip)),
                        Protocol::Ip6(ip) if ip.is_unspecified() => Some(AddressError::Ip6(ip)),
                        _ => None,
                    })
                    .collect::<Vec<_>>()
            })
            .flatten()
            .collect::<Vec<_>>();

        if !dial_address_errors.is_empty() {
            return DialError::AddressConfiguration(dial_address_errors);
        }

This filtering shall collect all errors in all address configurations and report them as a single error.
Could you consider this?

[thomaseizinger]

Filtering the addresses sounds good to me. We need to do it as late as possible, after we gathered the addresses from the behaviours. We should also add a log statement to inform users that we are dropping one of they addresses they provided.

Overall, this may result in a NoAddresses error which might leave people puzzling if they supply one but don't realise it is 0.0.0.0.

This will need a test too!

[vnermolaev]

I planned to introduce a new error variant in DialError and return all erroneous addresses in that variant. The presented code snippet does exactly that, albeit skips the introduction of the error variants.

Would you prefer to

• filter and drop incorrect addresses with an appropriate log statement or
• collect incorrect addresses and report them to the developer to handle?

Let me know, and I will be able to make a PR.

[thomaseizinger]

Adding a new variant will be a breaking change so I'd rather not do that for such a corner case.

I think filtering is fine. Most users will never hit this so adding an extra variant for that is unnecessary complexity :)

[vnermolaev]

Hey @thomaseizinger
I've implementing filtering and then got to testing and I found this test multiple_addresses_err.
The test lists addresses that should produce connection errors

        let addresses = HashSet::from([
            multiaddr![Ip4([0, 0, 0, 0]), Tcp(rand::random::<u16>())],
            multiaddr![Ip4([0, 0, 0, 0]), Tcp(rand::random::<u16>())],
            multiaddr![Ip4([0, 0, 0, 0]), Tcp(rand::random::<u16>())],
            multiaddr![Udp(rand::random::<u16>())],
            multiaddr![Udp(rand::random::<u16>())],
            multiaddr![Udp(rand::random::<u16>())],
            multiaddr![Udp(rand::random::<u16>())],
            multiaddr![Udp(rand::random::<u16>())],
        ]);

All these addresses require to produce a SwarmEvent::OutgoingConnectionError which seems to cover the original issue, doesn't it?
The test is easily extandable to ip6 ::1 and 127.0.0.1.

[thomaseizinger]

We just need to change these addresses to something else then! :)