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
1 file src/exec.zig+110 −2
src/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.

view file