From dfa0560aff2491f2b0afaa5e810aed2a5081a325 Mon Sep 17 00:00:00 2001 From: Jan Engelhardt Date: Mon, 9 Sep 2024 09:49:31 +0200 Subject: [PATCH 1/4] dedicated: resolve out-of-bounds access during config parse ASAN reports: $GIT/Descent3/dedicated_server.cpp:350:24: runtime error: index 1024 out of bounds for type 'cvar_entry [36]' $GIT/Descent3/dedicated_server.cpp:350:14: runtime error: load of address 0x000001e677c0 with insufficient space for an object of type 'const char *' This can happen if a line in the .cfg starts with a '#'. --- Descent3/dedicated_server.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Descent3/dedicated_server.cpp b/Descent3/dedicated_server.cpp index 9b133ab6..13ccd0f1 100644 --- a/Descent3/dedicated_server.cpp +++ b/Descent3/dedicated_server.cpp @@ -346,7 +346,8 @@ int LoadServerConfigFile() { while (inf.ReadLine()) { int cmd; - while ((cmd = inf.ParseLine(operand, INFFILE_LINELEN)) > INFFILE_ERROR) { + while ((cmd = inf.ParseLine(operand, INFFILE_LINELEN)) > INFFILE_ERROR && + cmd != INFFILE_SYMBOL) { SetCVar(CVars[cmd].varname, operand, true); } } From 69dbf5bca74d5d69a3d04f8348bdd8a65a13d41d Mon Sep 17 00:00:00 2001 From: Jan Engelhardt Date: Mon, 9 Sep 2024 09:54:20 +0200 Subject: [PATCH 2/4] Resolve alloc-dealloc-mismatch in CFile::FreeSymbols ==89545==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x5020001007f0 f0 operator delete(void*, unsigned long) (/lib64/libasan.so.8+0xfe1f8) f1 InfFile::FreeSymbols() $GIT/cfile/inffile.cpp:63 f2 InfFile::Close() $GIT/cfile/inffile.cpp:115 f3 LoadServerConfigFile() $GIT/Descent3/dedicated_server.cpp:355 0x5020001007f0 is located 0 bytes inside of 11-byte region [0x5020001007f0,0x5020001007fb) allocated by thread T0 here: f0 operator new[](unsigned long) (/lib64/libasan.so.8+0xfd458) f1 InfFile::AddSymbol(char const*, char const*) $GIT/cfile/inffile.cpp:49 f2 InfFile::ParseLine(char*, int) $GIT/cfile/inffile.cpp:187 --- cfile/inffile.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfile/inffile.cpp b/cfile/inffile.cpp index 820bc2f1..73c5a355 100644 --- a/cfile/inffile.cpp +++ b/cfile/inffile.cpp @@ -60,7 +60,7 @@ void InfFile::FreeSymbols() { while ((sym = m_sym_list.start()) != 0) { sym = m_sym_list.unlink(); if (sym->t.text) - delete sym->t.text; + delete[] sym->t.text; delete sym; } } From 009645ac83f54738a9202ba1f8a9c8a86dfa3efe Mon Sep 17 00:00:00 2001 From: Jan Engelhardt Date: Mon, 9 Sep 2024 10:17:23 +0200 Subject: [PATCH 3/4] Switch netgame_info::server_config_name to dynamically allocated Resolve an out-of-bounds write in LoadServerConfigFile. A sufficiently long path, descent3 -dedicated /home/jengelh/.config/descent3/dedicated.conf causes the game server to exit with Error loading connection DLL 'cated.conf' Which hints at a buffer overflow. --- Descent3/dedicated_server.cpp | 4 ++-- Descent3/multi_external.h | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Descent3/dedicated_server.cpp b/Descent3/dedicated_server.cpp index 13ccd0f1..57a9b143 100644 --- a/Descent3/dedicated_server.cpp +++ b/Descent3/dedicated_server.cpp @@ -328,7 +328,7 @@ int LoadServerConfigFile() { } if (GameArgs[t + 1][0]) { - strcpy(Netgame.server_config_name, GameArgs[t + 1]); + Netgame.server_config_name = GameArgs[t + 1]; } else { PrintDedicatedMessage(TXT_DS_MISSINGCONFIG); PrintDedicatedMessage("\n"); @@ -337,7 +337,7 @@ int LoadServerConfigFile() { // open file if (!inf.Open(Netgame.server_config_name, "[server config file]", DedicatedServerLex)) { - PrintDedicatedMessage(TXT_DS_BADCONFIG, Netgame.server_config_name); + PrintDedicatedMessage(TXT_DS_BADCONFIG, Netgame.server_config_name.u8string().c_str()); PrintDedicatedMessage("\n"); return 0; } diff --git a/Descent3/multi_external.h b/Descent3/multi_external.h index 9d375185..83bfe419 100644 --- a/Descent3/multi_external.h +++ b/Descent3/multi_external.h @@ -121,6 +121,7 @@ typedef int HANDLE; #include "descent.h" //for MSN_NAMELEN #include "byteswap.h" #include +#include #define NETGAME_NAME_LEN 32 #define NETGAME_SCRIPT_LEN 32 @@ -249,7 +250,7 @@ struct netgame_info { char mission[MSN_NAMELEN]; char mission_name[MISSION_NAME_LEN]; char scriptname[NETGAME_SCRIPT_LEN]; - char server_config_name[PAGENAME_LEN]; + std::filesystem::path server_config_name; char connection_name[PAGENAME_LEN]; network_address server_address; // The address of the server that we're talking to - not used if we are the server From b9fbee0e25b52615c4d5637fd31824ae49b455f0 Mon Sep 17 00:00:00 2001 From: Jan Engelhardt Date: Mon, 9 Sep 2024 10:30:45 +0200 Subject: [PATCH 4/4] Resolve out-of-bounds accesses in DLLMultiInit vp[26] is `int *`, so it tries to read 4 bytes on amd64, even though TCP_Active, which is behind vp[26] is just a bool and 1 byte. ==95927==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000004734f40 at pc 0x7f4f8d93b952 bp 0x7ffc57f191b0 sp 0x7ffc57f191a8 READ of size 4 at 0x000004734f40 thread T0 f0 DLLMultiInit $GIT/netcon/includes/mdllinit.h:314 f1 LoadMultiDLL(char const*) $GIT/Descent3/multi_dll_mgr.cpp:690 f2 RunServerConfigs $GIT/Descent3/dedicated_server.cpp:236 f3 LoadServerConfigFile() $GIT/Descent3/dedicated_server.cpp:357 f4 InitDedicatedServer $GIT/Descent3/init.cpp:1778 f5 InitD3Systems2(bool) $GIT/Descent3/init.cpp:1952 f6 Descent3() $GIT/Descent3/descent.cpp:504 f7 oeD3LnxApp::run() $GIT/Descent3/sdlmain.cpp:151 0x000004734f41 is located 0 bytes after global variable 'TCP_active' defined in '$GIT/networking/networking.cpp:383:6' (0x4734f40) of size 1 SUMMARY: AddressSanitizer: global-buffer-overflow $GIT/netcon/includes/mdllinit.h:314 in DLLMultiInit --- netcon/includes/mdllinit.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netcon/includes/mdllinit.h b/netcon/includes/mdllinit.h index 017ce67e..2c3895a5 100644 --- a/netcon/includes/mdllinit.h +++ b/netcon/includes/mdllinit.h @@ -311,8 +311,8 @@ DLLNum_modems_found = (int *)API.vp[24]; DLLUse_DirectPlay = (bool *)API.vp[22]; #endif DLLDedicated_server = (bool *)API.vp[25]; -DLLTCP_active = (BOOL)*API.vp[26]; -DLLIPX_active = (BOOL)*API.vp[27]; +DLLTCP_active = *(bool *)API.vp[26]; +DLLIPX_active = *(bool *)API.vp[27]; DLLnw_ListenPort = (uint16_t)((size_t)API.vp[28] & 0xffff); DLLMulti_Gamelist_changed = (bool *)API.vp[29]; DLLPXO_hosted_lobby_name = (char *)API.vp[30];