Suggestion to prevent crashing clients

In craftbukkit, there were multiple exploits that could be used to crash a client using a plugin. Now, for the sake of being nice to clients I will not publicly post any code here – but I have a suggestion.

Possibly sponge could put checks on outbound packets to prevent the client from crashing with exceptions. All you had to do with Bukkit was send a packet with an out of bounds field and the client gets crashed (there’s other ways, but I forget where I put the source code).

In this way, only people with modified server distros will be able to crash clients… Until Microsoft fixes the issue on the client side.

2 Likes

This would be good, as you said:

But how resource hungry would be checking every packet before sending? (I’m always going to put performance before convenience)

3 Likes

Let’s pretend an exploit involves sending the player’s health (I’m just making this up). The server can send a packet with the health being -5 and let’s say that the client crashes on negative healths. Well, all the server has to do is make sure that the signum of the health field is 0 or 1. (Or its >=0 )

Shouldn’t cause too much of a problem. Possibly this could be configurable.

Then there should be config for each check preformed, because I’d imagine that they’re are thousands if not hundreds of thousands of circumstances witch could cause the client to crash. That is too many checks to preform, even if each one is as simple as checking the sign of an integer.

I guess that largely depends on the type of packets being sent. If it was strictly numerical data being sent back and forth, and sanity checks could be simple comparisons like that, then it would probably be fine. It may be better to rely on a plugin for that.

Relying on a plugin to do this would not work since the servers sending these bad packets would simply not install those plugins.

With this logic, the servers causing crashing could also just modify the opensource code of sponge to avoid this checks altogether, or simply switch it off in config. You can’t do anything about the servers being malicious, you can only help prevent them from being stupid.

2 Likes

Ah, guess that’s true, but I can’t imagine why that’d be an issue for someone more than once, they can just not join that server anymore.

@tebbenjo also a good point, assuming the server owner can code.

Or assuming they can google. :wink:

1 Like

Sanitizing packets on the server jar would protect against skids :slight_smile:

Clients crashing servers is an issue, but the other way around? It’s an annoyance to the user, but the user can now simply not ever return to the server.

Plus there is an unfathomable number of ways to crash the client.

Honestly, the biggest risk is if plugins can edit NBT data, etc., they may make a mistake. You can already edit NBT data on CraftBukkit but it’s not as straight forward.

2 Likes

Fair enough. I suppose any of the current ways for lagging / crashing servers that does not rely on DDoS or DoS do need to be handled. Not sure if the paralyze and fast eat bugs are patched in vanilla, but they might need to get a patchin (without relying on anti-cheating plugins) since they can harm all servers.

I think it’s important to note the difference between malicious crashes, or innocent issues. I agree with @sk89q - it is not reasonable to have validation for every packet, ensuring proper worlds, entity id’s, etc - but, I personally would appreciate (and would be happy to help implement) some sort of validation for the things that really shouldn’t cause crashes. Like strings being too long, with a couple lines of code per relevant packet - it wouldn’t be hard to completely stop that issue. Throwing server errors is always preferable over losing players.

Don’t too long strings freeze clients? I think these were used for creative mode when clients can make ‘lagblocks’ ie renaming an item to like 2k characters.

Ironically, I was one of the people who urged Jens strongly to limit the lengths of strings in the protocol. He did the next version, but it did not handle errors gracefully. This was because back then, Minecraft did not bound the length of strings at all.

I suppose it’s doable, but should it be handled when setting the data or when the packet is sent?

Would there be a performance difference between them? If it’s negligable this should be handled when the packet is sent. Otherwise, non-handled packets can be sent by plugins without being checked by Sponge (obviously making the method to send packets private would not work).

Although I’d generally think it good procedure to do sanity checks on mutators as well.

Too many sanity checks could hinder performance. Sadly the client will blindly accept every packet it receives and try to parse it all.

I’ve found that it’s generally pretty easy to crash clients. The most common one for me is doing large worldedits, if you where looking at what ever got pasted at the time when doing copy and pastes(It’s probably from to many blocks getting updated on the screen at one time.).

That’s another way to consider this. It won’t affect clients with good computers. Forcing say, and ArrayIndexOutOfBoundsException will crash all vanilla clients. Quite a few hacked clients have patched some of these.