EntityDestroyPacket

SECURITY.md

@@ -0,0 +1,55 @@
+
+## Vulnerabilities in original engine
+
+This document points out vulnerabilities in original game engine and describes vectors of attack
+to exploit them.
+
+This document is for educational purposes only to raise awareness (about learning how dangerous it is to run public Starbound
+server on original engine), and pursues no goal of harming users of original engine.
+
+Experienced blackhats already could take sources and dig these invulnerabilities themselves,
+since most of them are not buried anywhere deep in code.
+
+-----------
+
+### EntityDestroyPacket vulnerability
+
+When client sends EntityCreatePacket to WorldServer, it checks whenever received `entityId` is within
+allowed range (range of IDs allocated specifically for that client). Same happens on EntityUpdateSetPacket.
+
+However, someone forgot to put the same check when receiving EntityDestroyPacket, hence
+any client can remove ANY other entity inside world, including other PlayerEntitys'.
+
+On side note, original client makes sure it sends EntityDestroyPacket only for entities it owns.
+
+This attack require modified game client.
+
+-----------
+
+### Zip bomb in PacketSocket
+
+When packets are received on network socket, they are checked for not exceeding 16 MiB,
+by reading packet length header. However, when receiving compressed packets,
+only compressed size is checked against 16 MiB limit, and
+they are uncompressed in one shot, without limiting uncompressed size.
+
+This vulnerability allows to make server quickly run out of memory by forging zip-bomb packet.
+
+This attack require modified game client.
+
+-----------
+
+### Client's ShipWorld size
+
+When joining server, client sends contents of `.shipworld` in form of chunk map
+(Map with bytearray keys and bytearray values, which represent data stored inside BTreeDB).
+
+Server instances WorldServer with provided world chunks. The vulnerability lies within world's size.
+
+Original engine world's chunk map is always stored as tight 2D array of chunk (sector) pointers,
+and pointer array is always fully preallocated when world is instanced.
+
+So client can forge custom shipworld, with 2^31 x 2^31 dimensions, which will instantly cause
+server to consume at least 128 GiB of RAM when client connects.
+
+This attack does not require modified game client.

src/main/kotlin/ru/dbotthepony/kstarbound/Main.kt

@@ -103,8 +103,8 @@ fun main() {
 
 	Starbound.mailboxInitialized.submit {
 		val server = IntegratedStarboundServer(File("./"))
-		val world = ServerWorld.create(server, WorldGeometry(Vector2i(3000, 2000), true, false), LegacyWorldStorage.file(db))
+		//val world = ServerWorld.create(server, WorldGeometry(Vector2i(3000, 2000), true, false), LegacyWorldStorage.file(db))
-		world.thread.start()
+		//world.thread.start()
 
 		//ply = PlayerEntity(client.world!!)
 
@@ -113,7 +113,7 @@ fun main() {
 
 		//client.world!!.parallax = Starbound.parallaxAccess["garden"]
 
-		client.connectToLocalServer(server.channels.createLocalChannel())
+		//client.connectToLocalServer(server.channels.createLocalChannel())
 		//client.connectToRemoteServer(InetSocketAddress("127.0.0.1", 21025))
 		//client2.connectToLocalServer(server.channels.createLocalChannel(), UUID.randomUUID())
 		server.channels.createChannel(InetSocketAddress(21060))

src/main/kotlin/ru/dbotthepony/kstarbound/network/Connection.kt

@@ -39,7 +39,8 @@ abstract class Connection(val side: ConnectionSide, val type: ConnectionType) :
 		set(value) {
 			require(value in 1 .. ServerChannels.MAX_PLAYERS) { "Connection ID is out of range: $value" }
 			field = value
-			entityIDRange = value * -65536 .. 65535
+			val begin = value * -65536
+			entityIDRange = begin .. begin + 65535
 		}
 
 	var nickname: String = ""

src/main/kotlin/ru/dbotthepony/kstarbound/network/PacketRegistry.kt

@@ -20,6 +20,7 @@ import ru.dbotthepony.kstarbound.client.network.packets.JoinWorldPacket
 import ru.dbotthepony.kstarbound.client.network.packets.SpawnWorldObjectPacket
 import ru.dbotthepony.kstarbound.network.packets.ClientContextUpdatePacket
 import ru.dbotthepony.kstarbound.network.packets.EntityCreatePacket
+import ru.dbotthepony.kstarbound.network.packets.EntityDestroyPacket
 import ru.dbotthepony.kstarbound.network.packets.EntityUpdateSetPacket
 import ru.dbotthepony.kstarbound.network.packets.PingPacket
 import ru.dbotthepony.kstarbound.network.packets.PongPacket
@@ -440,7 +441,7 @@ class PacketRegistry(val isLegacy: Boolean) {
 			// Packets sent bidirectionally between world client and world server
 			LEGACY.add(::EntityCreatePacket)
 			LEGACY.add(EntityUpdateSetPacket::read)
-			LEGACY.skip("EntityDestroy")
+			LEGACY.add(::EntityDestroyPacket)
 			LEGACY.skip("EntityInteract")
 			LEGACY.skip("EntityInteractResult")
 			LEGACY.skip("HitRequest")

src/main/kotlin/ru/dbotthepony/kstarbound/network/packets/EntityCreatePacket.kt

@@ -35,6 +35,7 @@ class EntityCreatePacket(val entityType: EntityType, val storeData: ByteArrayLis
 	override fun play(connection: ServerConnection) {
 		if (entityID !in connection.entityIDRange) {
 			LOGGER.error("Player $connection tried to create entity $entityType with ID $entityID, but that's outside of allowed range ${connection.entityIDRange}!")
+			connection.disconnect("Creating entity with ID $entityID outside of allowed range ${connection.entityIDRange}")
 		} else {
 			val entity = when (entityType) {
 				EntityType.PLAYER -> {

src/main/kotlin/ru/dbotthepony/kstarbound/network/packets/EntityDestroyPacket.kt

@@ -0,0 +1,43 @@
+package ru.dbotthepony.kstarbound.network.packets
+
+import it.unimi.dsi.fastutil.bytes.ByteArrayList
+import org.apache.logging.log4j.LogManager
+import ru.dbotthepony.kommons.io.readByteArray
+import ru.dbotthepony.kommons.io.readSignedVarInt
+import ru.dbotthepony.kommons.io.writeByteArray
+import ru.dbotthepony.kommons.io.writeSignedVarInt
+import ru.dbotthepony.kstarbound.client.ClientConnection
+import ru.dbotthepony.kstarbound.network.IClientPacket
+import ru.dbotthepony.kstarbound.network.IServerPacket
+import ru.dbotthepony.kstarbound.server.ServerConnection
+import java.io.DataInputStream
+import java.io.DataOutputStream
+
+class EntityDestroyPacket(val entityID: Int, val finalNetState: ByteArrayList, val isDeath: Boolean) : IClientPacket, IServerPacket {
+	constructor(stream: DataInputStream, isLegacy: Boolean) : this(stream.readSignedVarInt(), ByteArrayList.wrap(stream.readByteArray()), stream.readBoolean())
+
+	override fun write(stream: DataOutputStream, isLegacy: Boolean) {
+		stream.writeSignedVarInt(entityID)
+		stream.writeByteArray(finalNetState.elements(), 0, finalNetState.size)
+		stream.writeBoolean(isDeath)
+	}
+
+	override fun play(connection: ServerConnection) {
+		if (entityID !in connection.entityIDRange) {
+			LOGGER.error("Client $connection tried to remove entity with ID $entityID, but that's outside of allowed range ${connection.entityIDRange}!")
+			connection.disconnect("Removing entity with ID $entityID outside of allowed range ${connection.entityIDRange}")
+		} else {
+			connection.enqueue {
+				entities[entityID]?.remove()
+			}
+		}
+	}
+
+	override fun play(connection: ClientConnection) {
+		TODO("Not yet implemented")
+	}
+
+	companion object {
+		private val LOGGER = LogManager.getLogger()
+	}
+}

src/main/kotlin/ru/dbotthepony/kstarbound/network/packets/EntityUpdateSetPacket.kt

@@ -34,6 +34,8 @@ class EntityUpdateSetPacket(val forConnection: Int, val deltas: Int2ObjectMap<By
 			for ((id, delta) in deltas) {
 				if (id !in connection.entityIDRange) {
 					LOGGER.error("Player $connection tried to update entity with ID $id, but that's outside of allowed range ${connection.entityIDRange}!")
+					connection.disconnect("Updating entity with ID $id outside of allowed range ${connection.entityIDRange}")
+					break
 				} else {
 					entities[id]?.networkGroup?.read(delta, Starbound.TIMESTEP, connection.isLegacy)
 				}

src/main/kotlin/ru/dbotthepony/kstarbound/server/ServerConnection.kt

@@ -179,8 +179,11 @@ class ServerConnection(val server: StarboundServer, type: ConnectionType) : Conn
 			flush()
 		}
 
+		world?.removePlayer(this)
+		world = null
 		tickets.values.forEach { it.ticket.cancel() }
 		tickets.clear()
+		tasks.clear()
 		pendingSend.clear()
 
 		if (::shipWorld.isInitialized) {

src/main/kotlin/ru/dbotthepony/kstarbound/server/world/ServerWorld.kt

@@ -103,7 +103,19 @@ class ServerWorld private constructor(
 	}
 
 	private fun doRemovePlayer(player: ServerConnection): Boolean {
-		return internalPlayers.remove(player)
+		if (internalPlayers.remove(player)) {
+			val itr = entities.int2ObjectEntrySet().iterator()
+
+			for ((id, entity) in itr) {
+				if (id in player.entityIDRange) {
+					entity.remove()
+				}
+			}
+
+			return true
+		}
+
+		return false
 	}
 
 	fun removePlayer(player: ServerConnection): CompletableFuture<Boolean> {

src/main/kotlin/ru/dbotthepony/kstarbound/util/ExecutionSpinner.kt

@@ -68,6 +68,7 @@ class ExecutionSpinner(private val waiter: Runnable, private val spinner: Boolea
 		carrier = Thread.currentThread()
 
 		while (isPaused) {
+			waiter.run()
 			LockSupport.park()
 		}
 
@@ -77,10 +78,14 @@ class ExecutionSpinner(private val waiter: Runnable, private val spinner: Boolea
 			waiter.run()
 			diff = timeUntilNextFrame()
 
-			if (diff >= SYSTEM_SCHEDULER_RESOLUTION * 2L)
+			if (PRECISE_WAIT) {
-				LockSupport.parkNanos(diff - SYSTEM_SCHEDULER_RESOLUTION)
+				if (diff >= SYSTEM_SCHEDULER_RESOLUTION * 2L)
-			else if (diff > SYSTEM_SCHEDULER_RESOLUTION)
+					LockSupport.parkNanos(diff - SYSTEM_SCHEDULER_RESOLUTION)
-				LockSupport.parkNanos(SYSTEM_SCHEDULER_RESOLUTION)
+				else if (diff > SYSTEM_SCHEDULER_RESOLUTION)
+					LockSupport.parkNanos(SYSTEM_SCHEDULER_RESOLUTION)
+			} else {
+				LockSupport.parkNanos(diff)
+			}
 		}
 
 		val mark = System.nanoTime()
@@ -98,6 +103,7 @@ class ExecutionSpinner(private val waiter: Runnable, private val spinner: Boolea
 	}
 
 	companion object {
+		private const val PRECISE_WAIT = false
 		private val LOGGER = LogManager.getLogger()
 		private var SYSTEM_SCHEDULER_RESOLUTION = 1_000_000L
 

src/main/kotlin/ru/dbotthepony/kstarbound/world/entities/AbstractEntity.kt

@@ -8,6 +8,7 @@ import ru.dbotthepony.kstarbound.client.StarboundClient
 import ru.dbotthepony.kstarbound.client.render.LayeredRenderer
 import ru.dbotthepony.kstarbound.defs.EntityType
 import ru.dbotthepony.kstarbound.defs.JsonDriven
+import ru.dbotthepony.kstarbound.network.packets.EntityDestroyPacket
 import ru.dbotthepony.kstarbound.network.syncher.MasterElement
 import ru.dbotthepony.kstarbound.network.syncher.NetworkedGroup
 import ru.dbotthepony.kstarbound.world.Chunk
@@ -131,6 +132,7 @@ abstract class AbstractEntity(path: String) : JsonDriven(path) {
 		check(world.entities.remove(entityID) == this) { "Tried to remove $this from $world, but removed something else!" }
 		world.orphanedEntities.remove(this)
 		onRemove(world)
+		world.broadcast(EntityDestroyPacket(entityID, ByteArrayList(), false))
 		innerWorld = null
 	}