Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stronger ticket tag checks #39

Merged
merged 2 commits into from
Apr 14, 2023
Merged

stronger ticket tag checks #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14da23a
stronger ticket tag checks
Glease Apr 14, 2023
a204ae8
fix too strong check
Glease Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/main/java/mods/railcraft/common/items/ItemRoutingTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;
import java.util.Set;

import mods.railcraft.api.core.items.IStackFilter;
import mods.railcraft.client.gui.GuiRoutingTable;
Expand All @@ -30,6 +31,9 @@
import net.minecraft.nbt.NBTTagString;
import net.minecraft.util.EnumChatFormatting;
import net.minecraft.world.World;
import net.minecraftforge.common.util.Constants;

import com.google.common.collect.ImmutableSet;

import cpw.mods.fml.relauncher.Side;
import cpw.mods.fml.relauncher.SideOnly;
Expand All @@ -39,6 +43,8 @@
*/
public class ItemRoutingTable extends ItemRailcraft implements IEditableItem {

private static final Set<String> editableKeys = ImmutableSet.of("title", "author", "pages");

public static final IStackFilter FILTER = new IStackFilter() {

@Override
Expand Down Expand Up @@ -71,22 +77,24 @@ public static ItemStack getItem() {
}

@Override
public boolean validateNBT(NBTTagCompound nbt) {
public boolean validateNBT(ItemStack oldStack, NBTTagCompound nbt) {
// player can edit title, author and pages, and nothing else
if (!IEditableItem.checkValidModification(oldStack.stackTagCompound, nbt, editableKeys)) return false;
return validBookTagContents(nbt);
}

public static boolean validBookTagContents(NBTTagCompound nbt) {
if (!validBookTagPages(nbt)) return false;
else if (nbt.hasKey("title")) {
else if (nbt.hasKey("title", Constants.NBT.TAG_STRING)) {
String s = nbt.getString("title");
return s != null && s.length() <= 16 ? nbt.hasKey("author") : false;
return s != null && s.length() <= 16 ? nbt.hasKey("author", Constants.NBT.TAG_STRING) : false;
}
return true;
}

public static boolean validBookTagPages(NBTTagCompound nbt) {
if (nbt == null) return false;
else if (!nbt.hasKey("pages")) return false;
else if (!nbt.hasKey("pages", Constants.NBT.TAG_LIST)) return false;
else {
NBTList<NBTTagList> pages = NBTPlugin.getNBTList(nbt, "pages", NBTPlugin.EnumNBTType.LIST);
for (NBTTagList pageNBT : pages) {
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/mods/railcraft/common/items/ItemTicket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,17 @@
import mods.railcraft.common.plugins.forge.PlayerPlugin;
import mods.railcraft.common.plugins.forge.RailcraftRegistry;
import mods.railcraft.common.util.inventory.InvTools;
import mods.railcraft.common.util.network.IEditableItem;

import net.minecraft.client.renderer.texture.IIconRegister;
import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.item.ItemStack;
import net.minecraft.nbt.NBTTagCompound;
import net.minecraft.nbt.NBTTagString;
import net.minecraft.util.EnumChatFormatting;
import net.minecraftforge.common.util.Constants;

import com.google.common.collect.ImmutableSet;
import com.mojang.authlib.GameProfile;

import cpw.mods.fml.relauncher.Side;
Expand All @@ -32,6 +35,7 @@
*/
public class ItemTicket extends ItemRailcraft {

private static final ImmutableSet<String> editableKeys = ImmutableSet.of("dest", "owner", "ownerId");
public static final IStackFilter FILTER = new IStackFilter() {

@Override
Expand Down Expand Up @@ -110,7 +114,9 @@ public static GameProfile getOwner(ItemStack ticket) {
return PlayerPlugin.readOwnerFromNBT(nbt);
}

public boolean validateNBT(NBTTagCompound nbt) {
public boolean validateNBT(ItemStack oldStack, NBTTagCompound nbt) {
if (!IEditableItem.checkValidModification(oldStack.stackTagCompound, nbt, editableKeys)) return false;
if (!nbt.hasKey("dest", Constants.NBT.TAG_STRING)) return false;
String dest = nbt.getString("dest");
return dest.length() < LINE_LENGTH;
}
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/mods/railcraft/common/util/misc/Game.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.Message;
import org.apache.logging.log4j.message.MessageFormatMessage;

Expand All @@ -27,6 +28,7 @@ public class Game {
public static final boolean IS_OBFUSCATED = !World.class.getSimpleName().equals("World");
public static final boolean IS_DEBUG = !Railcraft.VERSION.endsWith("0");
public static final boolean IS_BUKKIT;
public static final Logger LOGGER = LogManager.getLogger(Railcraft.MOD_ID);
public static boolean isGTNH;

static {
Expand Down Expand Up @@ -64,7 +66,7 @@ public static void log(Level level, String msg, Object... args) {
}

public static void log(Level level, Message msg) {
LogManager.getLogger(Railcraft.MOD_ID).log(level, msg);
LOGGER.log(level, msg);
}

public static void logTrace(Level level, String msg, Object... args) {
Expand Down
28 changes: 27 additions & 1 deletion src/main/java/mods/railcraft/common/util/network/IEditableItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
*/
package mods.railcraft.common.util.network;

import java.util.HashSet;
import java.util.Set;

import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.item.ItemStack;
import net.minecraft.nbt.NBTTagCompound;
Expand All @@ -15,7 +18,30 @@
*/
public interface IEditableItem {

boolean validateNBT(NBTTagCompound nbt);
boolean validateNBT(ItemStack oldStack, NBTTagCompound nbt);

boolean canPlayerEdit(EntityPlayer player, ItemStack stack);

/**
* @return true if edit only exists if editableKeys, false otherwise
*/
static boolean checkValidModification(NBTTagCompound oldTag, NBTTagCompound newTag, Set<String> editableKeys) {
Set<String> oldKeys = new HashSet<>();
// if there is any existing key, the edited tag should preserve it as is.
if (oldTag != null) {
for (Object o : oldTag.func_150296_c()) {
String key = (String) o;
if (editableKeys.contains(key)) continue;
if (!oldTag.getTag(key).equals(newTag.getTag(key))) return false;
oldKeys.add(key);
}
}
// it should not add any keys not present in the oldTag or is otherwise open to edit
for (Object o : newTag.func_150296_c()) {
String key = (String) o;
if (oldKeys.contains(key) || editableKeys.contains(key)) continue;
return false;
}
return true;
}
}
17 changes: 9 additions & 8 deletions src/main/java/mods/railcraft/common/util/network/PacketCurrentItemNBT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,19 @@
import java.util.HashSet;
import java.util.Set;

import mods.railcraft.common.core.Railcraft;
import mods.railcraft.common.util.inventory.InvTools;
import mods.railcraft.common.util.misc.Game;

import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.item.ItemStack;
import net.minecraft.nbt.NBTTagCompound;

import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.Marker;
import org.apache.logging.log4j.MarkerManager;

public class PacketCurrentItemNBT extends RailcraftPacket {

private static final Marker SECURITY_MARKER = MarkerManager.getMarker("SuspiciousPackets");
private static final Set<String> ALLOWED_TAGS = new HashSet<>(Arrays.asList("title", "author", "pages", "dest"));

private final EntityPlayer player;
Expand Down Expand Up @@ -53,16 +54,16 @@ public void readData(DataInputStream data) throws IOException {
IEditableItem eItem = (IEditableItem) stack.getItem();

if (!eItem.canPlayerEdit(player, currentItem)) {
Game.log(
Level.WARN,
"{0} attempted to edit an item he is not allowed to edit {0}.",
Railcraft.proxy.getPlayerUsername(player),
Game.LOGGER.warn(
SECURITY_MARKER,
"Player {} attempted to edit an item he is not allowed to edit {}.",
player.getGameProfile(),
currentItem.getItem().getUnlocalizedName());
return;
}

if (!eItem.validateNBT(stack.getTagCompound())) {
Game.log(Level.WARN, "Item NBT not valid!");
if (!eItem.validateNBT(currentItem, stack.getTagCompound())) {
Game.LOGGER.warn(SECURITY_MARKER, "Player {}: Item NBT not valid!", player.getGameProfile());
return;
}

Expand Down