From cfdfa4c48a4006caef6967a394f9ca8048cd0f7d Mon Sep 17 00:00:00 2001 From: rustytrees Date: Wed, 29 Jul 2026 12:25:56 -0400 Subject: [PATCH] fix(post-install): sanitize child output on the native interpreter path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ui/term_sanitize.zig` says it exists so "a hostile formula cannot rewrite scrollback or exfiltrate via terminal extensions", and it is well built — allowlist-based, dropping OSC (including OSC 52 clipboard writes), DCS, absolute cursor positioning, and scrollback erase. It was only wired into `runRubySandboxed`, i.e. the `--use-system-ruby` path. The native interpreter — the default, and the feature the README leads with — spawned children with stdout inherited, so a formula's `system` call wrote straight to the terminal and none of that filtering applied. The module's own doc comment overstated its coverage. Both stdout and stderr now go through the same pump, reusing the ruby path's `filterInto` rather than growing a second copy of the loop. Two threads, not an inline drain, so a chatty child cannot deadlock by filling one pipe while we block on the other; `std.process.spawn`'s `wait` owns the pipe fds, which is why the non-closing variant was split out. Trade-off: piping means the child no longer sees a TTY, so colour-on-TTY heuristics turn themselves off. That is the same trade the ruby path already makes, and `MALT_ALLOW_RAW_POST_INSTALL=1` continues to opt out of both. `--json`/`--ndjson` suppression still takes precedence. The regression test captures this process's real fd 1 and asserts an OSC 52 sequence never lands on it while the surrounding text survives; it fails against the previous `.inherit` behaviour. --- src/core/dsl/builtins/process.zig | 115 ++++++++++++++++++++++++++---- src/core/sandbox/macos.zig | 12 +++- 2 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/core/dsl/builtins/process.zig b/src/core/dsl/builtins/process.zig index 1b4d4aa8..6d200f98 100644 --- a/src/core/dsl/builtins/process.zig +++ b/src/core/dsl/builtins/process.zig @@ -6,17 +6,46 @@ const values = @import("../values.zig"); const pathname = @import("pathname.zig"); const sandbox = @import("../sandbox.zig"); const fallback_log = @import("../fallback_log.zig"); +const macos_sandbox = @import("../../sandbox/macos.zig"); const Value = values.Value; const BuiltinError = pathname.BuiltinError; const ExecCtx = pathname.ExecCtx; -/// Subprocess stdout shares the FD with malt's `--json` / `--ndjson` -/// document, so verbose tools like fc-cache would corrupt it. The caller -/// decides via `ExecCtx.suppress_child_stdout`; stderr stays inherited so -/// warnings still surface for the user. -fn childStdoutMode(suppress: bool) std.process.SpawnOptions.StdIo { - return if (suppress) .ignore else .inherit; +/// Post_install output is attacker-influenced: a formula's `system` call can +/// emit whatever the child writes, and a terminal will act on it. The +/// `--use-system-ruby` path already pumps the child through +/// `ui/term_sanitize.zig`, which drops OSC (including OSC 52 clipboard +/// writes), DCS, absolute cursor positioning, and scrollback erase. The +/// native interpreter — the default path, and the one the README leads with — +/// inherited the terminal directly, so none of that applied to it. +/// +/// Piping means the child no longer sees a TTY, so colour-on-TTY heuristics +/// turn themselves off. That is the same trade the ruby path already makes, +/// and `MALT_ALLOW_RAW_POST_INSTALL=1` opts out of both. +fn childStdioMode(suppress: bool, raw: bool) std.process.SpawnOptions.StdIo { + if (suppress) return .ignore; + return if (raw) .inherit else .pipe; +} + +/// Pump `child`'s piped stdout/stderr through the sanitizer, then reap it. +/// Threads (rather than draining inline) so a chatty child cannot deadlock by +/// filling one pipe while we block on the other. `wait` owns the pipe fds, so +/// the pumps use the non-closing `filterInto`. +fn waitSanitized(ctx: ExecCtx, child: *std.process.Child) !std.process.Child.Term { + var out_thread: ?std.Thread = null; + var err_thread: ?std.Thread = null; + if (child.stdout) |f| { + out_thread = std.Thread.spawn(.{}, macos_sandbox.filterInto, .{ f.handle, std.c.STDOUT_FILENO }) catch null; + } + if (child.stderr) |f| { + err_thread = std.Thread.spawn(.{}, macos_sandbox.filterInto, .{ f.handle, std.c.STDERR_FILENO }) catch null; + } + // The pumps see EOF when the child's write ends close, i.e. when it exits, + // so joining before `wait` cannot hang on a live child. + if (out_thread) |t| t.join(); + if (err_thread) |t| t.join(); + return child.wait(ctx.io); } /// Coerce every argument to a string and collect them into an owned argv. @@ -53,12 +82,13 @@ pub fn system(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { error.PathSandboxViolation => return BuiltinError.PathSandboxViolation, }; - const child_stdout = childStdoutMode(ctx.suppress_child_stdout); + const raw = macos_sandbox.rawPassthroughEnabled(ctx.environ); var child = std.process.spawn(ctx.io, .{ .argv = spawn_argv, - .stdout = child_stdout, + .stdout = childStdioMode(ctx.suppress_child_stdout, raw), + .stderr = childStdioMode(false, raw), }) catch return BuiltinError.SystemCommandFailed; - const term = child.wait(ctx.io) catch return BuiltinError.SystemCommandFailed; + const term = waitSanitized(ctx, &child) catch return BuiltinError.SystemCommandFailed; switch (term) { .exited => |code| if (code == 0) return Value{ .bool = true } else recordFailure(ctx, argv_slice[0], code), @@ -268,11 +298,15 @@ fn readPipeAll(file: std.Io.File, allocator: std.mem.Allocator, max_bytes: usize return r.interface.allocRemaining(allocator, std.Io.Limit.limited(max_bytes)); } -test "childStdoutMode ignores subprocess stdout only when suppression is requested" { +test "childStdioMode: suppression wins, then raw passthrough, else sanitized pipe" { // Under --json/--ndjson the caller suppresses the child's stdout so it - // can't corrupt the document; otherwise it inherits malt's fd. - try std.testing.expect(std.meta.activeTag(childStdoutMode(true)) == .ignore); - try std.testing.expect(std.meta.activeTag(childStdoutMode(false)) == .inherit); + // can't corrupt the document — that still takes precedence. + try std.testing.expect(std.meta.activeTag(childStdioMode(true, false)) == .ignore); + try std.testing.expect(std.meta.activeTag(childStdioMode(true, true)) == .ignore); + // MALT_ALLOW_RAW_POST_INSTALL=1 hands the terminal straight to the child. + try std.testing.expect(std.meta.activeTag(childStdioMode(false, true)) == .inherit); + // Default: pipe, so the bytes can be run through the sanitizer first. + try std.testing.expect(std.meta.activeTag(childStdioMode(false, false)) == .pipe); } // Homebrew's formula-context `system` raises on failure, so a child @@ -508,3 +542,58 @@ test "system rejects an argv0 outside the sandbox roots before spawning" { system(ctx, null, &.{.{ .string = "/Users/me/evil" }}), ); } + +// The sanitizer only matters if it is actually in the path, so capture this +// process's real stdout and check what a hostile post_install would land on +// the user's terminal. +test "system strips terminal escapes a formula emits, keeping the text" { + if (@import("builtin").os.tag != .macos) return error.SkipZigTest; + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + var lio: std.Io.Threaded = .init(alloc, .{}); + defer lio.deinit(); + + const cap_path = try std.fmt.allocPrintSentinel( + alloc, + "/tmp/malt_sanitize_capture_{d}", + .{std.c.getpid()}, + 0, + ); + defer std.Io.Dir.cwd().deleteFile(lio.io(), cap_path) catch {}; + + // Swap fd 1 for a file, run the builtin, then put the real one back. + const saved = std.c.dup(std.c.STDOUT_FILENO); + try std.testing.expect(saved >= 0); + const cap_fd = std.c.open(cap_path.ptr, .{ .ACCMODE = .WRONLY, .CREAT = true, .TRUNC = true }, @as(std.c.mode_t, 0o600)); + try std.testing.expect(cap_fd >= 0); + _ = std.c.dup2(cap_fd, std.c.STDOUT_FILENO); + _ = std.c.close(cap_fd); + + const ctx = ExecCtx{ + .allocator = alloc, + .io = lio.io(), + .environ = .empty, + .cellar_path = "/opt/malt/Cellar/foo/1.0", + .malt_prefix = "/opt/malt", + }; + // OSC 52 is the clipboard write; the trailing text must survive. + const payload = "\x1b]52;c;ZXZpbA==\x07VISIBLE"; + _ = system(ctx, null, &.{ + .{ .string = "/bin/echo" }, + .{ .string = payload }, + }) catch {}; + + _ = std.c.dup2(saved, std.c.STDOUT_FILENO); + _ = std.c.close(saved); + + const f = try std.Io.Dir.openFileAbsolute(lio.io(), cap_path, .{}); + defer f.close(lio.io()); + var buf: [512]u8 = undefined; + const n = try f.readPositionalAll(lio.io(), &buf, 0); + const got = buf[0..n]; + + try std.testing.expect(std.mem.indexOf(u8, got, "VISIBLE") != null); + try std.testing.expect(std.mem.indexOf(u8, got, "\x1b]52") == null); + try std.testing.expect(std.mem.indexOfScalar(u8, got, 0x1b) == null); +} diff --git a/src/core/sandbox/macos.zig b/src/core/sandbox/macos.zig index abda3f32..d848ee6c 100644 --- a/src/core/sandbox/macos.zig +++ b/src/core/sandbox/macos.zig @@ -282,7 +282,7 @@ pub fn runRubySandboxed( return spawnFiltered(argv_z, envp, limits, stdio); } -fn rawPassthroughEnabled(environ: std.process.Environ) bool { +pub fn rawPassthroughEnabled(environ: std.process.Environ) bool { const v = std.process.Environ.getPosix(environ, "MALT_ALLOW_RAW_POST_INSTALL") orelse return false; return std.mem.eql(u8, v, "1"); } @@ -460,9 +460,17 @@ fn fdSinkWrite(ctx: *anyopaque, bytes: []const u8) term_sanitize.SinkError!void /// Reader thread: pull from the pipe, run through the sanitizer, /// push the surviving bytes to the parent's real fd. Exits on EOF -/// or write error. +/// or write error. Owns `pipe_fd` and closes it. fn filterLoop(pipe_fd: c_int, out_fd: c_int) void { defer _ = std.c.close(pipe_fd); + filterInto(pipe_fd, out_fd); +} + +/// Same pump, but the caller retains ownership of `pipe_fd`. The DSL's +/// `system`/`safe_popen_read` spawn through `std.process.spawn`, whose own +/// `wait` closes the pipes it handed out — closing here as well would free an +/// fd number a later `open` could already have reused. +pub fn filterInto(pipe_fd: c_int, out_fd: c_int) void { var sanitizer = term_sanitize.Sanitizer.init(); var ctx = FdSinkCtx{ .fd = out_fd }; const sink = term_sanitize.Sink{ .ctx = &ctx, .write_fn = fdSinkWrite };