kangarko / ChatControl-Red

Issue tracker and documentation for the next generation ChatControl Red, the most advanced chat management plugin.
49 stars 23 forks source link

1.19.3: Command Packets Issue #2271

Closed gre3x closed 1 year ago

gre3x commented 1 year ago

"/version ChatControlRed" - plugin version

10.16.8

Are you using MySQL?

Yes

Are you using BungeeCord?

Yes

Error log (if applicable)

No response

"/chc debug" output (strongly recommended)

No response

Information about the issue/bug

How do I prevent this from being used on my server?

gre3x commented 1 year ago

Please review with high priority due to the high potential impact

kangarko commented 1 year ago

Reviewing now, thank you for submitting.

kangarko commented 1 year ago

Feature temporarily disabled by default via a config key Enable_Forward_Command in BungeeControl Red until this is fully investigated and resolved. A release will be out tonight.

gre3x commented 1 year ago

@kangarko can you please make it a configurable whitelist of commands that are allowed to be sent as a forwarded commands instead? That way we dont have to disable the entire feature.

Something like:

forward_commands_whitelist:
- /give
- /say
- etc
kangarko commented 1 year ago

That still does not resolve the vulnerability, clients could just send /give commands to give themselves any item or I am mistaken? I am rather looking for some nonce or token verification

kangarko commented 1 year ago

@gre3x do you have the option to test the potential fix when it comes out? I don't posses a hacked client with that ability at the moment

gre3x commented 1 year ago

The whitelist wouldn't resolve the issue but it would allow for the temporary fix to not be as impactful if the server only wants to use forwarded commands for limited things (which I think is what most servers would use this feature for)

I do not have the option to test the potential fix when it comes out

gre3x commented 1 year ago

@kangarko are you able to share an approximate ETA for when the fix will be out?

kangarko commented 1 year ago

I identified a potential fix to broadcast on the BungeeCord channel instead, unfortunately I wasn't able to work due to disease. Probably gonna start working on it by tomorrow so by Monday it should be in production.

gre3x commented 1 year ago

Sounds good thank you @kangarko !

kangarko commented 1 year ago

@ElBananaa @TheIntolerant do you remember what kind of issues we had when I changed the plugin messaging channel to "BungeeCord"? (That was in attempt to solve "unknown packed..." client log spam. Apparently reverting back has opened this vulnerability without me knowing so we have to use BungeeCord channel.

ElBananaa commented 1 year ago

@ElBananaa @TheIntolerant do you remember what kind of issues we had when I changed the plugin messaging channel to "BungeeCord"? (That was in attempt to solve "unknown packed..." client log spam. Apparently reverting back has opened this vulnerability without me knowing so we have to use BungeeCord channel.

As far as I remember, the main issue was the unknown custom packed identifier client log spam. I'm not sure how other plugins handle this, but simply making sure plugin messages are not sent by players wouldn't fix the issue?

I guess you could also try to somehow authenticate servers (you could automatically generate a unique random 32 chars id assigned to each server, then simply deny plugin messages not coming from these servers...) I guess they are many ways to fix this, the whole point is to find the best way to do so. That's something you should definitely have a look with other plugin devs since they'll most likely have better answers about this x:

kangarko commented 1 year ago

I am gonna try doing what AuthMeBungee is doing https://github.com/AuthMe/AuthMeBungee/blob/master/src/main/java/fr/xephi/authmebungee/listeners/BungeeMessageListener.java#L51 and see if that helps.

Will be sending a test build here tonight.

kangarko commented 1 year ago

Alright I have tested my patch on two servers and three clients WITHOUT directly testing the vulnerability, I just rewrote it around how AuthMe does it > if you have access to such a hacked client please email a copy at matej@mineacademy.org so I can test it.

The build is out in production, let me know if you still spot anything suspicious, and if not, I will make a public announcement since this is quite of a high severity.

kangarko commented 1 year ago

I appreciate your patience!

kangarko commented 1 year ago

Since we both don't posses a client to test whether or not this patch solved the hole, I need to find a way to test it.

Sending a public broadcast would be risky in case it did not solve it, people could start exploiting it. Any ideas?

I don't have the time nor skill to edit the client at the moment. Pinging @TheIntolerant @ElBananaa @james090500 if you guys have an idea I'd appreciate it!

gre3x commented 1 year ago

@kangarko After updating to the latest versions, I am getting this spammed in the consoles of all of my spigot and bungee servers. Forwarding commands seem to work, but after a few minutes, if I attempt to forward a command to the Bungee it just crashes the bungee.

Is this expected? Is there anything i can do to fix this?

Console spam. This exception is sent for each of my spigot servers (replace "Lobby" with the names of all of my spigot servers in "Invalid UUID string: Lobby"):

https://pastebin.com/RQxLnU2Q

ElBananaa commented 1 year ago

Did you update bungeecontrol too?

gre3x commented 1 year ago

Yes @ElBananaa [14:29:23] [main/INFO]: Enabled plugin BungeeControl-Red version 3.13.6 by kangarko

gre3x commented 1 year ago

I updated the previous comment with the full trace

gre3x commented 1 year ago

This is the exception from the spigot servers:

[ChatControlRed] Plugin ChatControlRed v10.16.12 generated an exception whilst handling plugin message
java.lang.IllegalArgumentException: Invalid UUID string: Lobby
    at java.util.UUID.fromString1(UUID.java:280) ~[?:?]
    at java.util.UUID.fromString(UUID.java:258) ~[?:?]
    at org.mineacademy.chatcontrol.lib.bungee.BungeeListener$BungeeListenerImpl.onPluginMessageReceived(BungeeListener.java:142) ~[ChatControl-Red-10.16.12.jar:?]
    at org.bukkit.plugin.messaging.StandardMessenger.dispatchIncomingMessage(StandardMessenger.java:455) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.server.network.ServerGamePacketListenerImpl.handleCustomPayload(ServerGamePacketListenerImpl.java:3267) ~[app:?]
    at net.minecraft.network.protocol.game.ServerboundCustomPayloadPacket.handle(ServerboundCustomPayloadPacket.java:38) ~[app:?]
    at net.minecraft.network.protocol.game.ServerboundCustomPayloadPacket.handle(ServerboundCustomPayloadPacket.java:7) ~[app:?]
    at net.minecraft.network.protocol.PacketUtils.lambda$ensureRunningOnSameThread$1(PacketUtils.java:56) ~[app:?]
    at net.minecraft.server.TickTask.run(TickTask.java:18) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.util.thread.BlockableEventLoop.doRunTask(BlockableEventLoop.java:149) ~[app:?]
    at net.minecraft.util.thread.ReentrantBlockableEventLoop.doRunTask(ReentrantBlockableEventLoop.java:23) ~[app:?]
    at net.minecraft.server.MinecraftServer.doRunTask(MinecraftServer.java:1450) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.server.MinecraftServer.executeTask(MinecraftServer.java:192) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.util.thread.BlockableEventLoop.pollTask(BlockableEventLoop.java:122) ~[app:?]
    at net.minecraft.server.MinecraftServer.pollTaskInternal(MinecraftServer.java:1428) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.server.MinecraftServer.pollTask(MinecraftServer.java:1421) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.util.thread.BlockableEventLoop.managedBlock(BlockableEventLoop.java:132) ~[app:?]
    at net.minecraft.server.MinecraftServer.waitUntilNextTick(MinecraftServer.java:1399) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1310) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:322) ~[patched_1.17.1.jar:git-Purpur-"08dd6c7"]
    at java.lang.Thread.run(Thread.java:831) ~[?:?]
ElBananaa commented 1 year ago

@kangarko

kangarko commented 1 year ago

Investigating

kangarko commented 1 year ago

Found the cause. I have forgot to make sure that we only listen to our own plugin message channel since other plugins have a different data structure. The error is simply complaining that a third party plugin uses unknown data structure to us so I will add another security check to only read our own plugin messages.

Apologizes for the inconvenience.

kangarko commented 1 year ago

@james090500 please ping us here when you have the time to check if Velocity is compatible with the latest changes of BungeeFoundation, namely we now use BungeeCord as plugin message channel name

james090500 commented 1 year ago

Velocity can't listen to the bungee channel. So I can't implement these changes. You still need to keep it on a plugin:chcred channel .

kangarko commented 1 year ago

Ooh I forgot about this. But sending on "chcred" leaves the security hole opened. Do you have an idea how to solve this?

ElBananaa commented 1 year ago

@kangarko You'll probably find some useful tips on there. But basically, it seems like you can simply check if the sender and/or the receiver of the plugin message is a server or a player. So you could simply block any incoming plugin message on the plugin:chcred channel if it's sent by a player. (This is basically what's suggested on the github link i sent below)

https://www.spigotmc.org/wiki/sending-a-custom-plugin-message-from-bungeecord/ https://www.spigotmc.org/threads/safety-of-the-plugin-messaging-channels.420327/ https://github.com/SpigotMC/BungeeCord/issues/3191

gre3x commented 1 year ago

@kangarko just confirming, do the latest production versions have the security issue patched?

ElBananaa commented 1 year ago

@gre3x It should, yes. Simply read the update log for the previous updates on builtbybit

kangarko commented 1 year ago

Yes it does it althought I did not test it since I dont posses such a hacked client, I modeled AuthMeBungee for the patch so I am pretty confident it got patched.

I am investigating possible solutions to VelocityControl rn

kangarko commented 1 year ago

@james090500 are you sure Velocity the latest version does not support "BungeeCord" channel still? According to what I found they seemed to have added that feature:

https://stackoverflow.com/questions/71283635/how-can-i-use-bungeecord-message-channel-on-velocity https://github.com/VelocityPowered/BungeeQuack https://github.com/PaperMC/Velocity/blob/1ec77eb1236c26d54823b15bb0147532dbf25d2e/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BungeeCordMessageResponder.java#L52

kangarko commented 1 year ago

@james090500 so just a headsup, the latest ChatControl does only use the "BungeeCord" channel to broadcast plugin messages, you can see the latest structure of the packet here:

a

So you'd need to listen to BungeeCord channel and then determine the actual channel name such as "plugin:chcred" from the first utf string

gre3x commented 1 year ago

@kangarko would player's with the "chatcontrol.command.forward" permission still be able to use this packet exploit? Or is the permission only for using the /chc forward command and no level of permissions could enable this packet exploit?

gre3x commented 1 year ago

or @ElBananaa ^

kangarko commented 1 year ago

With forward permission the player can send any forward instructions so yes, but it is no longer considered an exploit because it is permission-based feature.

gre3x commented 1 year ago

@kangarko I mean if the player had the "chatcontrol.command.forward" permission, could they just send the command forward packet instead of typing the command? Or would they still need to type the command?

kangarko commented 1 year ago

Since the server does know if the player typed the command, it only receives the packet, they could put this in their client as a packet too.