Security fixes in plugin messaging (#1836)

* bugfix: wrong method

* security: handling packet on proxy

* refactor: reorder checks

* comments

* security: handling message on proxy

* comments: better

* style: following style guids

Author: SpigotRCE

compatibility/bungeecord-geyser/src/main/java/com/ghostchu/quickshop/compatibility/bungeecord.geyser/Main.java

@@ -2,6 +2,7 @@
 
 import com.google.common.io.ByteArrayDataOutput;
 import com.google.common.io.ByteStreams;
+import net.md_5.bungee.api.event.PluginMessageEvent;
 import net.md_5.bungee.api.event.ServerConnectedEvent;
 import net.md_5.bungee.api.plugin.Listener;
 import net.md_5.bungee.api.plugin.Plugin;
@@ -39,6 +40,18 @@ public void onDisable() {
     getProxy().unregisterChannel(BUNGEE_CHANNEL);
   }
 
+  @EventHandler
+  public void on(final PluginMessageEvent event) {
+    // Is this our business?
+    if(!BUNGEE_CHANNEL.equalsIgnoreCase(event.getTag())) {
+      return;
+    }
+    // Let's not be a snitch
+    // we don't want the client to send any message to the server
+    // nor do we want the proxy to send any message to the player
+    event.setCancelled(true);
+  }
+
   @EventHandler
   public void switchServer(final ServerConnectedEvent event) {
 

compatibility/bungeecord/src/main/java/com/ghostchu/quickshop/compatibility/bungeecord/Main.java

@@ -46,19 +46,24 @@ public void onDisable() {
 
   @EventHandler
   public void on(final PluginMessageEvent event) {
-
+    // Is this our business?
     if(!QUICKSHOP_BUNGEE_CHANNEL.equalsIgnoreCase(event.getTag())) {
       return;
     }
-
+    // Let's not be a snitch
+    // we don't want the client to send any message to the server
+    // nor do we want the proxy to send any message to the player
+    event.setCancelled(true);
+    // Is the source correct?
+    // we can only trust the server not the player
+    if(!(event.getSender() instanceof Server)) return; // Somebody is being nasty
+    // We can trust the source
+    // server sent us the message
     final ByteArrayDataInput in = ByteStreams.newDataInput(event.getData());
     final String subChannel = in.readUTF();
     if(SUB_CHANNEL_COMMAND.equalsIgnoreCase(subChannel)) {
-      // the receiver is a server when the proxy talks to a server
-      if(event.getReceiver() instanceof Server) {
-        final String command = in.readUTF();
-        processCommand(command, in);
-      }
+      final String command = in.readUTF();
+      processCommand(command, in);
     }
   }
 

compatibility/velocity/src/main/java/com/ghostchu/quickshop/compatibility/velocity/Main.java

@@ -124,18 +124,24 @@ public void onProxyShutdown(final ProxyShutdownEvent event) {
 
   @Subscribe
   public void on(final PluginMessageEvent event) {
-
+    // Is this our business?
     if(!QUICKSHOP_BUNGEE_CHANNEL.equals(event.getIdentifier())) {
       return;
     }
+    // Let's not be a snitch
+    // we don't want the client to send any message to the server
+    // nor do we want the proxy to send any message to the player
+    event.setResult(PluginMessageEvent.ForwardResult.handled());
+    // Is the source correct?
+    // we can only trust the server not the player
+    if(!(event.getSource() instanceof ServerConnection)) return;
+    // We can trust the source
+    // server sent us the message
     final ByteArrayDataInput in = event.dataAsDataStream();
     final String subChannel = in.readUTF();
     if(SUB_CHANNEL_COMMAND.equalsIgnoreCase(subChannel)) {
-      // the receiver is a server when the proxy talks to a server
-      if(event.getSource() instanceof ServerConnection) {
-        final String command = in.readUTF();
-        processCommand(command, in);
-      }
+      final String command = in.readUTF();
+      processCommand(command, in);
     }
   }
 
@@ -191,4 +197,4 @@ public void onServerKick(final ServerPostConnectEvent event) {
 
     pendingForward.remove(event.getPlayer().getUniqueId());
   }
-}
+}