Skip to content

Limit the amount and data size of incoming packets #3733

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

Closed
wants to merge 15 commits into from

Conversation

Outfluencer
Copy link
Collaborator

It is possible to make netty threads lag by spamming a massive amount of packets.
And you can make players time out with it.

I am not sure if i should say how specifically you can achieve that, publicly.

A simple packet limiter should mitigate those attacks.

A vanilla client should never exeed the default value of 2000 packets per second, but we can disguss about this if any concerns

@Outfluencer Outfluencer marked this pull request as draft August 26, 2024 13:20
@Outfluencer Outfluencer marked this pull request as ready for review August 26, 2024 13:29
@Janmm14
Copy link
Contributor

Janmm14 commented Aug 26, 2024

Would it maybe be useful to change this to have just have a task per connection on the netty thread every second setting the counter to 0? I think calling the currentTimeMillis method and checking for seconds elapsed for every packet is a waste of time.

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Aug 26, 2024

As you can see we dont get the millis every packet, we check for the millis every 2000th packet

Normally a client send 20-60 packets per second so this should not have any Performance impact at all

Edit: If you would set the limit to 1000 in the config every 1000th packet would be checked

@Janmm14
Copy link
Contributor

Janmm14 commented Aug 26, 2024

Oh, I somehow missed that part of the logic. My bad.

@andreasdc
Copy link

I saw that in 1 fork they also check the packet size in bytes, maybe it will be a good idea? Does it check every possible connection from player? Pretty nice idea for the counter.

@Outfluencer
Copy link
Collaborator Author

I saw that in 1 fork they also check the packet size in bytes, maybe it will be a good idea? Does it check every possible connection from player? Pretty nice idea for the counter.

i dont think a data size check is needed

@andreasdc
Copy link

I saw that in 1 fork they also check the packet size in bytes, maybe it will be a good idea? Does it check every possible connection from player? Pretty nice idea for the counter.

i dont think a data size check is needed

What if you have many large packets under the limit?

@Janmm14
Copy link
Contributor

Janmm14 commented Aug 26, 2024

What if you have many large packets under the limit?

Too large packets are causing kicks by bungee or spigot already.

@andreasdc
Copy link

What if you have many large packets under the limit?

Too large packets are causing kicks by bungee or spigot already.

That depends, but I guess we talk about the thing when pakets are received earlier on bungee than spigot.

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Aug 26, 2024

What if you have many large packets under the limit?

Too large packets are causing kicks by bungee or spigot already.

That depends, but I guess we talk about the thing when pakets are received earlier on bungee than spigot.

if the packet exceeds the protocol limits we throw an exception on bungee side, if we do not read the packet the minecraft server throws the exception
or did you mean something else?

@andreasdc
Copy link

What if you have many large packets under the limit?

Too large packets are causing kicks by bungee or spigot already.

That depends, but I guess we talk about the thing when pakets are received earlier on bungee than spigot.

if the packet exceeds the protocol limits we throw an exception on bungee side, if we do not read the packet the minecraft server throws the exception or did you mean something else?

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

@Outfluencer
Copy link
Collaborator Author

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

@andreasdc
Copy link

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

@Outfluencer
Copy link
Collaborator Author

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

@andreasdc
Copy link

andreasdc commented Aug 26, 2024

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

Spam under the packet limit with massive packets, I was more worried without the limiter.

@Outfluencer
Copy link
Collaborator Author

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

Spam under the packet limit with massive packets, I was more worried without the limiter.

thats not a bungeecord issue but this pr should mitigate that

@xism4
Copy link
Contributor

xism4 commented Aug 26, 2024

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

Spam under the packet limit with massive packets, I was more worried without the limiter.

thats not a bungeecord issue but this pr should mitigate that

Wouldn't an external plugin be better just for this?

@andreasdc
Copy link

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

Spam under the packet limit with massive packets, I was more worried without the limiter.

thats not a bungeecord issue but this pr should mitigate that

Wouldn't an external plugin be better just for this?

Too much resources for the plugin I think.

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

Spam under the packet limit with massive packets, I was more worried without the limiter.

thats not a bungeecord issue but this pr should mitigate that

I think it would be really nice to limit bytes too and maybe avoid checking instances of handler every packet.

@Outfluencer
Copy link
Collaborator Author

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

Spam under the packet limit with massive packets, I was more worried without the limiter.

thats not a bungeecord issue but this pr should mitigate that

Wouldn't an external plugin be better just for this?

Too much resources for the plugin I think.

I mean the packets that are handled on bukkit, I suppose it's possible to lag it before it even reaches the spigot.

i dont get your point

I think it's possible to mass spam packets, the netty thread on bungeecord will be lagged it even reaches the spigot.

how can you mass spam if there is a packet limit?

Spam under the packet limit with massive packets, I was more worried without the limiter.

thats not a bungeecord issue but this pr should mitigate that

I think it would be really nice to limit bytes too and maybe avoid checking instances of handler every packet.

checking for the handler should make no performace decrease at all

@andreasdc
Copy link

If it can be avoided I think it's good to not do that or check it just once. What do you think about the byte limiter?

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Aug 26, 2024

unnecessary the packet amount limit is enought

Edit: pardon its not

@Outfluencer
Copy link
Collaborator Author

Also we dont know the lenght of the packet if its compressed, we would need to read the lenght of every compressed packet. I think thats not worth

@andreasdc
Copy link

Also we dont know the lenght of the packet if its compressed, we would need to read the lenght of every compressed packet. I think thats not worth

Incoming compressed packet?

@Outfluencer
Copy link
Collaborator Author

Yes

@andreasdc
Copy link

Yes

I don't know about such packets, but if I do it in MinecraftDecoder the bytes are already read.

@Outfluencer
Copy link
Collaborator Author

Ah yes we decompress them anyway.

But this would have no benefits for bungee.

@andreasdc
Copy link

Ah yes we decompress them anyway.

But this would have no benefits for bungee.

Won't it be good idea to avoid spam with big packets?

@Outfluencer
Copy link
Collaborator Author

Ah yes we decompress them anyway.
But this would have no benefits for bungee.

Won't it be good idea to avoid spam with big packets?

big packets are not a problem i guess

@xism4
Copy link
Contributor

xism4 commented Aug 28, 2024

Ah yes we decompress them anyway.
But this would have no benefits for bungee.

Won't it be good idea to avoid spam with big packets?

Packets that are too large will simply be cancelled by disconnecting to that connection by the backend itself

@Outfluencer Outfluencer requested a review from md-5 August 28, 2024 10:58
@andreasdc
Copy link

Ah yes we decompress them anyway.
But this would have no benefits for bungee.

Won't it be good idea to avoid spam with big packets?

Packets that are too large will simply be cancelled by disconnecting to that connection by the backend itself

But you need the protection on the backend too and you need to listen there for every packet and check the size there too.
Great patch, many of which should have been applied earlier, but it's better later than never :D

@Outfluencer
Copy link
Collaborator Author

@md-5 what do you think about a packet amount limitation?

@md-5
Copy link
Member

md-5 commented Sep 23, 2024

If the Vanilla server can handle it, why not Bungee?

@andreasdc
Copy link

If the Vanilla server can handle it, why not Bungee?

Weird question, also I don't think that vanilla server can handle huge amount of packets.

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Sep 23, 2024

If the Vanilla server can handle it, why not Bungee?

The vanilla server also has a packet limiter that is kicking the player.
Specially for the case where packets are not forwarded to the backend, the client can send infinte amount of packets and maybe lag out a netty thread.

This is the vanilla packet limiter btw
image

EDIT: its disabled by default (i thought its not)

If you want to we could also disable it by default but i think its not a bad idea to use a default value like 2000, it should protect against packet spam and should not false flag.

@Outfluencer Outfluencer changed the title add packet limiter for incoming packets Limit the amount and data size of incoming packets Feb 12, 2025
@Outfluencer Outfluencer marked this pull request as draft February 12, 2025 08:27
@Outfluencer
Copy link
Collaborator Author

I'll add a second check, to check for the decompressed size of incoming packets soon.

@Outfluencer Outfluencer removed the request for review from md-5 February 12, 2025 09:01
@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Feb 12, 2025

I will heavily test the default values of 2048 max packets per second and 33554432 bytes (33MB) max data per second.
After i am done i will share my results and change defaults if necessary.

Log the packets sent by the player on kick
Ensure packet releasing by moving the checks inside the try final for releasing.
Changed default values
@Outfluencer
Copy link
Collaborator Author

the default limits are now 4096 packet per second and 33554432 bytes per second (33,5mb/s)
These values should filter any spam attacks for any kind of packets while not kicking legit players

I did a test on a hardcore pvp network for about 8 hours

The results where that there was a maximum amount of packets of 2737 per second. (probably a lagging player with autoclicker)
And a maximum of 60364 bytes per second, the server doesnt have big items like citybuild servers or factions, thats why i drasticly increased the max packets size to 33MB/s

but the vanilla client should never exeed these value
Only way to hit this limit is if the server gives the player an 8MB item and the player is clicking very fast on that item
BUT in all version where items was nbt the limit was on 2mb for the nbt so its also hard to achieve this values

@md-5 in my tests this fixed the issue we talked about.

@Outfluencer Outfluencer marked this pull request as ready for review February 12, 2025 18:36
@Outfluencer Outfluencer requested a review from md-5 February 12, 2025 18:36
md-5 pushed a commit that referenced this pull request Feb 15, 2025
@md-5
Copy link
Member

md-5 commented Feb 15, 2025

Thanks
556a15a

@md-5 md-5 closed this Feb 15, 2025
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.

5 participants