From dc18139a02adeeb499a77ffd6438a4cd58d4b17f Mon Sep 17 00:00:00 2001 From: A248 Date: Fri, 8 Mar 2024 11:25:04 -0500 Subject: [PATCH] Finally solve detection of the plugin messaging channel (#237) Channel detection is now re-fixed. After hours of testing, hopefully this will be the last time. --- .../core/env/AbstractEnvEnforcer.java | 28 ------------------ .../libertybans/core/env/EnvEnforcer.java | 6 ++-- .../core/punish/StandardLocalEnforcer.java | 16 +++++++--- .../core/scope/ServerNameListenerBase.java | 29 +++++++++++++++++-- .../env/spigot/SpigotMessageChannel.java | 7 ++--- .../env/standalone/StandaloneEnforcer.java | 2 +- 6 files changed, 46 insertions(+), 42 deletions(-) diff --git a/bans-core/src/main/java/space/arim/libertybans/core/env/AbstractEnvEnforcer.java b/bans-core/src/main/java/space/arim/libertybans/core/env/AbstractEnvEnforcer.java index 73f9476af..8a627a16a 100644 --- a/bans-core/src/main/java/space/arim/libertybans/core/env/AbstractEnvEnforcer.java +++ b/bans-core/src/main/java/space/arim/libertybans/core/env/AbstractEnvEnforcer.java @@ -21,12 +21,8 @@ import net.kyori.adventure.text.Component; import net.kyori.adventure.text.ComponentLike; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import space.arim.api.env.AudienceRepresenter; import space.arim.libertybans.core.config.InternalFormatter; -import space.arim.libertybans.core.env.message.PluginMessage; -import space.arim.omnibus.util.ThisClass; import space.arim.omnibus.util.concurrent.CentralisedFuture; import space.arim.omnibus.util.concurrent.FactoryOfTheFuture; @@ -42,8 +38,6 @@ public abstract class AbstractEnvEnforcer

implements EnvEnforcer

{ private final Interlocutor interlocutor; private final AudienceRepresenter audienceRepresenter; - private static final Logger logger = LoggerFactory.getLogger(ThisClass.get()); - protected AbstractEnvEnforcer(FactoryOfTheFuture futuresFactory, InternalFormatter formatter, Interlocutor interlocutor, AudienceRepresenter audienceRepresenter) { this.futuresFactory = Objects.requireNonNull(futuresFactory, "futuresFactory"); @@ -94,28 +88,6 @@ private CentralisedFuture sendToThoseWithPermissionNoPrefix(String permiss return doForAllPlayers((players) -> players.forEach(callback)); } - @Override - public final void sendPluginMessage(P player, PluginMessage pluginMessage, D data) { - if (!sendPluginMessageIfListening(player, pluginMessage, data)) { - logger.error( - "Attempted to send plugin message to {}, but it could not be sent. " + - "This suggests you enabled 'use-plugin-messaging' but are not using a network. " + - "Please address this critical security flaw immediately. " + - "It leaves your server vulnerable to clients spoofing the plugin messaging channel.", - player - ); - } - } - - /** - * Sends a plugin message to the given player if it is accepted by their client - * - * @param player the player to whom to send the message - * @param pluginMessage the plugin message - * @return true if sent, false if unsupported - */ - public abstract boolean sendPluginMessageIfListening(P player, PluginMessage pluginMessage, D data); - @Override public final void sendMessageNoPrefix(P player, ComponentLike message) { audienceRepresenter.toAudience(player).sendMessage(message); diff --git a/bans-core/src/main/java/space/arim/libertybans/core/env/EnvEnforcer.java b/bans-core/src/main/java/space/arim/libertybans/core/env/EnvEnforcer.java index eabae3a9f..38c7806c8 100644 --- a/bans-core/src/main/java/space/arim/libertybans/core/env/EnvEnforcer.java +++ b/bans-core/src/main/java/space/arim/libertybans/core/env/EnvEnforcer.java @@ -79,12 +79,14 @@ public interface EnvEnforcer<@PlatformPlayer P> { void kickPlayer(P player, Component message); /** - * Sends a plugin message to the given player. Must be used only for proxies. + * Sends a plugin message to the given player if it is accepted by their client. Only to be used for backend + * servers connected by a network. * * @param player the player to whom to send the message * @param pluginMessage the plugin message + * @return true if sent, false if unsupported */ - void sendPluginMessage(P player, PluginMessage pluginMessage, D data); + boolean sendPluginMessageIfListening(P player, PluginMessage pluginMessage, D data); /** * Sends a message to the given player. Does not include a prefix.
diff --git a/bans-core/src/main/java/space/arim/libertybans/core/punish/StandardLocalEnforcer.java b/bans-core/src/main/java/space/arim/libertybans/core/punish/StandardLocalEnforcer.java index a8413fc01..b71b589f9 100644 --- a/bans-core/src/main/java/space/arim/libertybans/core/punish/StandardLocalEnforcer.java +++ b/bans-core/src/main/java/space/arim/libertybans/core/punish/StandardLocalEnforcer.java @@ -246,12 +246,20 @@ private Consumer

enforcementCallback(Punishment punishment, Component message case BAN, KICK -> (player) -> { if (instanceType == InstanceType.GAME_SERVER && configs.getMainConfig().platforms().gameServers().usePluginMessaging()) { - envEnforcer.sendPluginMessage( + // Try to kick by plugin message + if (envEnforcer.sendPluginMessageIfListening( player, new KickPlayer(), new KickPlayer.Data(envEnforcer.getNameFor(player), message) - ); - } else { - envEnforcer.kickPlayer(player, message); + )) { + // Kicked by plugin message + return; + } + // This scenario is a remote possibility if the player joins and is very quickly kicked. + // That is, the plugin messaging channel is activated a small period after the join event. + // Using the existing APIs on Bukkit and Sponge, there is no solution to this problem without + // sacrificing the following important warning message for misconfigured setups. + logger.warn("Attempted to send plugin message to {}, but it could not be sent.", player); } + envEnforcer.kickPlayer(player, message); }; case MUTE -> (player) -> { /* diff --git a/bans-core/src/main/java/space/arim/libertybans/core/scope/ServerNameListenerBase.java b/bans-core/src/main/java/space/arim/libertybans/core/scope/ServerNameListenerBase.java index f7ae3bb6a..558957a2f 100644 --- a/bans-core/src/main/java/space/arim/libertybans/core/scope/ServerNameListenerBase.java +++ b/bans-core/src/main/java/space/arim/libertybans/core/scope/ServerNameListenerBase.java @@ -21,6 +21,7 @@ import jakarta.inject.Inject; import jakarta.inject.Singleton; +import org.slf4j.LoggerFactory; import space.arim.api.env.annote.PlatformPlayer; import space.arim.libertybans.core.config.Configs; import space.arim.libertybans.core.env.EnvEnforcer; @@ -28,13 +29,18 @@ import space.arim.libertybans.core.env.InstanceType; import space.arim.libertybans.core.env.PlatformListener; import space.arim.libertybans.core.env.message.GetServer; +import space.arim.omnibus.util.concurrent.EnhancedExecutor; +import space.arim.omnibus.util.concurrent.FactoryOfTheFuture; +import java.time.Duration; import java.util.function.Consumer; @Singleton public final class ServerNameListenerBase<@PlatformPlayer P, H> implements PlatformListener { private final Configs configs; + private final FactoryOfTheFuture futuresFactory; + private final EnhancedExecutor enhancedExecutor; private final InternalScopeManager scopeManager; private final EnvEnforcer

envEnforcer; private final EnvMessageChannel envMessageChannel; @@ -42,8 +48,11 @@ public final class ServerNameListenerBase<@PlatformPlayer P, H> implements Platf private final H handler; @Inject - public ServerNameListenerBase(InstanceType instanceType, Configs configs, InternalScopeManager scopeManager, + public ServerNameListenerBase(InstanceType instanceType, Configs configs, FactoryOfTheFuture futuresFactory, + EnhancedExecutor enhancedExecutor, InternalScopeManager scopeManager, EnvEnforcer

envEnforcer, EnvMessageChannel envMessageChannel) { + this.futuresFactory = futuresFactory; + this.enhancedExecutor = enhancedExecutor; if (instanceType != InstanceType.GAME_SERVER) { throw new IllegalStateException("Cannot use server name listener except for backend game servers"); } @@ -71,7 +80,23 @@ public void onJoin(P player) { if (configs.getMainConfig().platforms().gameServers().usePluginMessaging() && configs.getScopeConfig().serverName().autoDetect() && scopeManager.serverNameUndetected()) { - envEnforcer.sendPluginMessage(player, new GetServer(), null); + + // After the join event, sending a plugin message needs to happen on a delay + enhancedExecutor.scheduleOnce( + () -> { + // Recheck some conditions + if (scopeManager.serverNameUndetected()) { + futuresFactory.runSync(() -> { + envEnforcer.sendPluginMessageIfListening(player, new GetServer(), null); + }).exceptionally((ex) -> { + LoggerFactory.getLogger(getClass()).warn("Exception requesting backend server name", ex); + return null; + }); + } + }, + // 4 seconds is sufficient. If no response after 4 seconds, something is wrong with the proxy. + Duration.ofSeconds(4L) + ); } } diff --git a/bans-env/spigot/src/main/java/space/arim/libertybans/env/spigot/SpigotMessageChannel.java b/bans-env/spigot/src/main/java/space/arim/libertybans/env/spigot/SpigotMessageChannel.java index c2e566d40..dd056b59a 100644 --- a/bans-env/spigot/src/main/java/space/arim/libertybans/env/spigot/SpigotMessageChannel.java +++ b/bans-env/spigot/src/main/java/space/arim/libertybans/env/spigot/SpigotMessageChannel.java @@ -55,12 +55,9 @@ boolean sendPluginMessage(Player player, PluginMessage pluginMessage, boolean listened = player.getListeningPluginChannels().contains(BUNGEE_CHANNEL); // // 1. The backend server must NOT be in online mode - // 2. The channel must NOT be listened on, strangely enough - // Explanation: getListeningPluginChannels() should never return BungeeCord, because it is a special channel - // Therefore, if this channel IS listened on, that suggests the client is trying to spoof it. - // The anti-spoof check is used for pre-1.13 clients which may register legacy channel names. + // 2. The channel must be listened on // - boolean canSend = !plugin.getServer().getOnlineMode() && !listened; + boolean canSend = !plugin.getServer().getOnlineMode() && listened; if (canSend) { player.sendPluginMessage( plugin, BUNGEE_CHANNEL, diff --git a/bans-env/standalone/src/main/java/space/arim/libertybans/env/standalone/StandaloneEnforcer.java b/bans-env/standalone/src/main/java/space/arim/libertybans/env/standalone/StandaloneEnforcer.java index a6c386a1d..b194fc480 100644 --- a/bans-env/standalone/src/main/java/space/arim/libertybans/env/standalone/StandaloneEnforcer.java +++ b/bans-env/standalone/src/main/java/space/arim/libertybans/env/standalone/StandaloneEnforcer.java @@ -66,7 +66,7 @@ public void kickPlayer(Void player, Component message) { } @Override - public void sendPluginMessage(Void player, PluginMessage pluginMessage, D data) { + public boolean sendPluginMessageIfListening(Void player, PluginMessage pluginMessage, D data) { throw new UnsupportedOperationException(); }