From 2d775e3ef6c393ad1e0bbd06234fbb65631668ef Mon Sep 17 00:00:00 2001 From: Jan Rydzewski Date: Tue, 26 May 2026 01:00:35 +0000 Subject: [PATCH] network: free packet on unknown wire id in read_from_packet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit network_command_t::read_from_packet allocates an nwc by switch on p->get_id() and only reaches the receive(p) call (which transfers packet ownership to the nwc) when the switch produced one. An unknown id fell through default, the function returned NULL with p still live, and the caller — socket_info_t::receive_nwc — set its own packet member to NULL anyway, dropping the only pointer to the 8KB packet buffer. Each junk-id packet from a peer leaked. nettool's parallel implementation in src/nettool/nettool.cc has the identical leak in the same shape: switch falls through default with p still live, the caller (network_check_activity) doesn't free it. nettool only constructs nwc_service_t, so every other reply from the server hits default and leaks 8KB per packet — a malicious or buggy server can starve a long-running admin tool of memory. --- src/nettool/nettool.cc | 19 +++++++---- src/simutrans/network/network_cmd_ingame.cc | 37 ++++++++++++--------- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/nettool/nettool.cc b/src/nettool/nettool.cc index cdd60bcb3..8eb077a20 100644 --- a/src/nettool/nettool.cc +++ b/src/nettool/nettool.cc @@ -46,12 +46,19 @@ network_command_t* network_command_t::read_from_packet(packet_t *p) default: dbg->warning("network_command_t::read_from_socket", "received unknown packet id %d", p->get_id()); } - if (nwc) { - if (!nwc->receive(p) || p->has_failed()) { - dbg->warning("network_command_t::read_from_packet", "error while reading cmd from packet"); - delete nwc; - nwc = NULL; - } + if (nwc == NULL) { + // Unknown / unsupported wire id: free p before returning so a + // compromised server can't drive the admin tool out of memory + // by replying with junk-id packets (the caller in nettool's + // network_check_activity drops its packet pointer). Same + // shape as the network_cmd_ingame.cc version. + delete p; + return NULL; + } + if (!nwc->receive(p) || p->has_failed()) { + dbg->warning("network_command_t::read_from_packet", "error while reading cmd from packet"); + delete nwc; + nwc = NULL; } return nwc; } diff --git a/src/simutrans/network/network_cmd_ingame.cc b/src/simutrans/network/network_cmd_ingame.cc index fccb21e41..044a0da62 100644 --- a/src/simutrans/network/network_cmd_ingame.cc +++ b/src/simutrans/network/network_cmd_ingame.cc @@ -59,21 +59,28 @@ network_command_t* network_command_t::read_from_packet(packet_t *p) default: dbg->warning("network_command_t::read_from_socket", "received unknown packet id %d", p->get_id()); } - if (nwc) { - if (!nwc->receive(p) || p->has_failed()) { - dbg->warning("network_command_t::read_from_packet", "error while reading cmd from packet"); - delete nwc; - nwc = NULL; - } - else if (env_t::server) { - // The wire-supplied our_client_id is attacker-controlled. - // Identify the sender by its socket instead, so any later - // auth check (nwc_auth_player_t, nwc_chg_player_t, - // nwc_tool_t) reads the real slot and cannot be tricked - // into looking up someone else's player_unlocked bitmap - // or indexing past the socket list. - nwc->our_client_id = socket_list_t::get_client_id(p->get_sender()); - } + if (nwc == NULL) { + // Unknown / unsupported wire id: free p before returning so a + // malicious peer can't drive the server out of memory by + // flooding it with junk-id packets (callers null out their + // packet pointer unconditionally after this returns, so they + // won't free it either). + delete p; + return NULL; + } + if (!nwc->receive(p) || p->has_failed()) { + dbg->warning("network_command_t::read_from_packet", "error while reading cmd from packet"); + delete nwc; + nwc = NULL; + } + else if (env_t::server) { + // The wire-supplied our_client_id is attacker-controlled. + // Identify the sender by its socket instead, so any later + // auth check (nwc_auth_player_t, nwc_chg_player_t, + // nwc_tool_t) reads the real slot and cannot be tricked + // into looking up someone else's player_unlocked bitmap + // or indexing past the socket list. + nwc->our_client_id = socket_list_t::get_client_id(p->get_sender()); } return nwc; } -- 2.54.0