fix several critical security vulnerabilities

* discard unexpected network data safely

* validate hotbar slot from client

* validate entity id bounds when accessing mob data

* prevent manipulation of memory pointer through crafting grid

Credit to @vmpr0be and @loot01 for finding and reporting these issues.

Author: p2r3

include/globals.h

@@ -233,6 +233,7 @@ typedef struct {
   // 0x10 - eating, makes flagval_16 act as eating timer
   // 0x20 - client loading, uses flagval_16 as fallback timer
   // 0x40 - movement update cooldown
+  // 0x80 - craft_items lock (for storing pointers)
   uint8_t flags;
 } PlayerData;
 

include/tools.h

@@ -15,6 +15,7 @@ static inline int div_floor (int a, int b) {
 extern uint64_t total_bytes_received;
 ssize_t recv_all (int client_fd, void *buf, size_t n, uint8_t require_first);
 ssize_t send_all (int client_fd, const void *buf, ssize_t len);
+void discard_all (int client_fd, size_t remaining, uint8_t require_first);
 
 ssize_t writeByte (int client_fd, uint8_t byte);
 ssize_t writeUint16 (int client_fd, uint16_t num);

src/crafting.c

@@ -8,6 +8,13 @@
 
 void getCraftingOutput (PlayerData *player, uint8_t *count, uint16_t *item) {
 
+  // Exit early if craft_items has been locked
+  if (player->flags & 0x80) {
+    *count = 0;
+    *item = 0;
+    return;
+  }
+
   uint8_t i, filled = 0, first = 10, identical = true;
   for (i = 0; i < 9; i ++) {
     if (player->craft_items[i]) {

src/main.c

@@ -192,7 +192,7 @@ void handlePacket (int client_fd, int length, int packet_id, int state) {
     case 0x1B:
       if (state == STATE_PLAY) {
         // Serverbound keep-alive (ignored)
-        recv_all(client_fd, recv_buffer, length, false);
+        discard_all(client_fd, length, false);
       }
       break;
 
@@ -466,7 +466,7 @@ void handlePacket (int client_fd, int length, int packet_id, int state) {
         if (packet_id < 16) printf("0");
         printf("%X, length: %d, state: %d\n\n", packet_id, length, state);
       #endif
-      recv_all(client_fd, recv_buffer, length, false);
+      discard_all(client_fd, length, false);
       break;
 
   }
@@ -476,7 +476,7 @@ void handlePacket (int client_fd, int length, int packet_id, int state) {
   if (processed_length == length) return;
 
   if (length > processed_length) {
-    recv_all(client_fd, recv_buffer, length - processed_length, false);
+    discard_all(client_fd, length - processed_length, false);
   }
 
   #ifdef DEV_LOG_LENGTH_DISCREPANCY

src/packets.c

@@ -638,6 +638,8 @@ int cs_clickContainer (int client_fd) {
     } else
     #endif
     {
+      // Prevent accessing crafting-related slots when craft_items is locked
+      if (slot > 40 && player->flags & 0x80) return 1;
       p_item = &player->inventory_items[slot];
       p_count = &player->inventory_count[slot];
     }
@@ -811,7 +813,10 @@ int cs_setHeldItem (int client_fd) {
   PlayerData *player;
   if (getPlayerData(client_fd, &player)) return 1;
 
-  player->hotbar = (uint8_t)readUint16(client_fd);
+  uint8_t slot = readUint16(client_fd);
+  if (slot >= 9) return 1;
+
+  player->hotbar = slot;
 
   return 0;
 }
@@ -845,6 +850,8 @@ int cs_closeContainer (int client_fd) {
     }
     player->craft_items[i] = 0;
     player->craft_count[i] = 0;
+    // Unlock craft_items
+    player->flags &= ~0x80;
   }
 
   givePlayerItem(player, player->flagval_16, player->flagval_8);

src/procedures.c

@@ -68,6 +68,7 @@ void resetPlayerData (PlayerData *player) {
     player->craft_items[i] = 0;
     player->craft_count[i] = 0;
   }
+  player->flags &= ~0x80;
 }
 
 // Assigns the given data to a player_data entry
@@ -342,6 +343,13 @@ void spawnPlayer (PlayerData *player) {
 
   task_yield(); // Check task timer between packets
 
+  // Clear crafting grid residue, unlock craft_items
+  for (int i = 0; i < 9; i++) {
+    player->craft_items[i] = 0;
+    player->craft_count[i] = 0;
+  }
+  player->flags &= ~0x80;
+
   // Sync client inventory and hotbar
   for (uint8_t i = 0; i < 41; i ++) {
     sc_setContainerSlot(player->client_fd, 0, serverSlotToClientSlot(0, i), player->inventory_count[i], player->inventory_items[i]);
@@ -425,7 +433,9 @@ void broadcastPlayerMetadata (PlayerData *player) {
 // If client_fd is -1, broadcasts to all player
 void broadcastMobMetadata (int client_fd, int entity_id) {
 
-  MobData *mob = &mob_data[-entity_id - 2];
+  int mob_index = -entity_id - 2;
+  if (mob_index < 0 || mob_index >= MAX_MOBS) return;
+  MobData *mob = &mob_data[mob_index];
 
   EntityData *metadata;
   size_t length;
@@ -1266,6 +1276,8 @@ void handlePlayerUseItem (PlayerData *player, short x, short y, short z, uint8_t
       // is mutually exclusive with chests, though it is otherwise a
       // terrible idea for obvious reasons.
       memcpy(player->craft_items, &storage_ptr, sizeof(storage_ptr));
+      // Flag craft_items as locked due to holding a pointer
+      player->flags |= 0x80;
       // Show the player the chest UI
       sc_openScreen(player->client_fd, 2, "Chest", 5);
       // Load the slots of the chest from the block_changes array.
@@ -1420,7 +1432,9 @@ void interactEntity (int entity_id, int interactor_id) {
   PlayerData *player;
   if (getPlayerData(interactor_id, &player)) return;
 
-  MobData *mob = &mob_data[-entity_id - 2];
+  int mob_index = -entity_id - 2;
+  if (mob_index < 0 || mob_index >= MAX_MOBS) return;
+  MobData *mob = &mob_data[mob_index];
 
   switch (mob->type) {
     case 106: // Sheep
@@ -1551,7 +1565,10 @@ void hurtEntity (int entity_id, int attacker_id, uint8_t damage_type, uint8_t da
 
   } else { // The attacked entity is a mob
 
-    MobData *mob = &mob_data[-entity_id - 2];
+    int mob_index = -entity_id - 2;
+    if (mob_index < 0 || mob_index >= MAX_MOBS) return;
+    MobData *mob = &mob_data[mob_index];
+
     uint8_t mob_health = mob->data & 31;
 
     // Don't continue if the mob is already dead

src/tools.c

@@ -135,6 +135,17 @@ ssize_t send_all (int client_fd, const void *buf, ssize_t len) {
   return sent;
 }
 
+void discard_all (int client_fd, size_t remaining, uint8_t require_first) {
+  while (remaining > 0) {
+    size_t recv_n = remaining > MAX_RECV_BUF_LEN ? MAX_RECV_BUF_LEN : remaining;
+    ssize_t received = recv_all(client_fd, recv_buffer, recv_n, require_first);
+    if (received < 0) return;
+    if (received > remaining) return;
+    remaining -= received;
+    require_first = false;
+  }
+}
+
 ssize_t writeByte (int client_fd, uint8_t byte) {
   return send_all(client_fd, &byte, 1);
 }