Murmur assumes clients are UDP capable by default

[TheStaticTurtle]

Description

Murmur assumes that clients are UDP capable by default.

It only switches to TCP when it receives a UDPTunnel packet (see src/murmur/Server.cpp#L1764)

This is an issue for listen-only clients that can't use UDP.
Since they don't send audio, they will never send the UDPTunnel packet, which means the server will incorrectly send the data over UDP and the client will never receive it.

If the client uses the Force TCP options but can actually receive UDP it will still work because the whole UDP receive chain bypasses the forced TCP check.

Steps to reproduce

• Connect to the server with Force TCP
• Don't speak
• If some else in the channel speaks, you will get the data over UDP (look at the network stats or packet capture)
• If you speak it will switch to TCP

Mumble version

1.5.517

Mumble component

Server

OS

Linux

Reproducible?

Yes

Additional information

A possible fix would be to set aiUdpFlag to 0 in the ServerUser constructor.
If the client is using UDP it will quickly be set to 1 in the UDPMessageType::Audio handler of src/murmur/Server.cpp#970
Of course, that means that the client will receive everything over TCP until he speaks (but at least the client will get it).

Another option is changing the flag in the ping handler, but it isn't the best idea because UDP pings are sent and received by the client regardless of whether it's using TCP or not. This means it wouldn't honor the Force TCP option of the clients.

To truly solve this without compromise would require adding something like a force_tcp field to a TCP packet (maybe the Authenticate or UserState packet?). That way the server is actually aware of the client settings and not just guessing.

A truly horrible way to bypass it would be to send \xff\xff\xff over a UDPTunnel packet. It's just enought to pass the length check and wouldn't be interpreted as a packet. (Currently using that while debugging my implementation)

[Krzmbrzl]

To truly solve this without compromise would require adding something like a force_tcp field to a TCP packet (maybe the Authenticate or UserState packet?). That way the server is actually aware of the client settings and not just guessing.

That's the best option, imo.

[TheStaticTurtle]

I do think so, too.
The current version introduces many new things for the protocol. It would be an optimal time to modify it further.
I think that using UserState would be the best option, it would allow the client to switch protocols on the fly.

But I do also think that murmur shouldn't assume UDP by default, all the setup is exchanged over TCP.
If TCP doesn't work, there's no way UDP will work.
I think it would be best to continue to use TCP until both parties are sure that UDP works.

Ideally, adding a force_tcp field in the server .ini config and in the ServerConfig packet would also be nice to have (another way would be to simply not send the CryptSetup packet). It would allow the server to force clients to use TCP if it wants to.

As you said, yourself in the matrix chat UDP-TCP switching isn't the best right now, allowing clients and servers to specify what they actually want would be a pretty good step in making it more robust.

Also, (might be a different discussion) but if the client/server sets forced TCP, should it still send Ping packets or open the UDP socket at all? IMO it shouldn't, there might be another reason for someone forcing TCP than "I know UDP won't work" and right now (even if it's encrypted) UDP pings are a leak.

[Krzmbrzl]

I think that using UserState would be the best option, it would allow the client to switch protocols on the fly.

No, UserState is meant for the state of individual user entities and not for encoding information about a specific connection. Imo, the Authenticate message would be a better - it already contains fields for which audio codec to use, so a flag indicating whether or not the client wants to use UDP would it right in.

Note that this is already done in #5931 while working on #5517. I believe once this PR gets merged the only thing to fix this issue as well would be to default to TCP transmission until UDP is ready to go, right?

Also, (might be a different discussion) but if the client/server sets forced TCP, should it still send Ping packets or open the UDP socket at all? IMO it shouldn't, there might be another reason for someone forcing TCP than "I know UDP won't work" and right now (even if it's encrypted) UDP pings are a leak.

I'm not sure much would be gained from adding extra logic that prevents pings in cases where client and/or server willingly switched to TCP vs UDP just having broken down somewhere in the middle of a connection. To me this sounds like a bit fiddly to get right and thus the implementation would introduce some level of complexity into the code base that I don't think would be worth it. 🤔

[TheStaticTurtle]

Sorry for the delayed response, must have missed the email!

I agree, after thinking about it a bit more, UserState isn't the best choice. But I don't think Authenticate is either, what if the client wants to switch after being connected, can it just send a second packet?

Note that this is already done in #5931 while working on #5517. I believe once this PR gets merged the only thing to fix this issue as well would be to default to TCP transmission until UDP is ready to go, right?

Ah yes, I didn't see these, sorry!
I quickly looked over the PR, it's pretty cool. The only thing left is to default to TCP.

I'm not sure much would be gained from adding extra logic that prevents pings in cases where client and/or server willingly switched to TCP vs UDP just having broken down somewhere in the middle of a connection. To me this sounds like a bit fiddly to get right and thus the implementation would introduce some level of complexity into the code base that I don't think would be worth it. 🤔

I'm just pointing out that if the "Force TCP" checkbox is set, a user would expect that their client will send absolutely nothing via UDP. I don't really have a real life scenario where that would matter to the point of getting a check implemented, in mumble, tho. If it's really that important, they can drop the outgoing traffic with the firewall. I agree that it would require a bunch of time and will introduce complexity, and as you said, I'm not sure if it's that worth it.