From 8ee64498f14e596bf2858ee08202ab00824e3a12 Mon Sep 17 00:00:00 2001 From: Dino <8dino2@gmail.com> Date: Sat, 23 May 2026 19:06:41 -0400 Subject: [PATCH 1/4] fix(security): sanitize team names to prevent stuffcmd injection (#316 FIX-09) Team names (set by captains via teamname) were embedded unescaped into the quoted record command stuffed to clients by autorecord. A team name containing a doublequote, newline, or semicolon would break out of the quoted argument and inject arbitrary console commands on every player's client. Sanitize at intake (Cmd_Teamname_f) and at output (Cmd_AutoRecord_f) as defense-in-depth. Refs #316 --- src/action/a_cmds.c | 11 +++++++++++ src/action/a_match.c | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/action/a_cmds.c b/src/action/a_cmds.c index f66655a54..06fdff4c7 100644 --- a/src/action/a_cmds.c +++ b/src/action/a_cmds.c @@ -1232,6 +1232,7 @@ void RemoveSpaces(char *s) void Cmd_AutoRecord_f(edict_t * ent) { char rec_date[20], recstr[MAX_QPATH]; + char *p; time_t clock; time( &clock ); @@ -1249,6 +1250,16 @@ void Cmd_AutoRecord_f(edict_t * ent) Q_snprintf(recstr, sizeof(recstr), "%s-%s", rec_date, level.mapname); } + /* Belt-and-suspenders: even though teamname intake sanitizes, scrub anything + * that could break out of the quoted stuffcmd arg (recstr also includes + * level.mapname which is engine-controlled but cheap to harden). */ + for (p = recstr; *p; p++) { + if (*p == '"' || *p == '\\' || *p == '\n' || *p == '\r' || + *p == ';' || *p == '$' || (unsigned char)*p < 0x20) { + *p = '_'; + } + } + stuffcmd(ent, va("record \"%s\"\n", recstr)); } diff --git a/src/action/a_match.c b/src/action/a_match.c index 1ba9adb27..910821f37 100644 --- a/src/action/a_match.c +++ b/src/action/a_match.c @@ -495,6 +495,22 @@ qboolean CheckAbandon(void) return false; } +/* + * Replace shell/stuffcmd-dangerous characters with '_' in-place. + * Why: team names are echoed into stuffcmd'd console commands (autorecord, + * etc.). An unescaped '"', ';', '\n', or '$' lets a captain inject commands + * into every other player's console. + */ +static void sanitize_command_arg(char *s) +{ + for (; *s; s++) { + if (*s == '"' || *s == '\\' || *s == '\n' || *s == '\r' || + *s == ';' || *s == '$' || (unsigned char)*s < 0x20) { + *s = '_'; + } + } +} + void Cmd_Teamname_f(edict_t * ent) { int i, argc, teamNum; @@ -555,6 +571,11 @@ void Cmd_Teamname_f(edict_t * ent) temp[18] = 0; } + if (!temp[0]) + strcpy( temp, "noname" ); + + sanitize_command_arg(temp); + if (!temp[0]) strcpy( temp, "noname" ); From 67a9c666a94902a9404ded2fd4387e99efd1b2e3 Mon Sep 17 00:00:00 2001 From: Dino <8dino2@gmail.com> Date: Sat, 23 May 2026 19:07:40 -0400 Subject: [PATCH 2/4] fix(security): validate lrcon map/softmap arguments (#316 FIX-03) mapname was passed unvalidated to Q_strncpyz(level.nextmap, ...) and ultimately into the engine's map-change command path, where ';' is a command separator. A claimer could chain commands via `lrcon map "foo;quit"` or attempt path traversal via `lrcon map "../../etc/passwd"`. Add lrcon_valid_mapname() helper that restricts to Q_ispath() characters (alphanumeric + _ + -) and length < MAX_QPATH. Call from Lrcon_Map and Lrcon_Softmap before any further processing. Refs #316 --- src/action/g_lrcon.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/action/g_lrcon.c b/src/action/g_lrcon.c index dde1067da..13a899e15 100644 --- a/src/action/g_lrcon.c +++ b/src/action/g_lrcon.c @@ -21,6 +21,34 @@ extern cvar_t *lrcon_claimer_name; extern cvar_t *lrcon_claimer_ip; extern int dosoft; +/* + * lrcon_valid_mapname + * + * Returns true if mapname is safe to pass to the engine's map-change path. + * Restricts to alnum/underscore/hyphen — rejects path traversal (..), + * separators (/ \), command separators (; &), and quote/escape chars. + * Why: lrcon map / softmap embeds the name into a downstream AddCommandString + * path where ';' is a command separator; without validation a claimer can + * chain arbitrary server commands. + */ +static qboolean lrcon_valid_mapname(const char *s) +{ + size_t len; + + if (!s || !*s) + return false; + + len = strlen(s); + if (len >= MAX_QPATH) + return false; + + for (; *s; s++) { + if (!Q_ispath(*s)) + return false; + } + return true; +} + /* * Lrcon_CheckClaimer * @@ -237,6 +265,12 @@ void Lrcon_Map(edict_t *ent) mapname = gi.argv(2); + if (!lrcon_valid_mapname(mapname)) { + gi.cprintf(ent, PRINT_HIGH, + "Invalid mapname. Use alphanumeric, underscore, hyphen only.\n"); + return; + } + gi.bprintf(PRINT_HIGH, "%s is changing map to %s\n", ent->client->pers.netname, mapname); @@ -263,6 +297,12 @@ void Lrcon_Softmap(edict_t *ent) mapname = gi.argv(2); + if (!lrcon_valid_mapname(mapname)) { + gi.cprintf(ent, PRINT_HIGH, + "Invalid mapname. Use alphanumeric, underscore, hyphen only.\n"); + return; + } + gi.bprintf(PRINT_HIGH, "%s is soft-changing map to %s\n", ent->client->pers.netname, mapname); From 6773b1b9d42cafbe90c0096be2e11c67e7ce789d Mon Sep 17 00:00:00 2001 From: Dino <8dino2@gmail.com> Date: Sat, 23 May 2026 19:08:42 -0400 Subject: [PATCH 3/4] fix(security): restrict lrcon mode commands to exec (#316 FIX-02) The mode command field in lrcon.cfg was passed verbatim to AddCommandString, allowing operator-writable config to embed arbitrary server commands via ';'. An operator (or a config compiled from less-trusted input) could write a mode like: ctf|exec cfg/ctf.cfg; rcon_password "" Restrict mode commands at config-parse time to the strict form: exec .cfg Where must be non-empty, end in .cfg, contain no '..', no leading '/' or '\\', and only Q_ispath() chars plus '/' and '.'. Reject malformed entries with a dprintf warning at startup so operators see why their mode was dropped. Refs #316 --- src/action/a_game.c | 48 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/src/action/a_game.c b/src/action/a_game.c index 64aeff1db..5431999c3 100644 --- a/src/action/a_game.c +++ b/src/action/a_game.c @@ -1739,14 +1739,58 @@ void ReadLrconConfig(void) game.lrcon_config.allowed_cvars_count++; } } else if (!strcmp(reading_section, "modes")) { - // Format: name|command + // Format: name|exec + // Why: mode command is passed verbatim to AddCommandString. + // Without restriction, an operator (or compromised config) can + // embed arbitrary commands via ';'. Restrict to strict + // "exec .cfg" form. char *pipe = strchr(buf, '|'); if (pipe != NULL && game.lrcon_config.modes_count < MAX_LRCON_MODES) { + const char *cmd, *fname; + size_t flen; + qboolean valid = true; + *pipe = 0; + cmd = pipe + 1; + + // Must begin with "exec " + if (Q_strncasecmp(cmd, "exec ", 5) != 0) { + gi.dprintf("LRCON: rejecting mode '%s' — command must start with 'exec '\n", buf); + valid = false; + } + + if (valid) { + fname = cmd + 5; + while (*fname == ' ') fname++; + flen = strlen(fname); + + // Filename rules: non-empty, ends in .cfg, no traversal, + // only Q_ispath() chars plus '/' and '.' + if (flen < 5 || strcmp(fname + flen - 4, ".cfg") != 0) { + gi.dprintf("LRCON: rejecting mode '%s' — filename must end in .cfg\n", buf); + valid = false; + } else if (strstr(fname, "..") || fname[0] == '/' || fname[0] == '\\') { + gi.dprintf("LRCON: rejecting mode '%s' — filename has traversal or absolute path\n", buf); + valid = false; + } else { + const char *p; + for (p = fname; *p; p++) { + if (!(Q_ispath(*p) || *p == '/' || *p == '.')) { + gi.dprintf("LRCON: rejecting mode '%s' — filename has disallowed char\n", buf); + valid = false; + break; + } + } + } + } + + if (!valid) + continue; + Q_strncpyz(game.lrcon_config.modes[game.lrcon_config.modes_count].name, buf, sizeof(game.lrcon_config.modes[0].name)); Q_strncpyz(game.lrcon_config.modes[game.lrcon_config.modes_count].command, - pipe + 1, sizeof(game.lrcon_config.modes[0].command)); + cmd, sizeof(game.lrcon_config.modes[0].command)); gi.dprintf("LRCON: mode %d = %s -> %s\n", game.lrcon_config.modes_count, game.lrcon_config.modes[game.lrcon_config.modes_count].name, From 502550cc8f404a91abfc3b7226ebc74db51c55e9 Mon Sep 17 00:00:00 2001 From: Dino <8dino2@gmail.com> Date: Sat, 23 May 2026 19:10:59 -0400 Subject: [PATCH 4/4] fix(security): allowlist for lrcon stuffcmd (#316 FIX-01, FIX-19) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lrcon stuffcmd handler passed gi.args() verbatim to all targeted clients with no sanitization. A claimer could `lrcon stuffcmd all disconnect`, `lrcon stuffcmd all quit`, `lrcon stuffcmd all bind F1 "kill;quit"`, or chain multiple commands via ';' — effectively RCE on every connected client. Add operator-configured allowlist via new [allowed_stuffcmds] section in lrcon.cfg, parsed as a comma-delimited list of command names. Enforce in Lrcon_Stuffcmd: extract first token, reject if not on allowlist. Also reject command payloads containing ';', newline, or '$' to block chaining and cvar substitution bypass. If the allowlist is empty or the section is missing, all stuffcmds are denied — secure by default. Also (FIX-19): update lrcon.cfg.example with explicit warnings against whitelisting password / rcon_password / sv_load_ent / sys_* cvars, remove the dangerous `password` entry from the example, and document the new [allowed_stuffcmds] section. Adds: - MAX_LRCON_STUFFCMDS, allowed_stuffcmds[], allowed_stuffcmds_count fields - Parser branch in ReadLrconConfig Refs #316 --- action/lrcon.cfg.example | 70 ++++++++++++++++++++++++++++++++++++++++ src/action/a_game.c | 20 ++++++++++++ src/action/g_local.h | 3 ++ src/action/g_lrcon.c | 54 ++++++++++++++++++++++++++++--- 4 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 action/lrcon.cfg.example diff --git a/action/lrcon.cfg.example b/action/lrcon.cfg.example new file mode 100644 index 000000000..0adf2dce5 --- /dev/null +++ b/action/lrcon.cfg.example @@ -0,0 +1,70 @@ +// LRCON Configuration Example +// Copy to lrcon.cfg and customize for your server +// +// LRCON provides limited remote console access allowing players to +// claim temporary admin rights and execute restricted server commands + +[settings] +// Enable or disable LRCON on this server +enabled 1 + +// Quit the server when the last player leaves +// Set to 1 to enable, 0 to disable (default) +quit_on_empty 0 + +[allowed_cvars] +// List of cvars that can be queried and modified via lrcon +// One cvar per line - only whitelisted cvars can be changed +// +// WARNING: Do NOT whitelist sensitive cvars here. In particular, NEVER add: +// rcon_password - full server takeover +// password - lock all other players out +// sv_load_ent - bypass entity sandboxing +// sys_forcegamelib - load arbitrary game DLLs +// sys_* - system-level cvars +// Whitelist only gameplay-tuning cvars. +// +// Examples: +timelimit +fraglimit +teamdm +ctf +maxclients +hostname +dmflags +roundlimit +matchmode +teamplay +g_select_empty +sv_gravity +sv_fps +sv_antilag + +[allowed_stuffcmds] +// Comma-delimited list of commands that may be sent to clients via +// `lrcon stuffcmd `. +// +// SECURITY: If this section is empty or missing, all stuffcmds are DENIED. +// Without an allowlist, a claimer can stuffcmd `disconnect`, `quit`, +// arbitrary `bind`s, or chain commands — effectively RCE on every client. +// +// Recommended baseline allowlist (uncomment to enable): +// disconnect, reconnect, say, say_team, record, stoprecord + +[modes] +// Server configuration modes - allows players to switch configs quickly +// Format: mode_name|exec .cfg +// +// SECURITY: mode commands MUST be of the form `exec .cfg`. +// Filenames may only contain alphanumeric characters, '_', '-', '/', '.'. +// No '..' path traversal, no absolute paths, no command chaining. +// Malformed entries are rejected at startup with a warning. +// +// Examples: +// +// teamdm|exec cfg/teamdm.cfg +// ctf|exec cfg/ctf.cfg +// ffa|exec cfg/ffa.cfg +// duel|exec cfg/1v1.cfg +// instagib|exec cfg/instagib.cfg +// campmode|exec cfg/campmode.cfg diff --git a/src/action/a_game.c b/src/action/a_game.c index 5431999c3..1405ab6a4 100644 --- a/src/action/a_game.c +++ b/src/action/a_game.c @@ -1659,6 +1659,7 @@ void ReadLrconConfig(void) game.lrcon_config.quit_on_empty = 0; game.lrcon_config.allowed_cvars_count = 0; game.lrcon_config.modes_count = 0; + game.lrcon_config.allowed_stuffcmds_count = 0; // Get config filename from cvar lrcon_config_cvar = gi.cvar("lrcon_config", "lrcon.cfg", 0); @@ -1738,6 +1739,25 @@ void ReadLrconConfig(void) game.lrcon_config.allowed_cvars[game.lrcon_config.allowed_cvars_count]); game.lrcon_config.allowed_cvars_count++; } + } else if (!strcmp(reading_section, "allowed_stuffcmds")) { + // Comma-delimited list of commands allowed via `lrcon stuffcmd`. + // Why: without an allowlist, a claimer can stuffcmd `disconnect`, + // `quit`, arbitrary `bind`s, or chain commands via ';' — effectively + // RCE on every connected client. + char *tok, *saveptr_buf = buf; + while ((tok = strtok(saveptr_buf, ", \t")) != NULL) { + saveptr_buf = NULL; + if (game.lrcon_config.allowed_stuffcmds_count >= MAX_LRCON_STUFFCMDS) + break; + if (!*tok) + continue; + Q_strncpyz(game.lrcon_config.allowed_stuffcmds[game.lrcon_config.allowed_stuffcmds_count], + tok, sizeof(game.lrcon_config.allowed_stuffcmds[0])); + gi.dprintf("LRCON: allowed stuffcmd %d = %s\n", + game.lrcon_config.allowed_stuffcmds_count, + game.lrcon_config.allowed_stuffcmds[game.lrcon_config.allowed_stuffcmds_count]); + game.lrcon_config.allowed_stuffcmds_count++; + } } else if (!strcmp(reading_section, "modes")) { // Format: name|exec // Why: mode command is passed verbatim to AddCommandString. diff --git a/src/action/g_local.h b/src/action/g_local.h index 409384ad5..fadf83fcc 100644 --- a/src/action/g_local.h +++ b/src/action/g_local.h @@ -793,6 +793,7 @@ typedef struct precache_s { #define MAX_LRCON_CVARS 32 #define MAX_LRCON_MODES 16 +#define MAX_LRCON_STUFFCMDS 16 /* LRCON state - tracks current server claim */ typedef struct { @@ -817,6 +818,8 @@ typedef struct { char allowed_cvars[MAX_LRCON_CVARS][64]; /* Whitelisted cvar names */ int modes_count; /* Number of available modes */ lrcon_mode_t modes[MAX_LRCON_MODES]; /* Available server modes */ + int allowed_stuffcmds_count; /* Number of allowlisted client stuffcmds */ + char allowed_stuffcmds[MAX_LRCON_STUFFCMDS][32]; /* Allowlisted commands for `lrcon stuffcmd` */ } lrcon_config_t; // diff --git a/src/action/g_lrcon.c b/src/action/g_lrcon.c index 13a899e15..d493f7d9a 100644 --- a/src/action/g_lrcon.c +++ b/src/action/g_lrcon.c @@ -365,8 +365,12 @@ void Lrcon_Stuffcmd(edict_t *ent) const char *target_arg; const char *command; const char *cmd_start; + const char *p; + char cmd_name[32]; edict_t *target; int i, skip_count; + size_t name_len; + qboolean allowed; if (!Lrcon_CheckClaimer(ent)) return; @@ -386,6 +390,48 @@ void Lrcon_Stuffcmd(edict_t *ent) cmd_start++; } + /* Reject command-chaining or substitution characters anywhere in the + * payload. Without this, the allowlist below can be bypassed via + * `; `. */ + for (p = cmd_start; *p; p++) { + if (*p == ';' || *p == '\n' || *p == '\r' || *p == '$') { + gi.cprintf(ent, PRINT_HIGH, + "lrcon stuffcmd: command contains disallowed character\n"); + return; + } + } + + /* Extract the command name (first whitespace-delimited token) and check + * it against the operator-configured allowlist. The allowlist is loaded + * from the [allowed_stuffcmds] section in lrcon.cfg as a comma-delimited + * list. If empty, all stuffcmds are denied — secure-by-default. */ + for (name_len = 0; cmd_start[name_len] && cmd_start[name_len] != ' ' && + cmd_start[name_len] != '\t' && name_len < sizeof(cmd_name) - 1; + name_len++) { + cmd_name[name_len] = cmd_start[name_len]; + } + cmd_name[name_len] = '\0'; + + if (!cmd_name[0]) { + gi.cprintf(ent, PRINT_HIGH, "lrcon stuffcmd: empty command\n"); + return; + } + + allowed = false; + for (i = 0; i < game.lrcon_config.allowed_stuffcmds_count; i++) { + if (!Q_stricmp(cmd_name, game.lrcon_config.allowed_stuffcmds[i])) { + allowed = true; + break; + } + } + + if (!allowed) { + gi.cprintf(ent, PRINT_HIGH, + "lrcon stuffcmd: '%s' is not in [allowed_stuffcmds]\n", + cmd_name); + return; + } + if (!Q_stricmp(target_arg, "all")) { /* Send to all clients */ for (i = 0; i < game.maxclients; i++) { @@ -393,16 +439,16 @@ void Lrcon_Stuffcmd(edict_t *ent) if (!target->inuse || !target->client) continue; stuffcmd(target, va("%s\n", cmd_start)); } - gi.bprintf(PRINT_HIGH, "%s sent command to all players\n", - ent->client->pers.netname); + gi.bprintf(PRINT_HIGH, "%s sent '%s' to all players\n", + ent->client->pers.netname, cmd_name); } else { /* Send to specific client */ target = LookupPlayer(ent, target_arg, true, false); if (!target) return; stuffcmd(target, va("%s\n", cmd_start)); - gi.bprintf(PRINT_HIGH, "%s sent command to %s\n", - ent->client->pers.netname, target->client->pers.netname); + gi.bprintf(PRINT_HIGH, "%s sent '%s' to %s\n", + ent->client->pers.netname, cmd_name, target->client->pers.netname); } }