Add protection against large network channel lists

This commit is contained in:
LexManos committed 2026-10-12 19:16:19 -07:00
1 parent 3534d4664b
commit d389a628a1
6 files changed
+63 -7

No files matched your search

@@ -107,7 +107,11 @@ public class ClientCommandHandler {
new CommandSource() {
@Override
public void sendSystemMessage(Component message) {
mc.gui.hud.getChat().addClientSystemMessage(message);
var chat = mc.gui.hud.getChat();
if (mc.isSameThread())
chat.addClientSystemMessage(message);
else
mc.execute(() -> chat.addClientSystemMessage(message));
}
@Override
@@ -75,6 +75,7 @@ public class ForgeConfig {
}
public final ForgeConfigSpec.EnumValue<MigrationHelperMode> migrationHelperMode;
public final ForgeConfigSpec.LongValue channelListLimit;
Common(ForgeConfigSpec.Builder builder) {
builder.comment("General configuration settings")
@@ -85,6 +86,17 @@ public class ForgeConfig {
.translation("forge.configgui.migrationHelperMode")
.defineEnum("logLegacyTagWarnings", MigrationHelperMode.OFF);
channelListLimit = builder
.comment(
"Maximum size to use to store the channel list for a network connection.",
"The other end of the connection may send as many channels as it wishes.",
"Some malicious actors may use this to use up all your memory causing OutOfMemoryException",
"A sane default is 1KB (1024), but may need to be expanded for larger modpacks.",
"Forge will auto-adjust this if mods register their channels using our channel manager."
)
.translation("forge.configui.channellistlimit")
.defineInRange("channelListLimit", 1024, 1, Long.MAX_VALUE);
builder.pop();
}
}
@@ -9,9 +9,11 @@ import com.mojang.logging.LogUtils;
import net.minecraft.IdentifierException;
import net.minecraft.network.Connection;
import net.minecraft.network.FriendlyByteBuf;
import net.minecraft.network.chat.Component;
import net.minecraft.network.codec.StreamCodec;
import net.minecraft.network.protocol.common.custom.CustomPacketPayload;
import net.minecraft.resources.Identifier;
import net.minecraftforge.common.ForgeConfig;
import net.minecraftforge.event.ForgeEventFactory;
import net.minecraftforge.event.network.ChannelRegistrationChangeEvent;
import net.minecraftforge.event.network.CustomPayloadEvent;
@@ -26,6 +28,7 @@ import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.stream.Collectors;
@ApiStatus.Internal // TODO: Decide what I want to have public, Right now it should just be add/remove channels.
public class ChannelListManager {
@@ -70,7 +73,7 @@ public class ChannelListManager {
}
private static void register(Register payload, CustomPayloadEvent.Context ctx) {
updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.REGISTER);
updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.REGISTER, ctx.isClientSide());
ctx.setPacketHandled(true);
// Send the client's channels to the server whenever asked. If we're in the login state then our login wrapper will unwrap for us.
if (ctx.isClientSide())
@@ -78,7 +81,7 @@ public class ChannelListManager {
}
private static void unregister(Unregister payload, CustomPayloadEvent.Context ctx) {
updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.UNREGISTER);
updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.UNREGISTER, ctx.isClientSide());
ctx.setPacketHandled(true);
}
@@ -145,7 +148,7 @@ public class ChannelListManager {
return Collections.unmodifiableList(channels);
}
private static void updateFrom(CustomPayloadEvent.Context source, List<String> channels, final ChannelRegistrationChangeEvent.Type changeType) {
private static void updateFrom(CustomPayloadEvent.Context source, List<String> channels, final ChannelRegistrationChangeEvent.Type changeType, boolean isClient) {
var changed = new HashSet<Identifier>();
for (var channel : channels) {
// It also says nothing about the format of channels so ignore bad channels.
@@ -162,11 +165,20 @@ public class ChannelListManager {
ForgeEventFactory.onChannelRegistrationChange(source.getConnection(), changeType, changed);
// It is a known issue, that I don't really care about, that we don't get the true in-memory size of each channel due to character encoding.
// A sane limit of 1KB will be enough to catch any malicious actors even with non-8-bit strings.
var list = NetworkContext.get(source.getConnection());
for (var channel : changed) {
boolean fire = changeType == ChannelRegistrationChangeEvent.Type.UNREGISTER
? list.remoteChannels.remove(channel)
: list.remoteChannels.add(channel);
boolean fire = false;
if (changeType == ChannelRegistrationChangeEvent.Type.UNREGISTER) {
fire = list.remoteChannels.remove(channel);
if (fire)
list.remoteChannelsSize -= channel.toString().length();
} else {
fire = list.remoteChannels.add(channel);
if (fire)
list.remoteChannelsSize += channel.toString().length();
}
if (fire) {
var target = NetworkRegistry.findTarget(channel);
@@ -174,5 +186,21 @@ public class ChannelListManager {
target.registrationChange(channel, changeType == ChannelRegistrationChangeEvent.Type.REGISTER);
}
}
if (list.remoteChannelsSize >= NetworkRegistry.getChannelMemorySize() && list.remoteChannelsSize >= ForgeConfig.COMMON.channelListLimit.get()) {
LOGGER.warn("Received too many network channels, {} channels using {} bytes, Limit is currently {}, to change edit the Forge {} in config\forge_common.toml",
list.remoteChannels.size(), list.remoteChannelsSize, ForgeConfig.COMMON.channelListLimit.get(), ForgeConfig.COMMON.channelListLimit.getPath().stream().collect(Collectors.joining("."))
);
if (isClient) {
// We don't want to allow the server to screw with us, so just remove the added channels
for (var channel : changed) {
if (list.remoteChannels.remove(channel))
list.remoteChannelsSize -= channel.toString().length();
}
} else {
source.getSender().connection.disconnect(Component.translatable("forge.server.channellistlimit.kick"));
}
}
}
}
@@ -111,6 +111,8 @@ public class NetworkContext {
@ApiStatus.Internal
Set<Identifier> remoteChannels = new HashSet<>();
@ApiStatus.Internal
long remoteChannelsSize = 0;
private Set<Identifier> remoteChannelsView = Collections.unmodifiableSet(this.remoteChannels);
@ApiStatus.Internal
@@ -43,6 +43,7 @@ public class NetworkRegistry {
private static Map<Identifier, NetworkInstance> instances = Collections.synchronizedMap(new HashMap<>());
private static Map<Identifier, NetworkInstance> byName = Collections.synchronizedMap(new HashMap<>());
private static long memorySize = 0;
public static boolean acceptsVanillaClientConnections() {
return listRejectedVanillaMods(n -> n.clientAcceptedVersions).isEmpty() && DataPackRegistriesHooks.getSyncedCustomRegistries().isEmpty();
@@ -199,9 +200,14 @@ public class NetworkRegistry {
if (NetworkRegistry.byName.containsKey(name))
error("Payload name " + name + " already registered.");
memorySize += name.toString().length(); // This can fail if people don't use our api. So this is a best effort.
byName.put(name, instance);
}
static long getChannelMemorySize() {
return memorySize;
}
static void register(NetworkInstance instance) {
checkLock(instance);
@@ -200,6 +200,10 @@
"forge.configgui.stabilizeDirectionGetNearest": "Stabilize Direction Get Nearest",
"forge.configgui.stabilizeDirectionGetNearest.tooltip": "When enabled, a slightly biased Direction#getNearest calculation will be used to prevent normal fighting on 45 degree angle faces.",
"forge.configgui.channellistlimit": "Channel List Size Limit",
"forge.configgui.channellistlimit.tooltip": "Maximum size to use to store the channel list for a network connection. Some malicious actors may use this to use up all your memory. A sane default is 1KB (1024), but may need to be expanded for larger modpacks.",
"forge.server.channellistlimit.kick": "Received too many networking channels",
"forge.controlsgui.shift": "SHIFT + %s",
"forge.controlsgui.control": "CTRL + %s",
"forge.controlsgui.control.mac": "CMD + %s",