From d28195c08ca67cf6190d78251dcb0515a9cbfa21 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 15:34:44 +0000 Subject: [PATCH] fix(security): harden the exec/FS trust boundary in server_actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three injection classes were reachable from GUI-supplied values: 1. Remote command injection. The SSH paths in executeAction/streamLogs/ getServerStatus built 'ssh podman ... ' with no protection; ssh concatenates its remote args into one string the remote login shell re-parses, so a container_name like 'foo; rm -rf /' ran the rm. (executeSSH existed but was unused AND its own '--' does not stop remote-shell reparsing — its doc comment was wrong.) 2. SSH option injection. An unvalidated host such as '-oProxyCommand=evil' is read by ssh as an option. 3. Path traversal. run_script accepted 'scripts/../../etc/x' (prefix check only); write_server_config interpolated a raw profile_id into a path; and Settings.xml element text was interpolated with no escaping (operator attrs escaped only '"'). Fixes: - Allowlist validators isSafeContainerName / isSafeHost / isSafeProfileId reject every shell metacharacter and path-traversal char at the boundary. - Single SSH path: the three callers build a bare command and route through dispatch() -> executeSSH, which now POSIX-single-quotes every remote arg (shellQuoteInto) as defence-in-depth; false doc comment corrected. - run_script also rejects '..' components and absolute paths. - write_server_config validates profile_id and XML-escapes all five entities in every interpolated value (xmlEscapeInto/Alloc). - parseAndDispatch converts validation errors into a structured failure result instead of propagating. - 10 adversarial unit tests (injection payloads, traversal, quoting, escaping). Grep-gate: no inline 'ssh' arg-building remains. 150 tests green. Also adds timeout-minutes to push-email-notify.yml (Hypatia workflow_audit). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TaWWedv6VQqeZaPvc94keN --- .github/workflows/push-email-notify.yml | 1 + src/interface/ffi/src/server_actions.zig | 391 ++++++++++++++++++----- 2 files changed, 319 insertions(+), 73 deletions(-) diff --git a/.github/workflows/push-email-notify.yml b/.github/workflows/push-email-notify.yml index 4b4e754..cf36822 100644 --- a/.github/workflows/push-email-notify.yml +++ b/.github/workflows/push-email-notify.yml @@ -13,6 +13,7 @@ jobs: name: Email on push if: ${{ vars.PUSH_EMAIL_ENABLED == 'true' }} runs-on: ubuntu-latest + timeout-minutes: 5 steps: - name: Send push notification email uses: dawidd6/action-send-mail@6e502825a508b867ab2954ad6343b68787624c01 # pinned diff --git a/src/interface/ffi/src/server_actions.zig b/src/interface/ffi/src/server_actions.zig index 16b9542..10e848f 100644 --- a/src/interface/ffi/src/server_actions.zig +++ b/src/interface/ffi/src/server_actions.zig @@ -38,6 +38,105 @@ pub const ActionResult = struct { /// Container runtime used to manage the server. pub const Runtime = enum { podman, docker, systemd }; +// ═══════════════════════════════════════════════════════════════════════════════ +// Input validation — the exec/FS trust boundary +// +// Values that reach a command line or a filesystem path (container names, +// hostnames, profile ids) are validated against strict allowlists BEFORE use. +// This is the primary defence: an SSH command is ultimately re-parsed by the +// remote login shell (ssh concatenates its remote args into one string — the +// `--` separator only stops *ssh* from reading them as options, it does NOT +// stop the remote shell), so the only robust guard is to ensure the values +// carry no shell metacharacters and no path-traversal in the first place. +// ═══════════════════════════════════════════════════════════════════════════════ + +/// A container / systemd-unit name is safe if non-empty, starts alphanumeric, +/// and contains only `[A-Za-z0-9._@-]` — the Podman/Docker charset plus `@` +/// for systemd template units. None of these are shell metacharacters. +pub fn isSafeContainerName(name: []const u8) bool { + if (name.len == 0 or name.len > 255) return false; + if (!std.ascii.isAlphanumeric(name[0])) return false; + for (name) |c| { + if (!(std.ascii.isAlphanumeric(c) or c == '.' or c == '_' or c == '-' or c == '@')) return false; + } + return true; +} + +/// An SSH host is safe if non-empty, does not start with '-' (which ssh would +/// read as an option — e.g. `-oProxyCommand=…`), and contains only hostname / +/// IP-literal characters `[A-Za-z0-9._:\[\]-]`. No shell metacharacters, no +/// whitespace. +pub fn isSafeHost(host: []const u8) bool { + if (host.len == 0 or host.len > 255) return false; + if (host[0] == '-') return false; + for (host) |c| { + if (!(std.ascii.isAlphanumeric(c) or c == '.' or c == '_' or + c == '-' or c == ':' or c == '[' or c == ']')) return false; + } + return true; +} + +/// A profile id is safe (for use in a filesystem path) if non-empty and made +/// only of `[A-Za-z0-9_-]`. Excluding '.' and '/' makes path traversal +/// (`../`) impossible by construction. +pub fn isSafeProfileId(id: []const u8) bool { + if (id.len == 0 or id.len > 128) return false; + for (id) |c| { + if (!(std.ascii.isAlphanumeric(c) or c == '_' or c == '-')) return false; + } + return true; +} + +/// True if `path` contains a `..` component (splitting on both `/` and `\`). +/// Catches traversal that a prefix check alone would miss. +fn pathHasDotDot(path: []const u8) bool { + var it = std.mem.splitAny(u8, path, "/\\"); + while (it.next()) |seg| { + if (std.mem.eql(u8, seg, "..")) return true; + } + return false; +} + +/// Append `s` to `list` as a POSIX-shell single-quoted token: wrap in single +/// quotes and replace each embedded `'` with `'\''`. Safe against every shell +/// metacharacter. Used to belt-and-suspenders the remote SSH command even +/// though callers also allowlist-validate their inputs. +fn shellQuoteInto(list: *std.ArrayList(u8), allocator: Allocator, s: []const u8) !void { + try list.append(allocator, '\''); + for (s) |c| { + if (c == '\'') { + try list.appendSlice(allocator, "'\\''"); + } else { + try list.append(allocator, c); + } + } + try list.append(allocator, '\''); +} + +/// Append `s` to `list` with the five XML predefined entities escaped, so a +/// value can be safely interpolated into element text or a double-quoted +/// attribute without breaking (or injecting into) the document. +fn xmlEscapeInto(list: *std.ArrayList(u8), allocator: Allocator, s: []const u8) !void { + for (s) |c| { + switch (c) { + '&' => try list.appendSlice(allocator, "&"), + '<' => try list.appendSlice(allocator, "<"), + '>' => try list.appendSlice(allocator, ">"), + '"' => try list.appendSlice(allocator, """), + '\'' => try list.appendSlice(allocator, "'"), + else => try list.append(allocator, c), + } + } +} + +/// XML-escape `s` into a freshly allocated, caller-owned buffer. +fn xmlEscapeAlloc(allocator: Allocator, s: []const u8) ![]u8 { + var list: std.ArrayList(u8) = .empty; + errdefer list.deinit(allocator); + try xmlEscapeInto(&list, allocator, s); + return list.toOwnedSlice(allocator); +} + // ═══════════════════════════════════════════════════════════════════════════════ // Action execution // ═══════════════════════════════════════════════════════════════════════════════ @@ -59,22 +158,23 @@ pub fn executeAction( .systemd => "systemctl", }; - // Build the command arguments + // Validate untrusted inputs at the trust boundary before they can reach a + // command line (local exec or a shell-reparsed remote SSH command). + if (!isSafeContainerName(container_name)) { + main.setErrorStr("invalid container/unit name"); + return error.InvalidParam; + } + const remote = host.len > 0 and !isLocalhost(host); + if (remote and !isSafeHost(host)) { + main.setErrorStr("invalid host"); + return error.InvalidParam; + } + + // Build the container/systemd command WITHOUT any SSH prefix; dispatch() + // decides local-vs-remote and applies safe SSH quoting for the remote case. var argv_buf: [16][]const u8 = undefined; var argc: usize = 0; - // If remote, prefix with SSH - if (host.len > 0 and !isLocalhost(host)) { - argv_buf[argc] = "ssh"; - argc += 1; - argv_buf[argc] = "-o"; - argc += 1; - argv_buf[argc] = "StrictHostKeyChecking=accept-new"; - argc += 1; - argv_buf[argc] = host; - argc += 1; - } - switch (runtime) { .podman, .docker => { argv_buf[argc] = runtime_cmd; @@ -187,7 +287,7 @@ pub fn executeAction( argc += 1; argv_buf[argc] = "--no-pager"; argc += 1; - return runCommand(allocator, argv_buf[0..argc]); + return dispatch(allocator, host, argv_buf[0..argc]); }, else => { argv_buf[argc] = "status"; @@ -200,15 +300,30 @@ pub fn executeAction( }, } - return runCommand(allocator, argv_buf[0..argc]); + return dispatch(allocator, host, argv_buf[0..argc]); +} + +/// Run `cmd_argv` either locally or, for a non-local host, over SSH. Callers +/// build the bare command (no ssh prefix); this centralises the local-vs-remote +/// decision so there is exactly one SSH code path. Assumes the caller has +/// already validated `host` via `isSafeHost` when remote. +fn dispatch(allocator: Allocator, host: []const u8, cmd_argv: []const []const u8) !ActionResult { + if (host.len > 0 and !isLocalhost(host)) { + return executeSSH(allocator, host, "", cmd_argv); + } + return runCommand(allocator, cmd_argv); } /// Run a command via SSH on a remote host. /// -/// `argv` is passed as a separate exec argument to SSH via `ssh -- host argv[0] argv[1]...`, -/// so each element is passed as a distinct argument to the remote process — the remote -/// shell is bypassed entirely (SSH calls execvp directly when given an arg list, not -/// a shell string). This prevents shell injection from user-supplied values. +/// SSH does NOT bypass the remote shell: it concatenates the post-target +/// arguments into a single string that the remote login shell re-parses (the +/// `--` separator only stops *ssh itself* from reading them as options). So to +/// prevent injection we POSIX-single-quote every element of `remote_argv` +/// (`shellQuoteInto`), which neutralises all shell metacharacters. Callers +/// additionally allowlist-validate the untrusted values, making this +/// defence-in-depth. `user` may be empty, in which case the target is just +/// `host` (ssh uses the default user). /// /// Returns the combined stdout output and exit code. pub fn executeSSH( @@ -218,24 +333,33 @@ pub fn executeSSH( remote_argv: []const []const u8, ) !ActionResult { if (remote_argv.len == 0) return error.InvalidParam; + if (!isSafeHost(host)) return error.InvalidParam; var target_buf: [512]u8 = undefined; - const target = std.fmt.bufPrint(&target_buf, "{s}@{s}", .{ user, host }) catch return error.InvalidParam; + const target = if (user.len > 0) + std.fmt.bufPrint(&target_buf, "{s}@{s}", .{ user, host }) catch return error.InvalidParam + else + host; + + // Join the (individually shell-quoted) remote args into one command string. + // ssh will pass this string to the remote login shell; the quoting makes + // each original element a single literal token there. + var remote_cmd: std.ArrayList(u8) = .empty; + defer remote_cmd.deinit(allocator); + for (remote_argv, 0..) |a, i| { + if (i != 0) try remote_cmd.append(allocator, ' '); + try shellQuoteInto(&remote_cmd, allocator, a); + } - // Build: ssh -o ... target -- remote_argv[0] remote_argv[1]... - const fixed_prefix: []const []const u8 = &.{ + const full_argv: []const []const u8 = &.{ "ssh", "-o", "StrictHostKeyChecking=accept-new", "-o", "ConnectTimeout=10", target, "--", + remote_cmd.items, }; - const full_argv = try allocator.alloc([]const u8, fixed_prefix.len + remote_argv.len); - defer allocator.free(full_argv); - @memcpy(full_argv[0..fixed_prefix.len], fixed_prefix); - @memcpy(full_argv[fixed_prefix.len..], remote_argv); - return runCommand(allocator, full_argv); } @@ -253,19 +377,22 @@ pub fn streamLogs( .systemd => "journalctl", }; + if (!isSafeContainerName(container_name)) { + main.setErrorStr("invalid container/unit name"); + return error.InvalidParam; + } + const remote = host.len > 0 and !isLocalhost(host); + if (remote and !isSafeHost(host)) { + main.setErrorStr("invalid host"); + return error.InvalidParam; + } + var lines_buf: [16]u8 = undefined; const lines_str = std.fmt.bufPrint(&lines_buf, "{d}", .{lines}) catch "100"; var argv_buf: [16][]const u8 = undefined; var argc: usize = 0; - if (host.len > 0 and !isLocalhost(host)) { - argv_buf[argc] = "ssh"; - argc += 1; - argv_buf[argc] = host; - argc += 1; - } - switch (runtime) { .podman, .docker => { argv_buf[argc] = runtime_cmd; @@ -295,7 +422,7 @@ pub fn streamLogs( }, } - const result = try runCommand(allocator, argv_buf[0..argc]); + const result = try dispatch(allocator, host, argv_buf[0..argc]); if (!result.success) { allocator.free(result.output); return error.LogRetrievalFailed; @@ -312,16 +439,19 @@ pub fn getServerStatus( host: []const u8, container_name: []const u8, ) ![]const u8 { + if (!isSafeContainerName(container_name)) { + main.setErrorStr("invalid container/unit name"); + return error.InvalidParam; + } + const remote = host.len > 0 and !isLocalhost(host); + if (remote and !isSafeHost(host)) { + main.setErrorStr("invalid host"); + return error.InvalidParam; + } + var argv_buf: [16][]const u8 = undefined; var argc: usize = 0; - if (host.len > 0 and !isLocalhost(host)) { - argv_buf[argc] = "ssh"; - argc += 1; - argv_buf[argc] = host; - argc += 1; - } - argv_buf[argc] = "podman"; argc += 1; argv_buf[argc] = "inspect"; @@ -333,7 +463,7 @@ pub fn getServerStatus( argv_buf[argc] = container_name; argc += 1; - const result = try runCommand(allocator, argv_buf[0..argc]); + const result = try dispatch(allocator, host, argv_buf[0..argc]); if (!result.success) { // Container might not exist — return a synthetic status allocator.free(result.output); @@ -447,7 +577,13 @@ fn parseAndDispatch(allocator: Allocator, json_str: []const u8) !ActionResult { else .podman; - return executeAction(allocator, host, action, container, runtime); + return executeAction(allocator, host, action, container, runtime) catch |err| { + // Validation failures (bad container name / host) and exec errors are + // surfaced as a structured failure result, not a crash — the GUI shows + // the message via gossamer_gsa_server_action's JSON envelope. + const msg = try std.fmt.allocPrint(allocator, "action rejected: {s}", .{@errorName(err)}); + return ActionResult{ .success = false, .output = msg, .exit_code = -1 }; + }; } // ═══════════════════════════════════════════════════════════════════════════════ @@ -582,9 +718,14 @@ pub export fn gossamer_gsa_run_script( const env_json_str = std.mem.span(env_json_z); const secret = std.mem.span(stdin_secret_z); - // Path constraint: must start with "scripts/" - if (!std.mem.startsWith(u8, script_path, "scripts/")) { - main.setErrorStr("script path must be under scripts/"); + // Path constraint: must be under scripts/ AND contain no traversal. A bare + // startsWith("scripts/") is bypassable ("scripts/../../etc/x"), so also + // reject any ".." path component and any absolute path. + if (!std.mem.startsWith(u8, script_path, "scripts/") or + std.fs.path.isAbsolute(script_path) or + pathHasDotDot(script_path)) + { + main.setErrorStr("script path must be under scripts/ with no traversal"); return @intFromEnum(main.GsaResult.permission_denied); } @@ -720,8 +861,10 @@ pub export fn gossamer_gsa_write_server_config( const config_str = std.mem.span(config_json_z); const ops_str = std.mem.span(operators_json_z); - if (profile_id.len == 0) { - main.setErrorStr("missing profile_id"); + // profile_id becomes a path segment (container/{profile_id}/Settings.xml), + // so it must be a strict allowlisted identifier — no '.', '/', or traversal. + if (!isSafeProfileId(profile_id)) { + main.setErrorStr("invalid profile_id"); return @intFromEnum(main.GsaResult.invalid_param); } @@ -753,21 +896,41 @@ pub export fn gossamer_gsa_write_server_config( } }; - const server_name = S.get(cfg, "ServerName", "GSA Server"); - const description = S.get(cfg, "ServerDescription", "Managed by GSA"); - const welcome = S.get(cfg, "WelcomeMessage", "Welcome!"); - const port = S.get(cfg, "ServerPort", "6000"); - const max_players = S.get(cfg, "MaxPlayers", "10"); - const is_private = S.get(cfg, "IsPrivate", "true"); - const password = S.get(cfg, "ServerPassword", ""); - const is_pve = S.get(cfg, "IsPvE", "true"); - const raids = S.get(cfg, "IsRaidsEnabled", "false"); - const wipe_days = S.get(cfg, "WipePeriodDays", "0"); - const gather_mult = S.get(cfg, "GatheringSpeedMultiplier", "2.0"); - const learn_mult = S.get(cfg, "LearningSpeedMultiplier", "2.0"); - const craft_mult = S.get(cfg, "CraftingSpeedMultiplier", "2.0"); - - // Build operators XML fragment + // Every value below is operator-supplied (via the Nexus Setup GUI form) and + // is interpolated into XML element text / attributes, so each is XML-escaped + // to prevent breaking or injecting into the document. Escaped copies are + // arena-freed together at function exit. + var xml_arena = std.heap.ArenaAllocator.init(allocator); + defer xml_arena.deinit(); + const xa = xml_arena.allocator(); + const esc = struct { + fn e(a: Allocator, obj: std.json.ObjectMap, key: []const u8, fallback: []const u8) []const u8 { + const raw = if (obj.get(key)) |v| + (switch (v) { + .string => |s| s, + else => fallback, + }) + else + fallback; + return xmlEscapeAlloc(a, raw) catch fallback; + } + }.e; + + const server_name = esc(xa, cfg, "ServerName", "GSA Server"); + const description = esc(xa, cfg, "ServerDescription", "Managed by GSA"); + const welcome = esc(xa, cfg, "WelcomeMessage", "Welcome!"); + const port = esc(xa, cfg, "ServerPort", "6000"); + const max_players = esc(xa, cfg, "MaxPlayers", "10"); + const is_private = esc(xa, cfg, "IsPrivate", "true"); + const password = esc(xa, cfg, "ServerPassword", ""); + const is_pve = esc(xa, cfg, "IsPvE", "true"); + const raids = esc(xa, cfg, "IsRaidsEnabled", "false"); + const wipe_days = esc(xa, cfg, "WipePeriodDays", "0"); + const gather_mult = esc(xa, cfg, "GatheringSpeedMultiplier", "2.0"); + const learn_mult = esc(xa, cfg, "LearningSpeedMultiplier", "2.0"); + const craft_mult = esc(xa, cfg, "CraftingSpeedMultiplier", "2.0"); + + // Build operators XML fragment (fully XML-escaped attributes) var ops_xml: std.ArrayList(u8) = .empty; defer ops_xml.deinit(allocator); @@ -778,16 +941,9 @@ pub export fn gossamer_gsa_write_server_config( const name = S.get(item.object, "name", ""); if (steam_id.len == 0) continue; ops_xml.appendSlice(allocator, " \n") catch continue; } } @@ -959,3 +1115,92 @@ test "executeAction: systemd logs uses journalctl" { _ = result.exit_code; _ = result.success; } + +// ── Security: exec / FS trust boundary ─────────────────────────────────────── + +test "isSafeContainerName rejects shell metacharacters, accepts valid" { + try std.testing.expect(isSafeContainerName("mc-server_1.prod")); + try std.testing.expect(isSafeContainerName("minecraft@main")); // systemd template unit + try std.testing.expect(!isSafeContainerName("foo; rm -rf /")); + try std.testing.expect(!isSafeContainerName("foo`whoami`")); + try std.testing.expect(!isSafeContainerName("foo$(id)")); + try std.testing.expect(!isSafeContainerName("foo bar")); + try std.testing.expect(!isSafeContainerName("foo|bar")); + try std.testing.expect(!isSafeContainerName("-startsdash")); + try std.testing.expect(!isSafeContainerName("")); +} + +test "isSafeHost rejects ssh-option injection and metacharacters" { + try std.testing.expect(isSafeHost("example.com")); + try std.testing.expect(isSafeHost("10.0.0.5")); + try std.testing.expect(isSafeHost("[2001:db8::1]")); + try std.testing.expect(!isSafeHost("-oProxyCommand=evil")); // leading dash = ssh option + try std.testing.expect(!isSafeHost("host; rm -rf /")); + try std.testing.expect(!isSafeHost("host$(id)")); + try std.testing.expect(!isSafeHost("")); +} + +test "isSafeProfileId forbids path traversal characters" { + try std.testing.expect(isSafeProfileId("cryofall")); + try std.testing.expect(isSafeProfileId("minecraft-java")); + try std.testing.expect(!isSafeProfileId("../../etc")); + try std.testing.expect(!isSafeProfileId("a/b")); + try std.testing.expect(!isSafeProfileId("a.b")); // '.' excluded to kill ".." + try std.testing.expect(!isSafeProfileId("")); +} + +test "pathHasDotDot catches traversal a prefix check misses" { + try std.testing.expect(pathHasDotDot("scripts/../../etc/passwd")); + try std.testing.expect(pathHasDotDot("scripts/./../scripts/x")); + try std.testing.expect(!pathHasDotDot("scripts/steam-stage.sh")); + try std.testing.expect(!pathHasDotDot("scripts/sub/dir/ok.sh")); +} + +test "shellQuoteInto neutralises metacharacters" { + const allocator = std.testing.allocator; + var list: std.ArrayList(u8) = .empty; + defer list.deinit(allocator); + try shellQuoteInto(&list, allocator, "foo; rm -rf /"); + try std.testing.expectEqualStrings("'foo; rm -rf /'", list.items); + + list.clearRetainingCapacity(); + try shellQuoteInto(&list, allocator, "it's"); + try std.testing.expectEqualStrings("'it'\\''s'", list.items); +} + +test "xmlEscapeInto escapes all five predefined entities" { + const allocator = std.testing.allocator; + var list: std.ArrayList(u8) = .empty; + defer list.deinit(allocator); + try xmlEscapeInto(&list, allocator, "Tom & 'Jerry'"); + try std.testing.expectEqualStrings( + "<a href="x">Tom & 'Jerry'</a>", + list.items, + ); +} + +test "executeAction rejects a malicious container name before exec" { + const allocator = std.testing.allocator; + try std.testing.expectError( + error.InvalidParam, + executeAction(allocator, "localhost", .Start, "mc; rm -rf /", .podman), + ); +} + +test "executeAction rejects a malicious remote host before exec" { + const allocator = std.testing.allocator; + try std.testing.expectError( + error.InvalidParam, + executeAction(allocator, "-oProxyCommand=evil", .Start, "mc-1", .podman), + ); +} + +test "run_script rejects path traversal" { + const rc = gossamer_gsa_run_script("scripts/../../etc/passwd", "", "{}", ""); + try std.testing.expectEqual(@intFromEnum(main.GsaResult.permission_denied), rc); +} + +test "write_server_config rejects a traversal profile_id" { + const rc = gossamer_gsa_write_server_config("../../etc", "{}", "[]"); + try std.testing.expectEqual(@intFromEnum(main.GsaResult.invalid_param), rc); +}