diff --git a/build.zig b/build.zig index f64c44f6..756567fd 100644 --- a/build.zig +++ b/build.zig @@ -343,7 +343,17 @@ pub fn build(b: *std.Build) void { .name = "lib_tests", .root_module = lib_test_module, }); + // `atomic.maltPrefixOrAbort()` reads the ambient environment and falls back + // to the literal `/opt/malt`, so a test that forgets to set a fixture + // prefix operates on the developer's real install — silently, and with + // `createTempDir`/`cleanupTempDir` doing real deletes. Default every test + // run to a throwaway prefix so isolation is a property of the harness + // rather than of each test author remembering. Tests that set their own + // `MALT_PREFIX` are unaffected; those that assert the unset default still + // call `unsetenv` explicitly. + const test_prefix = "/tmp/malt-test-prefix"; const run_lib_tests = b.addRunArtifact(lib_tests); + run_lib_tests.setEnvironmentVariable("MALT_PREFIX", test_prefix); test_step.dependOn(&run_lib_tests.step); const install_lib_tests = b.addInstallArtifact(lib_tests, .{ @@ -383,6 +393,7 @@ pub fn build(b: *std.Build) void { t.root_module.addImport("test_io", test_io_mod); const run_t = b.addRunArtifact(t); + run_t.setEnvironmentVariable("MALT_PREFIX", test_prefix); test_step.dependOn(&run_t.step); // Only installed when the user asks for `zig build test-bin` diff --git a/src/core/cask.zig b/src/core/cask.zig index b688b316..8e5dbcec 100644 --- a/src/core/cask.zig +++ b/src/core/cask.zig @@ -67,6 +67,11 @@ pub fn parseCask(allocator: std.mem.Allocator, json_bytes: []const u8) !Cask { // component must be rejected here — the one ingestion choke point — // before any sink sees it. A compromised tap is the threat. if (!path_component.isPathComponent(token) or !path_component.isPathComponent(version)) return CaskError.ParseFailed; + // Artifact strings are the *other* half of the tap-controlled path surface: + // `app` lands in `/` ahead of a `deleteTree`, and `binary` + // resolves under the keg and symlinks into `/bin`. Screen them at + // the same choke point rather than at each sink. + try validateArtifactPaths(obj); return .{ .token = token, @@ -453,6 +458,61 @@ pub fn parseBinaryTarget(obj: std.json.ObjectMap) ?[]const u8 { return null; } +/// Homebrew lets a `binary` source name the prefix explicitly; the remainder is +/// still a subpath and is screened as one. +const homebrew_prefix_var = "$HOMEBREW_PREFIX/"; + +/// Reject any `app` or `binary` artifact string that would escape the directory +/// it is resolved against. Only the artifact kinds malt actually turns into +/// paths are screened: `font` has its own sanitizer in `cask_font.zig`, and +/// `pkg` reaches the installer as the cached download path, not as a name. +fn validateArtifactPaths(obj: std.json.ObjectMap) CaskError!void { + const artifacts = switch (obj.get("artifacts") orelse return) { + .array => |a| a, + else => return, + }; + + for (artifacts.items) |item| { + const art = switch (item) { + .object => |o| o, + else => continue, + }; + + // `app` — every string entry becomes `/`. + if (art.get("app")) |val| if (val == .array) { + for (val.array.items) |entry| switch (entry) { + .string => |s| if (!path_component.isRelativeSubpath(s)) return CaskError.ParseFailed, + else => {}, + }; + }; + + // `binary` — items[0] is the source under the keg; any trailing object + // may carry a `target` rename hint that becomes `/bin/`. + if (art.get("binary")) |val| if (val == .array) { + for (val.array.items, 0..) |entry, i| switch (entry) { + .string => |s| { + const rel = if (std.mem.startsWith(u8, s, homebrew_prefix_var)) + s[homebrew_prefix_var.len..] + else + s; + if (!path_component.isRelativeSubpath(rel)) return CaskError.ParseFailed; + }, + .object => |o| { + // Only the first element is the source; later objects are + // option hashes, and `target` is the one malt honours. + if (i == 0) continue; + if (o.get("target")) |tv| switch (tv) { + // The link name is a single entry in `/bin`. + .string => |t| if (!path_component.isPathComponent(t)) return CaskError.ParseFailed, + else => {}, + }; + }, + else => {}, + }; + }; + } +} + fn firstArtifactArray(obj: std.json.ObjectMap, key: []const u8) ?std.json.Array { const artifacts_val = obj.get("artifacts") orelse return null; const artifacts = switch (artifacts_val) { @@ -1112,13 +1172,19 @@ pub const CaskInstaller = struct { fn resolveCaskBinaryPath(self: *CaskInstaller, root: []const u8, src: []const u8) ![]u8 { const env_prefix = "$HOMEBREW_PREFIX/"; if (std.mem.startsWith(u8, src, env_prefix)) { + const rel = src[env_prefix.len..]; + // Second line of defence: `parseCask` already screened this, but the + // resolved path is opened read-write and chmod 0755'd, so the sink + // does not lean on an upstream guard. + if (!path_component.isRelativeSubpath(rel)) return error.InstallFailed; return try std.fmt.allocPrint( self.allocator, "{s}/{s}", - .{ self.prefix, src[env_prefix.len..] }, + .{ self.prefix, rel }, ); } if (std.mem.indexOfScalar(u8, src, '/') != null) { + if (!path_component.isRelativeSubpath(src)) return error.InstallFailed; return try std.fmt.allocPrint(self.allocator, "{s}/{s}", .{ root, src }); } return (findFileInTree(self.io, self.allocator, root, src) catch null) orelse @@ -1146,6 +1212,10 @@ pub const CaskInstaller = struct { exec_file.setPermissions(self.io, std.Io.File.Permissions.fromMode(0o755)) catch {}; exec_file.close(self.io); + // The link name is one entry in `/bin`; a `target` carrying a + // separator would delete and re-create somewhere else entirely. + if (!path_component.isPathComponent(link_name)) return error.InstallFailed; + var bin_parent_buf: [512]u8 = undefined; const bin_parent = std.fmt.bufPrint(&bin_parent_buf, "{s}/bin", .{self.prefix}) catch return error.InstallFailed; @@ -1237,12 +1307,16 @@ pub fn hashFileSha256(io: std.Io, file_path: []const u8) ![64]u8 { return hash_mod.hashFileSha256Hex(io, file_path); } -/// Verify `file_path` hashes to `expected` (lowercase hex). A null -/// `expected` or the literal `"no_check"` skips verification — -/// mirrors Homebrew's `sha256 :no_check` escape hatch for casks that +/// Verify `file_path` hashes to `expected` (lowercase hex). The literal +/// `"no_check"` skips verification — Homebrew's escape hatch for casks that /// cannot be pinned (auto-updating installers). +/// +/// An *absent* hash is not that escape hatch. Homebrew always emits `sha256` +/// for a cask, so a null here means a malformed or hostile manifest; treating +/// it as "verified" would silently drop every cask to transport-only integrity, +/// and `installPkg` hands the result to `sudo installer -target /`. pub fn verifyFileSha256(io: std.Io, file_path: []const u8, expected: ?[]const u8) !void { - const expected_hash = expected orelse return; + const expected_hash = expected orelse return error.Sha256Missing; if (std.mem.eql(u8, expected_hash, "no_check")) return; const got = try hashFileSha256(io, file_path); @@ -1552,6 +1626,104 @@ test "parseCask rejects path-traversal in token or version" { } } +test "parseCask rejects path-traversal in an app artifact" { + const a = std.testing.allocator; + // `app` names are interpolated into `/` and handed to + // `deleteTree` before the copy runs, so a climbing name is a destructive + // primitive for a hostile tap — reject it at the same choke point that + // already screens token/version. + const bad = [_][]const u8{ + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":["../Evil.app"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":["../../../../Users/x/Documents"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":["/Applications/Evil.app"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":["Sub/../../Evil.app"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":[""]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":["Evil\u0000.app"]}]} + , + }; + for (bad) |json| { + try std.testing.expectError(error.ParseFailed, parseCask(a, json)); + } +} + +test "parseCask rejects path-traversal in a binary artifact" { + const a = std.testing.allocator; + // The `binary` source resolves under the keg and the `target` rename hint + // becomes `/bin/` for a deleteFile + symlink pair. Both + // escape their root if a `..` survives ingestion. + const bad = [_][]const u8{ + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["../../../etc/evil"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["/etc/passwd"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["tool",{"target":"../../../../Users/x/.zshenv"}]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["tool",{"target":"sub/tool"}]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["$HOMEBREW_PREFIX/../../etc/evil"]}]} + , + }; + for (bad) |json| { + try std.testing.expectError(error.ParseFailed, parseCask(a, json)); + } +} + +test "parseCask accepts the artifact shapes real casks use" { + const a = std.testing.allocator; + // The guard must not narrow what already installs: nested app bundles, + // a binary nested under the extracted tree, the `$HOMEBREW_PREFIX/` form, + // and a plain rename target all stay valid. + const ok = [_][]const u8{ + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":["Firefox.app"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"app":["Sub Dir/My App.app"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["bin/tool"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["$HOMEBREW_PREFIX/bin/tool"]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.tar.gz","artifacts":[{"binary":["codex-aarch64-apple-darwin",{"target":"codex"}]}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.zip","artifacts":[{"font":["Some.ttf"],"target":"whatever/ignored"}]} + , + \\{"token":"ok","version":"1.0","url":"https://e/x.pkg","artifacts":[{"pkg":["Thing.pkg"]}]} + , + }; + for (ok) |json| { + var cask = try parseCask(a, json); + cask.deinit(); + } +} + +test "verifyFileSha256 refuses an artifact with no declared hash" { + // Homebrew always emits `sha256` for a cask — a real digest or the literal + // `no_check`. Treating an *absent* field as "verified" silently downgrades + // every such cask to transport-only integrity, and `installPkg` hands the + // result to `sudo installer`. + const io = std.Options.debug_io; + const a = std.testing.allocator; + // Process-unique so overlapping test runs can't share the fixture. + const f = try std.fmt.allocPrint(a, "/tmp/malt_cask_nosha_{d}", .{std.c.getpid()}); + defer a.free(f); + defer std.Io.Dir.cwd().deleteFile(io, f) catch {}; + { + const fh = try std.Io.Dir.createFileAbsolute(io, f, .{ .truncate = true }); + defer fh.close(io); + try fh.writeStreamingAll(io, "TAMPERED"); + } + + try std.testing.expectError(error.Sha256Missing, verifyFileSha256(io, f, null)); + // The explicit opt-out still works, and a real digest still verifies. + try verifyFileSha256(io, f, "no_check"); + const good = try hashFileSha256(io, f); + try verifyFileSha256(io, f, &good); +} + test "parseCask accepts legitimate token and version" { const a = std.testing.allocator; // The guard is charset-agnostic, so real versions an allowlist would diff --git a/src/core/dsl/builtins/fileutils.zig b/src/core/dsl/builtins/fileutils.zig index 5c24a953..0e254560 100644 --- a/src/core/dsl/builtins/fileutils.zig +++ b/src/core/dsl/builtins/fileutils.zig @@ -18,6 +18,21 @@ const Value = values.Value; const BuiltinError = pathname.BuiltinError; const ExecCtx = pathname.ExecCtx; +/// Hold a symlink target to the same keg/prefix boundary a write gets. +/// An absolute target is checked as-is; a relative one is resolved against the +/// link's parent directory (POSIX semantics) before checking, so `../lib/x` +/// from `/bin/y` is judged as `/lib/x` rather than rejected outright +/// for containing a `..`. +fn validateLinkTarget(ctx: ExecCtx, target: []const u8, link_path: []const u8) BuiltinError!void { + const parent = std.fs.path.dirname(link_path) orelse "/"; + const resolved = std.fs.path.resolve(ctx.allocator, &.{ parent, target }) catch + return BuiltinError.OutOfMemory; + // Scratch only — the check consumes it, nothing downstream holds it. + defer ctx.allocator.free(resolved); + sandbox.validatePath(resolved, ctx.cellar_path, ctx.malt_prefix) catch + return BuiltinError.PathSandboxViolation; +} + /// rm — remove a file or array of files pub fn rm(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { if (args.len == 0) return Value{ .nil = {} }; @@ -45,7 +60,12 @@ pub fn rm(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { pub fn rmR(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { if (args.len == 0) return Value{ .nil = {} }; const path = try args[0].asString(ctx.allocator); - sandbox.validatePath(path, ctx.cellar_path, ctx.malt_prefix) catch + // Resolve the parent, not the leaf: `deleteTree` unlinks a symlinked leaf + // without following it (so an in-keg alias stays removable), but it *does* + // traverse intermediate components — which is how a planted directory + // symlink turned this into an out-of-keg recursive delete. Same guard + // `cp`/`cp_r` already use for their destination. + sandbox.validateWriteDir(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; std.Io.Dir.cwd().deleteTree(ctx.io, path) catch {}; return Value{ .nil = {} }; @@ -60,7 +80,9 @@ pub fn rmRf(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { pub fn mkdirP(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { if (args.len == 0) return Value{ .nil = {} }; const path = try args[0].asString(ctx.allocator); - sandbox.validatePath(path, ctx.cellar_path, ctx.malt_prefix) catch + // `createDirPath` follows intermediate symlinks, so the lexical check alone + // let a planted directory link materialise the tree outside the keg. + sandbox.validateWriteDir(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; std.Io.Dir.cwd().createDirPath(ctx.io, path) catch {}; return Value{ .nil = {} }; @@ -199,8 +221,13 @@ pub fn lnS(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { const target = try args[0].asString(ctx.allocator); const link_path = try args[1].asString(ctx.allocator); if (target.len == 0 or link_path.len == 0) return Value{ .nil = {} }; - sandbox.validatePath(link_path, ctx.cellar_path, ctx.malt_prefix) catch + sandbox.validateWriteDir(ctx.io, link_path, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; + // The target was previously unchecked, which let a formula mint a doorway + // out of the keg for any later builtin (or the linker) to walk through. + // POSIX resolves a relative target against the link's own directory, so + // resolve it the same way and hold it to the same boundary as a write. + try validateLinkTarget(ctx, target, link_path); if (std.fs.path.dirname(link_path)) |parent| { std.Io.Dir.cwd().createDirPath(ctx.io, parent) catch {}; @@ -335,6 +362,135 @@ test "cp refuses to copy through an intermediate-directory symlink out of the ke try std.testing.expectError(error.FileNotFound, std.Io.Dir.cwd().access(io, escaped, .{})); } +test "ln_s refuses a target that points out of the keg" { + const io = std.Options.debug_io; + const alloc = std.testing.allocator; + var s = try Scratch.init("fileutils_lns_target"); + defer s.deinit(); + const base = s.base; + + const keg = try std.fs.path.join(alloc, &.{ base, "keg" }); + defer alloc.free(keg); + const outside = try std.fs.path.join(alloc, &.{ base, "outside" }); + defer alloc.free(outside); + const link = try std.fs.path.join(alloc, &.{ keg, "d" }); + defer alloc.free(link); + + try std.Io.Dir.cwd().createDirPath(io, keg); + try std.Io.Dir.cwd().createDirPath(io, outside); + + // Only the link path was screened before; an unscreened target turns the + // keg into a doorway that later builtins traverse. + const ctx: ExecCtx = .{ .allocator = alloc, .io = io, .environ = undefined, .cellar_path = keg, .malt_prefix = keg }; + try std.testing.expectError( + BuiltinError.PathSandboxViolation, + lnS(ctx, null, &.{ Value{ .string = outside }, Value{ .string = link } }), + ); + try std.testing.expectError(error.FileNotFound, std.Io.Dir.cwd().access(io, link, .{})); +} + +test "ln_s still links to a target inside the keg" { + const io = std.Options.debug_io; + const alloc = std.testing.allocator; + var s = try Scratch.init("fileutils_lns_ok"); + defer s.deinit(); + const keg = s.base; + + const target = try std.fs.path.join(alloc, &.{ keg, "real.txt" }); + defer alloc.free(target); + const link = try std.fs.path.join(alloc, &.{ keg, "alias" }); + defer alloc.free(link); + { + const f = try std.Io.Dir.createFileAbsolute(io, target, .{}); + defer f.close(io); + try f.writeStreamingAll(io, "x"); + } + + const ctx: ExecCtx = .{ .allocator = alloc, .io = io, .environ = undefined, .cellar_path = keg, .malt_prefix = keg }; + _ = try lnS(ctx, null, &.{ Value{ .string = target }, Value{ .string = link } }); + try std.Io.Dir.cwd().access(io, link, .{}); +} + +test "rm_r refuses to delete through an intermediate-directory symlink out of the keg" { + const io = std.Options.debug_io; + const alloc = std.testing.allocator; + var s = try Scratch.init("fileutils_rmr_dirlink"); + defer s.deinit(); + const base = s.base; + + const keg = try std.fs.path.join(alloc, &.{ base, "keg" }); + defer alloc.free(keg); + const outside = try std.fs.path.join(alloc, &.{ base, "outside" }); + defer alloc.free(outside); + const victim = try std.fs.path.join(alloc, &.{ outside, "precious" }); + defer alloc.free(victim); + const dirlink = try std.fs.path.join(alloc, &.{ keg, "d" }); + defer alloc.free(dirlink); + const through = try std.fs.path.join(alloc, &.{ dirlink, "precious" }); + defer alloc.free(through); + + try std.Io.Dir.cwd().createDirPath(io, keg); + try std.Io.Dir.cwd().createDirPath(io, victim); + // Planted directly: `ln_s` now refuses this, but a symlink can also arrive + // inside the bottle itself, so the delete path must stand on its own. + try std.Io.Dir.symLinkAbsolute(io, outside, dirlink, .{}); + + // `cp`/`cp_r` already resolve intermediate symlinks; `rm_r` was still + // lexical, so the same planted link reached outside the boundary. + const ctx: ExecCtx = .{ .allocator = alloc, .io = io, .environ = undefined, .cellar_path = keg, .malt_prefix = keg }; + try std.testing.expectError( + BuiltinError.PathSandboxViolation, + rmR(ctx, null, &.{Value{ .string = through }}), + ); + try std.Io.Dir.cwd().access(io, victim, .{}); +} + +test "rm_r still deletes a tree inside the keg" { + const io = std.Options.debug_io; + const alloc = std.testing.allocator; + var s = try Scratch.init("fileutils_rmr_ok"); + defer s.deinit(); + const keg = s.base; + + const doomed = try std.fs.path.join(alloc, &.{ keg, "build" }); + defer alloc.free(doomed); + try std.Io.Dir.cwd().createDirPath(io, doomed); + + const ctx: ExecCtx = .{ .allocator = alloc, .io = io, .environ = undefined, .cellar_path = keg, .malt_prefix = keg }; + _ = try rmR(ctx, null, &.{Value{ .string = doomed }}); + try std.testing.expectError(error.FileNotFound, std.Io.Dir.cwd().access(io, doomed, .{})); +} + +test "mkdir_p refuses to create through an intermediate-directory symlink out of the keg" { + const io = std.Options.debug_io; + const alloc = std.testing.allocator; + var s = try Scratch.init("fileutils_mkdirp_dirlink"); + defer s.deinit(); + const base = s.base; + + const keg = try std.fs.path.join(alloc, &.{ base, "keg" }); + defer alloc.free(keg); + const outside = try std.fs.path.join(alloc, &.{ base, "outside" }); + defer alloc.free(outside); + const dirlink = try std.fs.path.join(alloc, &.{ keg, "d" }); + defer alloc.free(dirlink); + const through = try std.fs.path.join(alloc, &.{ dirlink, "made" }); + defer alloc.free(through); + const escaped = try std.fs.path.join(alloc, &.{ outside, "made" }); + defer alloc.free(escaped); + + try std.Io.Dir.cwd().createDirPath(io, keg); + try std.Io.Dir.cwd().createDirPath(io, outside); + try std.Io.Dir.symLinkAbsolute(io, outside, dirlink, .{}); + + const ctx: ExecCtx = .{ .allocator = alloc, .io = io, .environ = undefined, .cellar_path = keg, .malt_prefix = keg }; + try std.testing.expectError( + BuiltinError.PathSandboxViolation, + mkdirP(ctx, null, &.{Value{ .string = through }}), + ); + try std.testing.expectError(error.FileNotFound, std.Io.Dir.cwd().access(io, escaped, .{})); +} + test "touch refuses to create through a symlink out of the keg" { const io = std.Options.debug_io; const alloc = std.testing.allocator; diff --git a/src/core/dsl/builtins/process.zig b/src/core/dsl/builtins/process.zig index 1b4d4aa8..b5894ca0 100644 --- a/src/core/dsl/builtins/process.zig +++ b/src/core/dsl/builtins/process.zig @@ -240,8 +240,17 @@ pub fn safePopenRead(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError! const argv_slice = try buildArgv(ctx, args); if (argv_slice.len == 0) return Value{ .string = "" }; + // Same two gates as `system`: this is the identical DSL surface, so + // capturing stdout must not be a way to spawn unconfined. + sandbox.validateArgv(argv_slice, ctx.cellar_path, ctx.malt_prefix) catch + return BuiltinError.PathSandboxViolation; + const spawn_argv = sandbox.fenceArgv(ctx.allocator, argv_slice, ctx.cellar_path, ctx.malt_prefix, .{}) catch |e| switch (e) { + error.OutOfMemory => return BuiltinError.OutOfMemory, + error.PathSandboxViolation => return BuiltinError.PathSandboxViolation, + }; + var child = std.process.spawn(ctx.io, .{ - .argv = argv_slice, + .argv = spawn_argv, .stdout = .pipe, .stderr = .ignore, }) catch return Value{ .string = "" }; @@ -491,6 +500,66 @@ test "system and safe_popen_read keep their distinct empty-args returns" { try std.testing.expect(popen == .string and popen.string.len == 0); } +// `safe_popen_read` is the same DSL surface as `system` — reachable from any +// formula's post_install — so it must clear the same argv0 gate. Without it the +// builtin is an unfenced-exec hole straight through the sandbox. +test "safe_popen_read rejects an argv0 outside the sandbox roots before spawning" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + var lio: std.Io.Threaded = .init(arena.allocator(), .{}); + defer lio.deinit(); + const ctx = ExecCtx{ + .allocator = arena.allocator(), + .io = lio.io(), + .environ = undefined, + .cellar_path = "/opt/malt/Cellar/foo/1.0", + .malt_prefix = "/opt/malt", + }; + try std.testing.expectError( + BuiltinError.PathSandboxViolation, + safePopenRead(ctx, null, &.{.{ .string = "/Users/me/evil" }}), + ); +} + +// The argv0 lint waves bare/system-dir commands through, so the sandbox-exec +// fence is what actually contains a write the *arguments* aim outside the keg. +// `system` is fenced; `safe_popen_read` must be too, or a formula can reach any +// path the user can write by picking the capturing builtin instead. +test "safe_popen_read runs under the sandbox-exec write fence" { + 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 base = try std.fmt.allocPrint(alloc, "/tmp/malt_popen_fence_{d}", .{std.c.getpid()}); + std.Io.Dir.cwd().deleteTree(lio.io(), base) catch {}; + defer std.Io.Dir.cwd().deleteTree(lio.io(), base) catch {}; + const keg = try std.fmt.allocPrint(alloc, "{s}/Cellar/foo/1.0", .{base}); + try std.Io.Dir.cwd().createDirPath(lio.io(), keg); + + const ctx = ExecCtx{ + .allocator = alloc, + .io = lio.io(), + .environ = .empty, + .cellar_path = keg, + .malt_prefix = keg, + }; + + // `/usr/bin/touch` clears the argv0 lint (system dir), so only the fence + // can stop the write landing outside the keg. + const outside = try std.fmt.allocPrint(alloc, "{s}/ESCAPED", .{base}); + _ = try safePopenRead(ctx, null, &.{ + .{ .string = "/usr/bin/touch" }, + .{ .string = outside }, + }); + try std.testing.expectError( + error.FileNotFound, + std.Io.Dir.accessAbsolute(lio.io(), outside, .{}), + ); +} + test "system rejects an argv0 outside the sandbox roots before spawning" { var arena = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena.deinit(); diff --git a/src/fs/path_component.zig b/src/fs/path_component.zig index 815b90cf..fe43f30a 100644 --- a/src/fs/path_component.zig +++ b/src/fs/path_component.zig @@ -16,6 +16,44 @@ pub fn isPathComponent(s: []const u8) bool { return true; } +/// True when `s` is a usable *relative subpath* — one or more components that +/// stay inside the directory it is resolved against. Unlike `isPathComponent` +/// it tolerates `/`, because some tap-controlled strings legitimately nest +/// (`binary "bin/tool"`, `app "Sub/My App.app"`). It still bars what hops out: +/// empty, absolute, a `..` component, an empty component (`a//b`), or a NUL. +/// +/// `..` is rejected component-wise, not as a substring — a name like +/// `foo..bar` is a real filename and must survive. +pub fn isRelativeSubpath(s: []const u8) bool { + if (s.len == 0) return false; + if (s[0] == '/') return false; + if (std.mem.indexOfScalar(u8, s, 0) != null) return false; + var it = std.mem.splitScalar(u8, s, '/'); + while (it.next()) |comp| { + if (comp.len == 0) return false; // leading, trailing, or doubled '/' + if (std.mem.eql(u8, comp, "..")) return false; + } + return true; +} + +test "isRelativeSubpath rejects shapes that leave the base directory" { + const bad = [_][]const u8{ + "", "/abs/path", "..", "../x", + "a/../../b", "a/b/..", "a//b", "a/", + "/", "a\x00b", "../", "sub/../../../etc", + }; + for (bad) |s| try std.testing.expect(!isRelativeSubpath(s)); +} + +test "isRelativeSubpath accepts the nested names real casks ship" { + const ok = [_][]const u8{ + "Firefox.app", "Sub Dir/My App.app", "bin/tool", + "a..b", "foo..bar/baz", "codex-aarch64-apple-darwin", + "share/man/man1/x.1", + }; + for (ok) |s| try std.testing.expect(isRelativeSubpath(s)); +} + test "isPathComponent rejects component-hopping shapes" { const bad = [_][]const u8{ "", ".", "..", "a/b", "../evil", "foo/", "a..b", "1..0", "a\x00b" }; for (bad) |s| try std.testing.expect(!isPathComponent(s)); diff --git a/src/net/client.zig b/src/net/client.zig index 6666a9f8..fd55955e 100644 --- a/src/net/client.zig +++ b/src/net/client.zig @@ -376,7 +376,11 @@ pub const HttpClient = struct { /// `https://evil.tld/github.com` never receives the token. Hosts are /// compared case-insensitively (DNS is case-insensitive), so a legit /// host never loses the token to capitalisation. - fn githubTokenApplies(url: []const u8) bool { + /// Public so the host gate can be pinned without a live request — the + /// credential-injection branch used to be covered by actually calling + /// `formulae.brew.sh`, which sent whatever token the developer had + /// exported to a third party. + pub fn githubTokenApplies(url: []const u8) bool { const host = urlHost(url) orelse return false; if (std.ascii.eqlIgnoreCase(host, "github.com")) return true; if (std.ascii.endsWithIgnoreCase(host, ".github.com")) return true; @@ -1397,6 +1401,41 @@ fn environFrom(entries: [:null]const ?[*:0]const u8) std.process.Environ { return .{ .block = .{ .slice = entries } }; } +test "githubTokenApplies: only the four credential-bearing hosts match" { + const yes = [_][]const u8{ + "https://github.com/indaco/malt", + "https://api.github.com/repos/x/y", + "https://formulae.brew.sh/api/formula/jq.json", + "https://ghcr.io/v2/homebrew/core/jq/blobs/sha256:ab", + "https://GitHub.com/x", // the host comparison is case-insensitive + "https://github.com./x", // FQDN root form of the same host + "https://user@github.com/x", // userinfo is stripped before matching + "https://github.com:443/x", // as is the port + }; + for (yes) |u| try std.testing.expect(HttpClient.githubTokenApplies(u)); +} + +test "githubTokenApplies: a look-alike host or a path lookalike never gets the token" { + // The gate is a host-component match precisely so these cannot collect a + // credential; pin that rather than trusting the comment. + const no = [_][]const u8{ + "https://github.com.evil.tld/x", + "https://evil.tld/github.com", + "https://notgithub.com/x", + "https://formulae.brew.sh.evil.tld/x", + "https://ghcr.io.evil.tld/x", + "https://evil.tld/?u=https://ghcr.io/", + "https://xghcr.io/x", + "not-a-url", + "", + // The scheme is matched case-sensitively, so this yields no host at + // all. That is fail-closed (no credential leaves), so it is pinned + // here as the current, safe behaviour rather than treated as a match. + "HTTPS://github.com/x", + }; + for (no) |u| try std.testing.expect(!HttpClient.githubTokenApplies(u)); +} + test "githubApiToken: MALT_GITHUB_TOKEN is preferred over HOMEBREW_GITHUB_API_TOKEN" { const entries = [_:null]?[*:0]const u8{ "MALT_GITHUB_TOKEN=malt-tok".ptr, diff --git a/tests/cask_test.zig b/tests/cask_test.zig index 5fbed02e..462e3de4 100644 --- a/tests/cask_test.zig +++ b/tests/cask_test.zig @@ -644,17 +644,23 @@ test "verifyFileSha256 rejects a truncated or over-length digest" { ); } -test "verifyFileSha256 skips verification on null or :no_check" { +test "verifyFileSha256 skips verification only on an explicit :no_check" { // `sha256 :no_check` is Homebrew's opt-out for self-updating // installers. Honouring the skip prevents spurious failures on // perfectly valid casks. + // + // An *absent* hash is not that opt-out and no longer shares its path. + // Homebrew always emits `sha256` for a cask, so null means a malformed or + // hostile manifest; accepting it silently downgraded those casks to + // transport-only integrity, and a `pkg` artifact is then handed to + // `sudo installer -target /`. var path_buf: [128]u8 = undefined; const p = try tempFilePath("skip", &path_buf); try writeFile(p, "anything"); defer malt_fs.cwd().deleteFile(std.Options.debug_io, p) catch {}; - try cask.verifyFileSha256(testIo(), p, null); try cask.verifyFileSha256(testIo(), p, "no_check"); + try testing.expectError(error.Sha256Missing, cask.verifyFileSha256(testIo(), p, null)); } test "verifyFileSha256 accepts a correct digest on a >1 MiB file" { diff --git a/tests/progress_e2e_test.zig b/tests/progress_e2e_test.zig index 725def37..ed341c3e 100644 --- a/tests/progress_e2e_test.zig +++ b/tests/progress_e2e_test.zig @@ -3,6 +3,7 @@ const std = @import("std"); const testing = std.testing; +const malt = @import("malt"); const client_mod = @import("malt").client; const progress_mod = @import("malt").progress; @@ -25,7 +26,16 @@ const TestTracker = struct { } }; +/// Live-network tests are opt-in — see `tests/progress_test.zig`. Set +/// `MALT_TEST_NETWORK=1` to run them. +fn liveNetworkAllowed() bool { + const v = std.process.Environ.getPosix(malt.app_ctx.processEnviron(), "MALT_TEST_NETWORK") orelse + return false; + return std.mem.eql(u8, v, "1"); +} + test "HTTP GET with progress callback fires correctly" { + if (!liveNetworkAllowed()) return error.SkipZigTest; var http = client_mod.HttpClient.init(std.Options.debug_io, std.process.Environ.empty, testing.allocator); defer http.deinit(); @@ -60,6 +70,7 @@ test "HTTP GET with progress callback fires correctly" { } test "HTTP GET without progress (null) still works" { + if (!liveNetworkAllowed()) return error.SkipZigTest; var http = client_mod.HttpClient.init(std.Options.debug_io, std.process.Environ.empty, testing.allocator); defer http.deinit(); diff --git a/tests/progress_test.zig b/tests/progress_test.zig index 4c5fabd6..c95b476a 100644 --- a/tests/progress_test.zig +++ b/tests/progress_test.zig @@ -426,7 +426,17 @@ test "Response.deinit frees the owned body buffer" { extern fn setenv(name: [*:0]const u8, value: [*:0]const u8, overwrite: c_int) c_int; extern fn unsetenv(name: [*:0]const u8) c_int; +/// Live-network tests are opt-in. They reach third-party hosts, which makes the +/// suite non-hermetic, dependent on someone else's uptime, and noisy on a +/// firewalled or sandboxed machine. Set `MALT_TEST_NETWORK=1` to run them. +fn liveNetworkAllowed() bool { + const v = std.process.Environ.getPosix(malt.app_ctx.processEnviron(), "MALT_TEST_NETWORK") orelse + return false; + return std.mem.eql(u8, v, "1"); +} + test "HttpClient.get retries on a 503 response status" { + if (!liveNetworkAllowed()) return error.SkipZigTest; // httpbin.org/status/503 returns a deterministic 503 which is one of // the retry-eligible codes (429/503/504). We don't care about the // final result — we just need kcov to see the retry branch execute. @@ -461,22 +471,22 @@ test "HttpClient.get retries on connection failure before giving up" { } else |_| {} } -test "HttpClient.get injects auth header when HOMEBREW_GITHUB_API_TOKEN is set" { - // Force the env var to a sentinel for the duration of this test — - // both formulae.brew.sh and ghcr.io URLs pick up the token branch, - // which is the 4-line block (lines 54-61) in client.zig. - _ = setenv("HOMEBREW_GITHUB_API_TOKEN", "fake-testing-token", 1); - defer _ = unsetenv("HOMEBREW_GITHUB_API_TOKEN"); - - var http = client_mod.HttpClient.init(std.Options.debug_io, malt.app_ctx.processEnviron(), testing.allocator); - defer http.deinit(); - - // The URL matches the formulae.brew.sh guard so the header path runs. - // We then hit an invalid subpath; any non-2xx response is fine — we - // only care that the token-branch code compiled and executed. - var resp = http.get("https://formulae.brew.sh/api/formula/jq.json") catch |err| { - std.debug.print("Skipping token header test (network error: {s})\n", .{@errorName(err)}); - return; - }; - defer resp.deinit(); +test "HttpClient auth header: token resolution and host gate, without a request" { + // This used to build the client from the *process* environment and issue a + // real GET to formulae.brew.sh. `githubApiToken` prefers MALT_GITHUB_TOKEN + // over the HOMEBREW_ var this test sets, so on a machine with the former + // exported the suite mailed a live credential to a third-party host. + // + // Both halves of the injection decision are pure, so assert them directly + // against a synthetic environ and never open a socket. + const entries = [_:null]?[*:0]const u8{"HOMEBREW_GITHUB_API_TOKEN=fake-testing-token".ptr}; + const environ: std.process.Environ = .{ .block = .{ .slice = &entries } }; + + try testing.expectEqualStrings( + "fake-testing-token", + client_mod.HttpClient.githubApiToken(environ).?, + ); + // …and it is only ever offered to the hosts the gate names. + try testing.expect(client_mod.HttpClient.githubTokenApplies("https://formulae.brew.sh/api/formula/jq.json")); + try testing.expect(!client_mod.HttpClient.githubTokenApplies("https://example.com/formulae.brew.sh")); }