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

Use narrower type to fix possible panic with IPv6 address argument #6498

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Use narrower type to fix possible panic with IPv6 address argument #6498

merged 1 commit into from
Sep 3, 2024

Conversation

maxz
Copy link
Contributor

@maxz maxz commented Jul 24, 2024

This is a further cleanup following some discoveries I made when working with your PSK exchange.

As already written during PR #6477, calling psk-exchange or more specifically talpid_tunnel_config_client::request_ephemeral_peer with an IPv6 resulted in an EAFNOSUPPORT error.
With the current program it never makes sense to pass an Ipv6Addr to request_ephemeral_peer, since it can't handle an IPv6 connection even if the tunnel configuration service was listening to an IPv6.

There are two locations where request_ephemeral_peer is called. One of them is in talpid-wireguard/src/lib.rs, the other one is in the psk-exchange example.
Since the address is statically set to the IPv4 gateway in the former case, presently the only possible case for problems to happen was if someone entered an IPv6 when using the example.
I already disabled the possibility to enter such an address as part of the linked PR, but it still is a wart and it would not feel right to leave it in this faulty way.

So I had to decide whether to fix the IPv6 connectivity or to disable it.
I modified talpid-tunnel-config-client/src/lib.rs and talpid-tunnel-config-client/examples/psk-exchange to work with an IPv6 and then called psk-exchange with fc00:bbbb:bbbb:bb01::1 (the IPv6 gateway address) which failed with ECONNREFUSED. This seems to suggest that the tunnel configuration service is not listening to an IPv6 (at least not on that address and on port 1337, which would be the expected location.)

I tested psk-exchange just as described in my other PR and I did not know how to dedicatedly test talpid-wireguard/src/lib.rs but it compiles and the test suite (cargo test -p talpid-wireguard [it turns out that I did not properly call the tests at first, but I did now and with the patch from #6497 applied]) also finishes successfully. Since the input is static, I don't see any possible problem here.

If at some later point you decide to refactor this to support IPv6 it would probably be easiest to look at the passed IpAddr, check whether it is IPv4 or IPv6 and create an according socket.
Theoretically you could use an IPv6 socket to make IPv4 connections but the socket option IPV6_V6ONLY has to be disabled and this would turn into a portability nightmare and also has some security implications, so I'd recommend against it.


This change is Reviewable

@maxz
Copy link
Contributor Author

maxz commented Jul 25, 2024

Rebased onto the latest state of the main branch, which now contains the fix for the talpid-wireguard test suite.

The functions `request_ephemeral_peer` and consecutively `new_client`
accepted an `IpAddr`, but due to only ever preparing a v4 socket this
lead to panic due to an `EAFNOSUPPORT` error if an IPv6 was provided.

It would also have made sense to change `new_client` to create either
an IPv4 or IPv6 socket depending on the type of the address, but the
tuncfg service is currently not accepting IPv6 connections, therefore
this was the cleaner change.
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

LGTM.

This makes sense, thanks! You're correct that we do not currently use IPv6 here.

@dlon dlon merged commit c1e95cd into mullvad:main Sep 3, 2024
48 of 50 checks passed
@maxz maxz deleted the fix-indirect-type-mismatch branch September 3, 2024 11:21
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.

2 participants