search

sirjonasxx / G-Earth

Cross-platform Habbo packet manipulator
MIT License
91 stars 47 forks source link

issue with programs connecting with extension port #125

Closed sirjonasxx closed 2 years ago

sirjonasxx commented 2 years ago

image

dorving commented 2 years ago

@sirjonasxx just for the first packet?

Since the extension server only handles one packet, that being: https://github.com/sirjonasxx/G-Earth/blob/8cd429cc2313af2dece44a21688b7ef4f002cb06/G-Earth/src/main/java/gearth/services/extension_handler/extensions/implementations/network/NetworkExtensionInfo.java#L99 Can we not base the cap for any incoming packet on that (so not just the first one)?

Currently I set the cap as max 100 characters per string encoded in the packet. This might be a bit of a lowball estimate, thoughts?

WiredSpast commented 2 years ago

It's not the only packet the server also get's huge packets like inventory, catalog etc. back from extensions, so putting on a character limit on all incoming packets isn't really a good idea

dorving commented 2 years ago

It's not the only packet the server also get's huge packets like inventory, catalog etc. back from extensions, so putting on a character limit on all incoming packets isn't really a good idea

I want to add the check only in NetworkExtensionsProducer, since this only handles the extension info packet it should be fine right? It doesn't do anything with other packets. Or am I missing something?

WiredSpast commented 2 years ago

Ah yeah there it only handles that yes, but I think it would be better to add an else to this if

https://github.com/sirjonasxx/G-Earth/blob/8cd429cc2313af2dece44a21688b7ef4f002cb06/G-Earth/src/main/java/gearth/services/extension_handler/extensions/implementations/network/NetworkExtensionsProducer.java#L63-L77

So that it just terminates the connection when it doesn't give the correct ID

WiredSpast commented 2 years ago

When the received packet does have the correct ID but invalid content the code within the current if already causes it to terminate (I think)

dorving commented 2 years ago

When the received packet does have the correct ID but invalid content the code within the current if already causes it to terminate (I think)

The main issue is that a diff app connecting to the port, may send some data that is read as the size of the packet, which may allocate an humongous byte array for the packet read buffer.

So unless it reads the header at the start, the check has to be before the packet is being read. If the header is encoded at the start, then we could also first read the header, check if the ID is valid and then do the check in the code u linked. Can also do that!

sirjonasxx commented 2 years ago

PR looks good, I think the boundary is a bit low though since we don't know how the future of that packet looks like, and there's technically no restraints on title/description size etc, perhaps we could do the header check first and then be less strict about the size

dorving commented 2 years ago

PR looks good, I think the boundary is a bit low though since we don't know how the future of that packet looks like, and there's technically no restraints on title/description size etc, perhaps we could do the header check first and then be less strict about the size

Cheers! I increased the max string size to 4_000 , so the final max cap for the packet is now roughly 48kb ((8000 * 6) + 4). If the packet where to change structurally in the future, we'll have to modify the constants I defined, though I think this is preferable over maintaining magic numbers.