add safeties for 6xBB and 6xBC commands

src/Client.hh

@@ -35,7 +35,7 @@ public:
     // TODO: It'd be nice to use a pattern here (e.g. all server-side flags are
     // in the high bits) but that would require re-recording or manually
     // rewriting all the tests
-    CLIENT_SIDE_MASK                           = 0xF73CFFFF7C0BFFFB,
+    CLIENT_SIDE_MASK                           = 0xE73CFFFF7C0BFFFB,
 
     // Version-related flags
     CHECKED_FOR_DC_V1_PROTOTYPE                = 0x0000000000000002,
@@ -73,6 +73,7 @@ public:
     SHOULD_SEND_ENABLE_SAVE                    = 0x0004000000000000,
     SWITCH_ASSIST_ENABLED                      = 0x0000000100000000,
     IS_CLIENT_CUSTOMIZATION                    = 0x0100000000000000,
+    EP3_ALLOW_6xBC                             = 0x1000000000000000, // Server-side only
 
     // Cheat mode and option flags
     INFINITE_HP_ENABLED                        = 0x0000000200000000,
@@ -80,7 +81,7 @@ public:
     DEBUG_ENABLED                              = 0x0000000800000000,
     ITEM_DROP_NOTIFICATIONS_1                  = 0x0010000000000000,
     ITEM_DROP_NOTIFICATIONS_2                  = 0x0020000000000000,
-    FORCE_BATTLE_MODE_GAME                     = 0x0800000000000000,
+    FORCE_BATTLE_MODE_GAME                     = 0x0800000000000000, // Server-side only
 
     // Proxy option flags
     PROXY_SAVE_FILES                           = 0x0000001000000000,

src/CommandFormats.hh

@@ -5831,14 +5831,25 @@ struct G_IdentifyResult_BB_6xB9 {
 } __packed_ws__(G_IdentifyResult_BB_6xB9, 0x18);
 
 // 6xBA: Sync card trade state (Episode 3)
-// This command calls various member functions in TCardTradeServer.
+// This command calls various member functions in TCardTrade. This is used
+// after both players are standing at the respective kiosks and are ready to
+// trade cards.
 
 struct G_SyncCardTradeState_Ep3_6xBA {
   G_ClientIDHeader header;
-  le_uint16_t what = 0; // Low byte must be < 9; this indexes into a handler table
-  le_uint16_t unknown_a2 = 0;
-  le_uint32_t unknown_a3 = 0;
-  le_uint32_t unknown_a4 = 0;
+  // Values for what:
+  // 1 = add card to trade (card_id and count used)
+  // 2 = remove card from trade (card_id and count used)
+  // 3 = first confirmation
+  // 4 = cancel first confirmation
+  // 5 = second confirmation
+  // 6 = cancel second confirmation
+  // 7 = leave trade window
+  // Anything else = does nothing
+  le_uint16_t what = 0;
+  le_uint16_t unused = 0;
+  le_uint32_t card_id = 0; // Only used when what = 1 or 2
+  le_uint32_t count = 0; // Only used when what = 1 or 2
 } __packed_ws__(G_SyncCardTradeState_Ep3_6xBA, 0x10);
 
 // 6xBA: BB accept tekker result (handled by the server)
@@ -5848,28 +5859,35 @@ struct G_AcceptItemIdentification_BB_6xBA {
   le_uint32_t item_id = 0;
 } __packed_ws__(G_AcceptItemIdentification_BB_6xBA, 8);
 
-// 6xBB: Sync card trade state (Episode 3)
-// This command calls various member functions in TCardTradeServer.
-// TODO: Certain invalid values for slot/args in this command can crash the
-// client (what is properly bounds-checked). Find out the actual limits for
-// slot/args and make newserv enforce them.
+// 6xBB: Sync card trade server state (Episode 3)
+// This command calls various member functions in TCardTradeServer. This is
+// used before both players have entered the card trade sequence (as opposed to
+// 6xBA, which is used during that sequence).
 
-struct G_SyncCardTradeState_Ep3_6xBB {
+struct G_SyncCardTradeServerState_Ep3_6xBB {
   G_ClientIDHeader header;
-  le_uint16_t what = 0; // Must be < 5; this indexes into a jump table
-  le_uint16_t slot = 0;
+  // Values for what:
+  // 0 = request slot (leader sends accept message with what=1)
+  // 1 = accept slot (args[0] is the accepted client ID)
+  // 2 = cancel all slot requests
+  // 3 = replace all slots (args[0, 1] are the two client IDs to accept into
+  //     the two slots)
+  // 4 = relinquish all slots
+  // Anything else = does nothing
+  le_uint16_t what = 0;
+  le_uint16_t slot = 0; // Must be 0 or 1 (not bounds checked!)
   parray<le_uint32_t, 4> args;
-} __packed_ws__(G_SyncCardTradeState_Ep3_6xBB, 0x18);
+} __packed_ws__(G_SyncCardTradeServerState_Ep3_6xBB, 0x18);
 
 // 6xBB: BB bank request (handled by the server)
 
 // 6xBC: Card counts (Episode 3)
-// This is sent by the client in response to a 6xB5x38 command.
-// It's possible that this is an early, now-unused implementation of the CAx49
-// command. When the client receives this command, it copies the data into a
-// globally-allocated array, but nothing reads from this array. Curiously, this
-// command is smaller than 0x400 bytes, but uses the extended subcommand format
-// anyway (and uses the 6D command rather than 62).
+// This is sent by the client in response to a 6xB5x38 command. This is used
+// along with 6xB5x38 so clients can see each other's card counts. Curiously,
+// this command is smaller than 0x400 bytes (even on NTE) but uses the extended
+// subcommand format anyway.
+// An Episode 3 client will crash if it receives this command when the card
+// trade window is not active.
 
 struct G_CardCounts_Ep3NTE_6xBC {
   G_ExtendedHeaderT<G_UnusedHeader> header;
@@ -6855,8 +6873,8 @@ struct G_AdvanceFromStartingRollsPhase_Ep3_CAx37 {
 // 6xB5x38: Card counts request
 // This command causes the client identified by requested_client_id to send a
 // 6xBC command to the client identified by reply_to_client_id (privately, via
-// the 6D command). This appears to be unused; it is likely superseded by the
-// CAx49 command.
+// the 6D command). This is sent at the beginning of the card trade window
+// sequence.
 
 struct G_CardCountsRequest_Ep3_6xB5x38 {
   G_CardBattleCommandHeader header = {0xB5, sizeof(G_CardCountsRequest_Ep3_6xB5x38) / 4, 0, 0x38, 0, 0, 0};

src/ItemCreator.cc

@@ -212,7 +212,7 @@ ItemCreator::DropResult ItemCreator::on_box_item_drop_with_area_norm(uint8_t are
 }
 
 ItemCreator::DropResult ItemCreator::on_monster_item_drop_with_area_norm(uint32_t enemy_type, uint8_t area_norm) {
-  // Note: THe original GC implementation uses (enemy_type > 0x58) here; we
+  // Note: The original GC implementation uses (enemy_type > 0x58) here; we
   // extend it to the full array size for BB
   if (enemy_type >= 0x64) {
     this->log.warning("Invalid enemy type: %" PRIX32, enemy_type);

src/ProxyCommands.cc

@@ -1185,6 +1185,15 @@ static HandlerResult S_6x(shared_ptr<ProxyServer::LinkedSession> ses, uint16_t,
         }
       }
 
+    } else if ((static_cast<uint8_t>(data[0]) == 0xBB) && is_ep3(ses->version())) {
+      if (!validate_6xBB(check_size_t<G_SyncCardTradeServerState_Ep3_6xBB>(data))) {
+        return HandlerResult::Type::SUPPRESS;
+      }
+      return HandlerResult::Type::MODIFIED;
+
+    } else if ((static_cast<uint8_t>(data[0]) == 0xBC) && !ses->config.check_flag(Client::Flag::EP3_ALLOW_6xBC)) {
+      return HandlerResult::Type::SUPPRESS;
+
     } else if ((static_cast<uint8_t>(data[0]) == 0xBD) &&
         ses->config.check_flag(Client::Flag::PROXY_EP3_UNMASK_WHISPERS) &&
         is_ep3(ses->version())) {
@@ -1228,15 +1237,15 @@ static HandlerResult C_GXB_61(shared_ptr<ProxyServer::LinkedSession> ses, uint16
     C_CharacterData_V3_61_98* pd;
     if (flag == 4) { // Episode 3
       auto& ep3_pd = check_size_t<C_CharacterData_Ep3_61_98>(data);
-      if (ep3_pd.ep3_config.is_encrypted) {
-        decrypt_trivial_gci_data(
-            &ep3_pd.ep3_config.card_counts,
-            offsetof(Episode3::PlayerConfig, decks) - offsetof(Episode3::PlayerConfig, card_counts),
-            ep3_pd.ep3_config.basis);
-        ep3_pd.ep3_config.is_encrypted = 0;
-        ep3_pd.ep3_config.basis = 0;
-        modified = true;
-      }
+      // if (ep3_pd.ep3_config.is_encrypted) {
+      //   decrypt_trivial_gci_data(
+      //       &ep3_pd.ep3_config.card_counts,
+      //       offsetof(Episode3::PlayerConfig, decks) - offsetof(Episode3::PlayerConfig, card_counts),
+      //       ep3_pd.ep3_config.basis);
+      //   ep3_pd.ep3_config.is_encrypted = 0;
+      //   ep3_pd.ep3_config.basis = 0;
+      //   modified = true;
+      // }
       pd = reinterpret_cast<C_CharacterData_V3_61_98*>(&ep3_pd);
     } else {
       if (is_ep3(ses->version()) && (ses->version() != Version::GC_EP3_NTE)) {
@@ -2056,10 +2065,8 @@ HandlerResult C_6x<void>(shared_ptr<ProxyServer::LinkedSession> ses, uint16_t, u
 
     } else if (data[0] == 0x48) {
       if (ses->config.check_flag(Client::Flag::INFINITE_TP_ENABLED)) {
-        send_player_stats_change(ses->client_channel,
-            ses->lobby_client_id, PlayerStatsChange::ADD_TP, 255);
-        send_player_stats_change(ses->server_channel,
-            ses->lobby_client_id, PlayerStatsChange::ADD_TP, 255);
+        send_player_stats_change(ses->client_channel, ses->lobby_client_id, PlayerStatsChange::ADD_TP, 255);
+        send_player_stats_change(ses->server_channel, ses->lobby_client_id, PlayerStatsChange::ADD_TP, 255);
       }
 
     } else if (data[0] == 0x5F) {
@@ -2068,6 +2075,13 @@ HandlerResult C_6x<void>(shared_ptr<ProxyServer::LinkedSession> ses, uint16_t, u
 
     } else if (data[0] == 0x60 || static_cast<uint8_t>(data[0]) == 0xA2) {
       return SC_6x60_6xA2(ses, data);
+
+    } else if (is_ep3(ses->version()) && (data.size() > 4) && (static_cast<uint8_t>(data[0]) == 0xB5)) {
+      if (data[4] == 0x38) {
+        ses->config.set_flag(Client::Flag::EP3_ALLOW_6xBC);
+      } else if (data[4] == 0x3C) {
+        ses->config.clear_flag(Client::Flag::EP3_ALLOW_6xBC);
+      }
     }
   }
 

src/ReceiveSubcommands.cc

@@ -1164,14 +1164,6 @@ static void on_forward_check_ep3_lobby(shared_ptr<Client> c, uint8_t command, ui
   }
 }
 
-static void on_forward_check_ep3_game(shared_ptr<Client> c, uint8_t command, uint8_t flag, void* data, size_t size) {
-  check_size_t<G_UnusedHeader>(data, size, 0xFFFF);
-  auto l = c->require_lobby();
-  if (l->is_game() && l->is_ep3()) {
-    forward_subcommand(c, command, flag, data, size);
-  }
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // Ep3 subcommands
 
@@ -1199,6 +1191,10 @@ static void on_ep3_battle_subs(shared_ptr<Client> c, uint8_t command, uint8_t fl
     if (l->is_game() && (cmd.client_id >= 4)) {
       return;
     }
+  } else if (header.subsubcommand == 0x38) {
+    c->config.set_flag(Client::Flag::EP3_ALLOW_6xBC);
+  } else if (header.subsubcommand == 0x3C) {
+    c->config.clear_flag(Client::Flag::EP3_ALLOW_6xBC);
   }
 
   if (!(s->ep3_behavior_flags & Episode3::BehaviorFlag::DISABLE_MASKING) && (c->version() != Version::GC_EP3_NTE)) {
@@ -1208,6 +1204,28 @@ static void on_ep3_battle_subs(shared_ptr<Client> c, uint8_t command, uint8_t fl
   forward_subcommand(c, command, flag, data.data(), data.size());
 }
 
+static void on_ep3_trade_card_counts(shared_ptr<Client> c, uint8_t command, uint8_t flag, void* data, size_t size) {
+  if (c->version() == Version::GC_EP3_NTE) {
+    check_size_t<G_CardCounts_Ep3NTE_6xBC>(data, size, 0xFFFF);
+  } else {
+    check_size_t<G_CardCounts_Ep3_6xBC>(data, size, 0xFFFF);
+  }
+
+  if (!command_is_private(command)) {
+    return;
+  }
+  auto l = c->require_lobby();
+  if (!l->is_game() || !l->is_ep3()) {
+    return;
+  }
+  auto target = l->clients.at(flag);
+  if (!target || !target->config.check_flag(Client::Flag::EP3_ALLOW_6xBC)) {
+    return;
+  }
+
+  forward_subcommand(c, command, flag, data, size);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Chat commands and the like
 
@@ -2335,12 +2353,46 @@ static void on_open_shop_bb_or_ep3_battle_subs(shared_ptr<Client> c, uint8_t com
   }
 }
 
+bool validate_6xBB(G_SyncCardTradeServerState_Ep3_6xBB& cmd) {
+  if ((cmd.header.client_id >= 4) || (cmd.slot > 1)) {
+    return false;
+  }
+
+  // TTradeCardServer uses 4 to indicate the slot is empty, so we allow 4 in
+  // the client ID checks below
+  switch (cmd.what) {
+    case 1:
+      if (cmd.args[0] >= 5) {
+        return false;
+      }
+      cmd.args[1] = 0;
+      cmd.args[2] = 0;
+      cmd.args[3] = 0;
+      break;
+    case 0:
+    case 2:
+    case 4:
+      cmd.args.clear(0);
+      break;
+    case 3:
+      if (cmd.args[0] >= 5 || cmd.args[1] >= 5) {
+        return false;
+      }
+      cmd.args[2] = 0;
+      cmd.args[3] = 0;
+      break;
+    default:
+      return false;
+  }
+  return true;
+}
+
 static void on_open_bank_bb_or_card_trade_counter_ep3(shared_ptr<Client> c, uint8_t command, uint8_t flag, void* data, size_t size) {
   auto l = c->require_lobby();
   if ((l->base_version == Version::BB_V4) && l->is_game()) {
     c->config.set_flag(Client::Flag::AT_BANK_COUNTER);
     send_bank(c);
-  } else if (l->is_ep3()) {
+  } else if (l->is_ep3() && validate_6xBB(check_size_t<G_SyncCardTradeServerState_Ep3_6xBB>(data, size))) {
     forward_subcommand(c, command, flag, data, size);
   }
 }
@@ -4827,7 +4879,7 @@ const SubcommandDefinition subcommand_definitions[0x100] = {
     /* 6xB9 */ {NONE, NONE, 0xB9, on_invalid},
     /* 6xBA */ {NONE, NONE, 0xBA, on_accept_identify_item_bb},
     /* 6xBB */ {NONE, NONE, 0xBB, on_open_bank_bb_or_card_trade_counter_ep3},
-    /* 6xBC */ {NONE, NONE, 0xBC, on_forward_check_ep3_game},
+    /* 6xBC */ {NONE, NONE, 0xBC, on_ep3_trade_card_counts},
     /* 6xBD */ {NONE, NONE, 0xBD, on_ep3_private_word_select_bb_bank_action, SDF::ALWAYS_FORWARD_TO_WATCHERS},
     /* 6xBE */ {NONE, NONE, 0xBE, forward_subcommand_m, SDF::ALWAYS_FORWARD_TO_WATCHERS | SDF::ALLOW_FORWARD_TO_WATCHED_LOBBY},
     /* 6xBF */ {NONE, NONE, 0xBF, on_forward_check_ep3_lobby},

src/ReceiveSubcommands.hh

@@ -122,3 +122,5 @@ protected:
       bool from_client_customization);
   G_SyncPlayerDispAndInventory_BaseV1 base_v1() const;
 };
+
+bool validate_6xBB(G_SyncCardTradeServerState_Ep3_6xBB& cmd);