Viarewind Server Crash
27chandler opened this issue ยท 9 comments
Describe the bug, provide any errors
A player was able to crash our server somehow. We don't know the method but we do have the error it rapidly produced before crashing: https://paste.gg/p/anonymous/f30d3b0c6d234392b1cbb0d21bd0399b
How can we reproduce it?
Unfortunately I do not know the steps to reproduce, although I do know the person who caused it most likely was using a hacked client.
Expected behaviour
The server to throw no errors and to not crash
Screenshots
ViaVersion Dump:
https://dump.viaversion.com/21e3bbb96fd244c128366fc262c5126928f2a85fabdd3884339ca7e4afde3033
Additional server info
Server is within an XCord network (BungeeCord fork)
Checklist (mark with [X] to check)
- I have included a ViaVersion dump
- If applicable, I have included a paste (not a screenshot) of the error
- I have tried a build from https://ci.viaversion.com/ and the issue still persists
- I have looked at https://www.spigotmc.org/resources/viarewind-legacy-support.52924/ and it does not offer a fix for my problem
IMPORTANT: Follow the Checklist or we can't reproduce your issue and your issue will be close
Does it really crash? Because it seems to be detecting it on the error, maybe it needs to detect it before 512 if thats the case... and not throw an exception for invalid client packets.
This is a library in common with all Via plugins.
Basically ViaVersion already tries to fix this by limiting the "depth" of the JSON object but 512 is considerably big and the packet contents and exception is thrown on each packet so maybe reducing the depth to 16 and removing the exception/packet content display should fix this.
Just to add, the same error came up on a seperate server of ours today and caused the crash again. So the error is definitely tied to the server crashing.
@linsaftw ViaVersion's task is to do what the Minecraft server and client does, and if the neither client or server do have a limit, it wouldn't be legitimate to add one in ViaRewind, it's the anticrasher's task to hook before any other plugins and do checks to prevent such exploits, I'll think about adding a config option for it in ViaRewind, but imo it's nothing which we are supposed to do
This is a known server crash exploit in 1.8.x servers, there is nothing Via* plugins can really do about this, it's something your anticrash/anticheat plugin ideally would fix by filtering the packet before it comes to ViaVersion.
But Via runs before the actual server decoder. How is the server supposed to fix something happening before server operations occur? You have to limit depth of the jsons you are decoding for 1.8. This only happens with ViaRewind installed, no matter if the fork has a hard limit to components, ViaRewind throws exceptions and decodes too much which lag the server.
@linsaftw ViaVersion's task is to do what the Minecraft server and client does, and if the neither client or server do have a limit, it wouldn't be legitimate to add one in ViaRewind, it's the anticrasher's task to hook before any other plugins and do checks to prevent such exploits, I'll think about adding a config option for it in ViaRewind, but imo it's nothing which we are supposed to do
No. You are introducing exploits to the server which are not present in forks. It's not the task of a fork to manually hook into plugins, and it's not the task of a plugin to introduce old exploits. You are forcing people to have to use an anti-exploit for something that is natively fixed in modern forks, but unfixed in the old outdated Paper.
I recommend making the limit configurable and to not throw exceptions.
I just saw that this is even before we switched the component library, in that case we/adventure actually introduced a new exploit here, looks like adventure had a limitation while Mojang doesn't have one, I think this should already be fixed since a few releases because we switched to a "better"/more protocol focused component library. I think my original response to the issue addressed the crash exploit itself and not the exception, sorry about that. The problem with having limitations is that we don't know the exactly limit/point were a server/client crashes and where it can deal with the component, having a to low limit for example could potentially drop data which the server could handle. But I'll add a config option next release to enable a limiter and to change the limitation size.