Skip to content
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

"Poisonous NBT" server vulnerability #1806

Closed
coolsquid opened this issue Apr 16, 2015 · 18 comments
Closed

"Poisonous NBT" server vulnerability #1806

coolsquid opened this issue Apr 16, 2015 · 18 comments

Comments

@coolsquid
Copy link

@coolsquid coolsquid commented Apr 16, 2015

@ammaraskar discovered a bug where the client can generate "poisonous NBT", increasing the server load extremely and making the server crash with an OutOfMemoryException. The issue has been described in a more technical way here. I know this is a Vanilla issue, but it's quite huge, so hopefully Forge can include a fix.

@coolsquid coolsquid changed the title NBT server vulnerability "Poisonous NBT" server vulnerability Apr 16, 2015
@KingLemming

This comment has been minimized.

Copy link
Member

@KingLemming KingLemming commented Apr 16, 2015

This has been known about for a long time by many of us.

Whitelisting pretty much solves it already.

Not saying it shouldn't be solved if possible. Just that it isn't the grand "sky is falling" revelation that it's being presented as in /r/minecraft.

@LexManos

This comment has been minimized.

Copy link
Member

@LexManos LexManos commented Apr 16, 2015

THere are a few potential fixes, but the issue isn't the NBT per-say. As that is limited to 2MB. It's the spam of packets thats the issue. As would be in any network protocol. The ONLY reason this is considered a 'vulnerability' is because of it's large payload to response ratio. Roughly 64:1, which in of itself isn't that big of a deal.

If he felt like it he could actually get the payload a lot smaller... {I did a test where it was 260 bytes -> 2 mb}
But yes as King says this is far from a "sky is falling" event.
If you have a proposition that would prevent the client from spamming packets let me know.

@DarkGuardsman

This comment has been minimized.

Copy link
Contributor

@DarkGuardsman DarkGuardsman commented Apr 16, 2015

May I suggest a way to monitor the source of the packets. Then have something that triggers an event call when the packet volume from a single source gets to high. This way a mod could be developed with the event to auto kick player's spamming packets

@cpw

This comment has been minimized.

Copy link
Contributor

@cpw cpw commented Apr 16, 2015

Pointless to try and protect IMO..
Its not about network flood, directly. You can spam a few dozen of these packets and murder memory.. Protection in FML or forge would likely invalidate many nested nbt use cases in regular mods..

@DarkGuardsman

This comment has been minimized.

Copy link
Contributor

@DarkGuardsman DarkGuardsman commented Apr 17, 2015

I guess this falls in with Xray hacking and being difficult to prevent.

@LexManos

This comment has been minimized.

Copy link
Member

@LexManos LexManos commented Apr 17, 2015

cpw its about both network and nbt. I understand exactly what is going on.
His MAIN 'exploit' was fixed, by Mojang limiting incoming NBT to 2MB.
Which he simply got around by spamming the packet.
It's not a combination exploit and NOT just NBT.
The NBT has been handled, it's the packets that are the issue right now.
A similar exploit can be done with the custom payload packet. The only thing about that is the data doesn't persist as long.

@cpw

This comment has been minimized.

Copy link
Contributor

@cpw cpw commented Apr 17, 2015

He can still construct a custom pipeline in FML/netty to compress/decompress the packet request, leveraging the 'best' of both worlds, kinda. Its a dumb attack tho and only affects only servers with no other security for long..
Its not like its information exposing, its simply a dumb dos attack that will likely result in quick white listing on any victim server. The server will likely not even suffer data loss..

@ammaraskar

This comment has been minimized.

Copy link

@ammaraskar ammaraskar commented Apr 17, 2015

His MAIN 'exploit' was fixed, by Mojang limiting incoming NBT to 2MB.

It wasn't, mojang tried to fix it but didn't actually test it. It turns out tag list ends were considered to not take up any space. With a single packet you can make the server use up more than a gig of memory, not much spamming required.

@Kubuxu

This comment has been minimized.

Copy link

@Kubuxu Kubuxu commented Apr 17, 2015

Spigot fixed it with small (very small) patch patch for 1.8.

@LexManos

This comment has been minimized.

Copy link
Member

@LexManos LexManos commented Apr 17, 2015

That doesn't seem to be a 'fix', however, been derping around with it all day because I had nothing better to do. And I can't actually reproduce it in any feasible manor. The concept is sound, but i've got my client pumping out 4TB worth or NBT data to the server every second and the server is chugging along just fine... The stacks that this targets are only short lived stacks that the packets discard when they are done, and the GC eats them up. Can't get a single case where it actually functions. Tested on the smallest servers I could find with the crappiest CPUs.. and nothing.

@ammaraskar

This comment has been minimized.

Copy link

@ammaraskar ammaraskar commented Apr 17, 2015

That's interesting, you mind showing me the code you're using to generate the nbt you're sending? I very specifically use LIST TAG ENDs. Aside from my personal testing I've got two independent reports here and here that it works fine.

@LexManos

This comment has been minimized.

Copy link
Member

@LexManos LexManos commented Apr 17, 2015

Was able to tweak my code a bit and get it generating invalid nbt that caused the server to freak out. But using normal mechanics didn't I was hoping not to have to serialize it myself.
Anyways, it does indeed allow you to eat memory, and this shows where A LOT of NBT data isn't being counted by the size tracker. Guess i'm going to have to go in and dissect all the NBT readers.

Also i'm not sure what you are meaning when you refer to LIST TAG ENDs. Lists do not have a END tag as they are length prefixed.

@ammaraskar

This comment has been minimized.

Copy link

@ammaraskar ammaraskar commented Apr 17, 2015

Lists do not have a END tag as they are length prefixed.

That's exactly the thing, you can put multiple END tags in lists, and it wont account for the size.

@LexManos

This comment has been minimized.

Copy link
Member

@LexManos LexManos commented Apr 17, 2015

oh ha, a list of type END wouldn't read anything from the data stream ha!
That'll teach me to poke around in MC without actually paying attention.
Damn that means my generic fix won't work.
The SizeTracker is in of itself very poorly implemented. There should be no reason to manually count 90% of the data. As hooking into the DataInput functions would be far easier. Just need to think of a way to allow for external incrementing of that.

@LexManos

This comment has been minimized.

Copy link
Member

@LexManos LexManos commented Apr 17, 2015

de066a8
I hate doing it this way, I would prefer to do something like this: https://gist.github.com/LexManos/6054f1ae2fc1937633df

But would require more patches to remove the vanilla calls. Changing the passed tracker for 'extra' data such as the object allocation. But, this works. Feel free to poke it.

@wgaylord

This comment has been minimized.

Copy link

@wgaylord wgaylord commented Apr 17, 2015

Well mojang fixed this finally. As a 1.8.4 release.

@LexManos LexManos closed this Apr 17, 2015
@LexManos

This comment has been minimized.

Copy link
Member

@LexManos LexManos commented Apr 17, 2015

It's fixed in Forge as well by the previous commit. We're a bit more lenient than 1.8.4 but it accomplishes the same thing. It doesn't address the plethora of other ways to crash the server via the client. But as this one is the trend of the day the fix is in. Just a note, the key is the TAG_END being in the list. My personal testing mod is reduced to a single list in the NBT.

Whenever cpw gets time he will backport it to 1.7.10 as he is the one in charge of that.

@cpw

This comment has been minimized.

Copy link
Contributor

@cpw cpw commented Apr 17, 2015

Should be back ported to 1.7.10 this weekend. What a DUMB waste of time..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.