fix(#316) Tier 1: security-critical LRCON + autorecord hardening

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 <id|all> <command>`.
+//
+// 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 <filename>.cfg
+//
+// SECURITY: mode commands MUST be of the form `exec <filename>.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

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));
 }
 

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,15 +1739,78 @@ 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|command
+				// Format: name|exec <filename.cfg>
+				// Why: mode command is passed verbatim to AddCommandString.
+				// Without restriction, an operator (or compromised config) can
+				// embed arbitrary commands via ';'. Restrict to strict
+				// "exec <safe-filename>.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,

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" );
 

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;
 
 //

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);
 
@@ -325,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;
 
@@ -346,23 +390,65 @@ 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
+	 * `<allowed-cmd>; <denied-cmd>`. */
+	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++) {
 			target = g_edicts + 1 + i;
 			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);
 	}
 }