Add expected string max length to protocol data for validity checks

[emnpiex]

the Java Edition has a number of server-side packet decoders/constructors that specify a maximum received length that a client can send before it'll reject that packet from the client.

for example, in 1.19.3's net.minecraft.network.protocol.login.ServerboundHelloPacket (unsure what mappings are used below):

public ServerboundHelloPacket(FriendlyByteBuf pBuffer) {
  this(pBuffer.readUtf(16), pBuffer.readOptional(FriendlyByteBuf::readUUID));
}

the name string is limited to 16 characters to prevent a malicious user, on multiple, new client connections, sending a fake handshake packet that claims to contain a "hostname" string of length 32767 (might only need to be a thousand for an effective attack so long as the string's prefixed length is 32767) and having the server reserve number_of_spoofed_clients * 32767 of memory semi-simultaneously in an effort to read the strings, crashing the server process. in theory, even one such spoof could crash a server with limited dedicated memory.

this maximum length information is not preserved in the JSONified version of the protocol present in:

minecraft-data/data/pc/1.19.3/protocol.json

Lines 1538 to 1553 in 5afbbdd

	"packet_login_start": [
	"container",
	[
	{
	"name": "username",
	"type": "string"
	},
	{
	"name": "playerUUID",
	"type": [
	"option",
	"UUID"
	]
	}
	]
	],

i can only assume the reason that this was left out of minecraft-data was the underlying technical issue of generally specifying a maximum accepted value for "countable" types (e.g. varint) not being supported in ProtoDef. i've made a feature request and PR for the underlying ProtoDef library: ProtoDef-io/ProtoDef#60.

i can make a proof of concept for one Minecraft version to determine the level of work required to update the protocol documentation for each version. is it anticipated that there will be any interest in resolving this issue?

[extremeheat]

server reserve number_of_spoofed_clients * 32767 of memory

Not sure why this would be a problem--you just make sure the length of a string or buffer doesn't exceed the size of the current buffer before trying to decode. This is just basic bounds checking.

In order to decode the packet you have to load the contents of the whole buffer into memory anyway, so reading a substring is just temporary garbage that will be written over or freed quickly.

This could be useful for validity and some sanity checks but not really useful for safety, for safety you want to simply discard big packets in first place.

[extremeheat]

Related #360

Please reopen if you still think it's valid

[emnpiex]

@extremeheat

hello, you marked this as 'waiting for op' and i did not see that until now, my apologies.

if the approaches for safety that you have laid out are available on a system then you are correct: this is not a vulnerability in these common use cases. treating this as a general correctness/portability/efficiency issue in the below.

you just make sure the length of a string or buffer doesn't exceed the size of the current buffer before trying to decode

this is not possible in an environment where packets are received to a pipe as chunks or, in the general case, when the full size of the received data cannot be known at the time that it begins parsing. you are right that, if the intention was purely to be optimal and safe, it would be best to not implement a Minecraft server in such an environment but if one does then the data in this repo cannot be used to make that more safe.

additionally, your suggested method would require tracking how many bytes have been read total by all packet-reading functions and comparing them to the total expected. this is an unnecessary efficiency loss in an environment that could potentially have extremely limited resources to allot to each packet (e.g. there have been multiple Minecraft server implementations on embedded hardware). if you believe the efficiency loss is not a concern worth tracking the protocol data for, the best argument i would raise for this as a value judgement is that Minecraft does it this way and you propose doing it a different way. it is also the case that the Java Edition does not allow an operator to choose a maximum packet length and disconnect clients which send packets exceeding that as you suggest in

for safety you want to simply discard big packets in first place.

so it is unclear why you would expect that for those implementing their own version. the Java Edition hard-codes the limit to be 2^21-1 in net.minecraft.network.Varint21FrameDecoder. i can think this suggestion would only be a reasonable thing to put in place for some movement/PvP/entity-interaction-focussed minigame modes but would certainly not be what i would expect to be in place for a SMP server. i do not know if the PrismarineJS crew are generally in favour of doing things the Minecraft way where feasible but if so then these are breakages from that.

i would also like to address the non-use of length-prefixing in terms of source-correctness. in Java 1.19.3, net.minecraft.network.protocol.login.ServerboundHelloPacket, for example, will disconnect clients in the packet-reading upon receiving a username input longer than 16 bytes and not attempt to read the name nor the optional UUID. as is, it's up to the implementer of whatever it is that is using the codec system to implement a disconnect check after the full message is read into memory.

i will lay out my intentions and allow you to assess whether this is a concern for this repo. regardless of the decisions of PrismarineJS, i intend to use length-prefix-checked strings/collections for my C++ codec system that will be based on the work of PrismarineJS/minecraft-data. i already have forks of the relevant libraries (proto-def link, minecraft-data link, the "validator" NodeJS module is just cloned and you change the ProtoDef module to point at my fork) that i can use if it is assessed to not be a concern for you but will be available to be merged upstream as i support more Minecraft versions. this is additional data precision that will be available to PrismarineJS/minecraft-data so it chooses. my codec library is intended to be portable and run on the barest metal one can find.

Please reopen if you still think it's valid

this may not have been directed at me but i do not have permissions to reopen issues in this repository.

thank you for your time.
- MnPX

[extremeheat]

Ok, reopened to continue #360 basically

This info could be added as an annotation for parsers to sanity check packet data against

[emnpiex]

@extremeheat

thank you :)

https://github.com/emnpiex/minecraft-data/tree/fix/1-19-3-lengths is not in a state where it could be merged into minecraft-data without making breaking several tests due to the dependency issues relating to the changes to the proto-def format. is there an appetite for the PrismarineJS team to merge ProtoDef-io/ProtoDef#63 (and the PRs it is based on) for this additional annotation data or is there a preferred alternative method to implement the changes?