Fix bug with ping servers

[AlphaBaqpla]

The library sends ping incorrectly, need to request a ping after receiving the status, and not breaking the connection.

I got this error when pinging the server:
[12:46:27 WARN]: Error whilst handling query packet from /95.217.180.53:52188
io.github.waterfallmc.waterfall.utils.FastException: No Session!

[ItsDrike]

Can you link some documentation (probably wiki.vg) that states that this is required?

I don't think there's anything in the minecraft protocol spec that would require first querying for status, and only
requesting ping afterwards. The point of the ping method is to not have to obtain status, and just get the ping time.

Also, most servers seem to be able to handle this without any issues, if this is just waterfall server implementation doing something custom, we can't do much about that, but we shouldn't put workarounds like this into code that's supposed to run for any general minecraft server.

This also brings in an issue of some server implementations not being able to properly return status for some reason, but standalone ping works on them. With this change, that would no longer be the case, and not even ping would be requestable.

It also doesn't make much sense to request status and not use it's result in ping, and do the exact same thing in status method, except that time the result is actually used. It just slows ping down, as ping is supposed to be the quick way of just checking if server is up or not, without caring about the status information.

[kireevm96]

Even a pure bungecord requires a status

https://github.com/SpigotMC/BungeeCord/blob/e71767688d81ebff2b913920a04c5a958b57e4d9/proxy/src/main/java/net/md_5/bungee/connection/InitialHandler.java

[AlphaBaqpla]

https://wiki.vg/Server_List_Ping

[kireevm96]

https://wiki.vg/Server_List_Ping

Ping Request
If the process is continued, the client will now send a Ping Request packet containing some payload which is not important.

[kevinkjt2000]

Hmm ping has been a separate method for a long time. Vanilla servers support doing ping without a status first, right? Some servers did not support the ping right after status, so as part of #316, the time was calculated locally from before and after the status is retrieved instead of using a ping message. Have you considered using the latency from the status object instead?

As far back as v2.3

mcstatus/mcstatus/server.py

Lines 35 to 46 in c94df6d

	def ping(self, retries=3, **kwargs):
	connection = TCPSocketConnection((self.host, self.port))
	exception = None
	for attempt in range(retries):
	try:
	pinger = ServerPinger(connection, host=self.host, port=self.port, **kwargs)
	pinger.handshake()
	return pinger.test_ping()
	except Exception as e:
	exception = e
	else:
	raise exception

ping has not executed read_status. I'm not dismissing the possibility of a bug here, but merely wondering why it would take so long to notice this and why most sever implementations support doing ping without reading status first.

[AlphaBaqpla]

a problem with the Query vulnerability was found, and when they started fixing it, this bug became noticeable

[kireevm96]

There will be no error, but this behavior causes the ip address to be blocked by the security system, since packets were not sent correctly

[kevinkjt2000]

a problem with the Query vulnerability was found, and when they started fixing it, this bug became noticeable

So, the server implementation that had a vulnerability in their query code decided to break the functionality of the ping code? Am I understanding that correctly?

[kireevm96]

by default, you should just send an error and that's it. in more secure ones, already block those who send packets crookedly

[ItsDrike]

According to docs, the status is clearly skippable and a simple ping can be requested without first going through status.

This means any implementation that requires a ping is acting differently from what's going on with vanilla minecraft servers. (Pure bungeecord is still not vanilla! By vanilla server we mean the actual .jar server downloadable from minecraft.net) it simply isn't our responsibility to cover for these cases, as there's potentially infinitely many of them (new server implementations can be created every day).

Yes, it's true that pretty much all minecraft clients will first send status, because they'll need it anyway (this sequence generally only happens once entering/refreshing the multiplayer tab, loading the motd of servers, etc.). However clearly, external clients should be able to only ask for ping, without status.

The fact that minecraft clients do it in this order doesn't mean it's the only way it should be doable, and server implementations shouldn't lock away those other approaches. This isn't a bug with mcstatus, it's a bug on bungeecord side.

(Also, this really should've been an issue instead, not a PR, people searching for this might have a hard time finding it here, amongst PRs)

[AlphaBaqpla]

a problem with the Query vulnerability was found, and when they started fixing it, this bug became noticeable

So, the server implementation that had a vulnerability in their query code decided to break the functionality of the ping code? Am I understanding that correctly?

Fixing Query vulnerabilities checks all packets, it is set that in order to receive a ping, a status request must first be sent. mcstatus interrupts the connection and due to which the protection thinks that the packet is not correct and bans the IP

[kevinkjt2000]

in order to receive a ping, a status request must first be sent

False. Vanilla servers respond as expected to ping messages without a previous status message on the same connection.

@ItsDrike Thanks for spotting that bit on the wiki.

I am closing this based on vanilla support and the only documentation available on wiki.vg indicates that this is allowed. This should instead be filed as a bug against this server's implementation. They seem to have caused a regression after this described security fix.