08ace490d251
remote (--on) execution shell-quotes argv into a single ssh command word, closing metacharacter injection on the remote shell (NINA-003)
- time
- 2026-07-02 13:22 UTC · 3 months ago
- machine
- koh2
- agent
- claude-sonnet-5
- session
- fable audit fix — exec.zig
- parent
- 2d2b5035db17
↓ nina-08ace490.tar.gzatlas at this save
files changed1 file
src/exec.zig+110 −2src/exec.zig+110 −2
@@ -291,6 +291,25 @@291291 try std.testing.expect(std.mem.indexOf(u8, failed.output, "detail") != null);292292 }293293 294+// Single-quote-wrap `arg` for a POSIX shell, escaping embedded single quotes as295+// '\''. ssh joins its trailing command arguments with spaces and hands the296+// result to the remote login shell for re-parsing, so passing nina's argv297+// through unquoted lets a package name, config path, or channel URL containing298+// shell metacharacters or spaces be word-split or executed on the remote host.299+fn shellQuote(gpa: std.mem.Allocator, arg: []const u8) ![]u8 {300+ var out: std.ArrayList(u8) = .empty;301+ try out.append(gpa, '\'');302+ for (arg) |ch| {303+ if (ch == '\'') {304+ try out.appendSlice(gpa, "'\\''");305+ } else {306+ try out.append(gpa, ch);307+ }308+ }309+ try out.append(gpa, '\'');310+ return out.toOwnedSlice(gpa);311+}312+294313 fn buildArgv(gpa: std.mem.Allocator, machine: *const types.Machine, cmd: []const []const u8) ![]const []const u8 {295314 if (machine.local) return cmd;296315 @@ -314,10 +333,85 @@314333 else315334 try gpa.dupe(u8, host);316335 try ssh.append(gpa, addr);317- for (cmd) |arg| try ssh.append(gpa, arg);336+337+ // Quote every word and join into one ssh argument, so ssh has nothing left338+ // to re-join with spaces — the remote shell receives exactly our argv.339+ var quoted_cmd: std.ArrayList(u8) = .empty;340+ for (cmd, 0..) |arg, i| {341+ if (i > 0) try quoted_cmd.append(gpa, ' ');342+ const q = try shellQuote(gpa, arg);343+ defer gpa.free(q);344+ try quoted_cmd.appendSlice(gpa, q);345+ }346+ try ssh.append(gpa, try quoted_cmd.toOwnedSlice(gpa));318347 return ssh.toOwnedSlice(gpa);319348 }320349 350+test "shellQuote round-trips metacharacters through a real shell" {351+ const a = std.testing.allocator;352+ const io = std.testing.io;353+354+ const dangerous = [_][]const u8{355+ "foo;reboot",356+ "foo && rm -rf /",357+ "has space",358+ "quo'te",359+ "$(whoami)",360+ "`whoami`",361+ "a|b>c<d",362+ };363+364+ for (dangerous) |arg| {365+ const quoted = try shellQuote(a, arg);366+ defer a.free(quoted);367+368+ // A shell asked to print out the quoted word verbatim must reproduce369+ // exactly the original string, byte for byte — proving the metacharacters370+ // stayed inert instead of being word-split or executed.371+ const script = try std.fmt.allocPrint(a, "printf '%s' {s}", .{quoted});372+ defer a.free(script);373+374+ const r = try std.process.run(a, io, .{ .argv = &.{ "sh", "-c", script } });375+ defer a.free(r.stdout);376+ defer a.free(r.stderr);377+378+ try std.testing.expectEqual(@as(u8, 0), switch (r.term) {379+ .exited => |c| c,380+ else => 1,381+ });382+ try std.testing.expectEqualStrings(arg, r.stdout);383+ }384+}385+386+test "buildArgv joins a remote command into a single quoted ssh argument" {387+ const a = std.testing.allocator;388+389+ const machine: types.Machine = .{390+ .name = "box",391+ .local = false,392+ .host = "example.com",393+ .user = "june",394+ };395+396+ const argv = try buildArgv(a, &machine, &.{ "nix-env", "-iA", "foo;reboot" });397+ defer {398+ // Indices 5 (addr) and 6 (the joined quoted command) are the only399+ // gpa-owned strings; the rest are literals baked into buildArgv.400+ a.free(@constCast(argv[5]));401+ a.free(@constCast(argv[6]));402+ a.free(argv);403+ }404+405+ // Everything after the ssh destination must collapse into exactly one406+ // argument — if ssh received the dangerous word as its own separate407+ // argument, it would rejoin it with spaces for the remote shell to408+ // re-parse, defeating the quoting.409+ try std.testing.expectEqualStrings("ssh", argv[0]);410+ try std.testing.expectEqualStrings("[email protected]", argv[5]);411+ try std.testing.expectEqual(@as(usize, 7), argv.len);412+ try std.testing.expectEqualStrings("'nix-env' '-iA' 'foo;reboot'", argv[6]);413+}414+321415 // --- NixOS rebuild ---322416 323417 // True if the machine's config dir holds a flake.nix. Only meaningful for a local@@ -479,6 +573,15 @@479573 const a = std.testing.allocator;480574 const io = std.testing.io;481575 576+ // panel.render() writes through output's global writer, which only main.zig577+ // initializes in real runs. Point it at a discard sink with color off so the578+ // test exercises the real render() call path without a live terminal.579+ var discard_buf: [256]u8 = undefined;580+ var discarding: std.Io.Writer.Discarding = .init(&discard_buf);581+ var env: std.process.Environ.Map = .init(a);582+ defer env.deinit();583+ output.init(&discarding.writer, io, &env, false);584+482585 var panel: output.BuildPanel = .{};483586 484587 // A single stderr line over 8192 bytes (the reader's fixed buffer size) used to@@ -488,6 +591,7 @@488591 @memset(&oversized, 'x');489592 490593 const script = try std.fmt.allocPrint(a,594+ \\exec >&2491595 \\printf 'these 3 paths will be fetched (12.3 MiB download, 45.6 MiB unpacked)\n'492596 \\printf '%s\n' "{s}"493597 \\printf 'error: nina test canary xyz-explanation\n'@@ -505,7 +609,11 @@505609 // extractWarnings) sees one giant concatenated line instead of real lines.506610 try std.testing.expect(std.mem.indexOf(u8, stderr, "error: nina test canary xyz-explanation\n") != null);507611 508- const translated = errors.translateError(a, stderr);612+ // translateError's returned strings are a mix of literals and heap allocations613+ // depending which layer matched; an arena sidesteps the ownership ambiguity.614+ var arena = std.heap.ArenaAllocator.init(a);615+ defer arena.deinit();616+ const translated = errors.translateError(arena.allocator(), stderr);509617 // Must reach Layer 2's structured extraction (innermostError needs a trimmed510618 // line starting with "error:") rather than falling through to the generic511619 // Layer-3 fallback, which is only reachable when line structure is lost.