From d09004a5221a464d16dd34c7b95f519c4c460275 Mon Sep 17 00:00:00 2001 From: rustytrees Date: Wed, 12 Aug 2026 16:47:01 -0400 Subject: [PATCH] fix(dsl): confine mutation path variants Apply resolved boundary checks to Pathname and FileUtils mutation variants that only used lexical checks. Validate symlink destinations and targets for scalar, array, and hash forms so formula steps cannot mutate files outside the prefix through planted links. --- src/core/dsl/builtins/fileutils.zig | 56 ++++----- src/core/dsl/builtins/pathname.zig | 18 ++- src/core/dsl/sandbox.zig | 18 +++ tests/dsl_builtins_test.zig | 189 ++++++++++++++++++++++++++++ 4 files changed, 244 insertions(+), 37 deletions(-) diff --git a/src/core/dsl/builtins/fileutils.zig b/src/core/dsl/builtins/fileutils.zig index 5ff89e72..6f8825e5 100644 --- a/src/core/dsl/builtins/fileutils.zig +++ b/src/core/dsl/builtins/fileutils.zig @@ -18,24 +18,6 @@ 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); - // Resolve the target's own parent chain, not just its spelling: a bottle - // can ship a directory symlink that a lexically in-keg target walks - // through. Same guard `rm_r`/`mkdir_p` use. - sandbox.validateWriteDir(ctx.io, 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,13 +27,13 @@ pub fn rm(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { .array => |items| { for (items) |item| { const path = item.asString(ctx.allocator) catch continue; - sandbox.validatePath(path, ctx.cellar_path, ctx.malt_prefix) catch continue; + sandbox.validateWriteDir(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch continue; std.Io.Dir.cwd().deleteFile(ctx.io, path) catch {}; } }, else => { const path = try args[0].asString(ctx.allocator); - sandbox.validatePath(path, ctx.cellar_path, ctx.malt_prefix) catch + sandbox.validateWriteDir(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; std.Io.Dir.cwd().deleteFile(ctx.io, path) catch {}; }, @@ -230,7 +212,10 @@ pub fn lnS(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { // 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); + sandbox.validateLinkTarget(ctx.allocator, ctx.io, target, link_path, ctx.cellar_path, ctx.malt_prefix) catch |e| switch (e) { + error.OutOfMemory => return BuiltinError.OutOfMemory, + error.PathSandboxViolation => return BuiltinError.PathSandboxViolation, + }; if (std.fs.path.dirname(link_path)) |parent| { std.Io.Dir.cwd().createDirPath(ctx.io, parent) catch {}; @@ -248,33 +233,40 @@ pub fn lnSf(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Value { switch (args[0]) { .array => |items| { const dest_dir = args[1].asString(ctx.allocator) catch return Value{ .nil = {} }; - sandbox.validatePath(dest_dir, ctx.cellar_path, ctx.malt_prefix) catch + sandbox.validateDirTarget(ctx.io, dest_dir, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; std.Io.Dir.cwd().createDirPath(ctx.io, dest_dir) catch {}; for (items) |item| { const target = item.asString(ctx.allocator) catch continue; const base = std.fs.path.basename(target); const link_path = std.fs.path.join(ctx.allocator, &.{ dest_dir, base }) catch continue; - std.Io.Dir.cwd().deleteFile(ctx.io, link_path) catch {}; - std.Io.Dir.symLinkAbsolute(ctx.io, target, link_path, .{}) catch {}; + defer ctx.allocator.free(link_path); + try forceSymlink(ctx, target, link_path); } }, else => { const target = try args[0].asString(ctx.allocator); const link_path = try args[1].asString(ctx.allocator); - sandbox.validatePath(link_path, ctx.cellar_path, ctx.malt_prefix) catch - return BuiltinError.PathSandboxViolation; - - if (std.fs.path.dirname(link_path)) |parent| { - std.Io.Dir.cwd().createDirPath(ctx.io, parent) catch {}; - } - std.Io.Dir.cwd().deleteFile(ctx.io, link_path) catch {}; - std.Io.Dir.symLinkAbsolute(ctx.io, target, link_path, .{}) catch {}; + try forceSymlink(ctx, target, link_path); }, } return Value{ .nil = {} }; } +fn forceSymlink(ctx: ExecCtx, target: []const u8, link_path: []const u8) BuiltinError!void { + sandbox.validateWriteDir(ctx.io, link_path, ctx.cellar_path, ctx.malt_prefix) catch + return BuiltinError.PathSandboxViolation; + sandbox.validateLinkTarget(ctx.allocator, ctx.io, target, link_path, ctx.cellar_path, ctx.malt_prefix) catch |e| switch (e) { + error.OutOfMemory => return BuiltinError.OutOfMemory, + error.PathSandboxViolation => return BuiltinError.PathSandboxViolation, + }; + if (std.fs.path.dirname(link_path)) |parent| { + std.Io.Dir.cwd().createDirPath(ctx.io, parent) catch {}; + } + std.Io.Dir.cwd().deleteFile(ctx.io, link_path) catch {}; + std.Io.Dir.symLinkAbsolute(ctx.io, target, link_path, .{}) catch {}; +} + const fs_test_io = std.Options.debug_io; var scratch_seq: std.atomic.Value(u32) = .init(0); diff --git a/src/core/dsl/builtins/pathname.zig b/src/core/dsl/builtins/pathname.zig index 911933ad..8180f61f 100644 --- a/src/core/dsl/builtins/pathname.zig +++ b/src/core/dsl/builtins/pathname.zig @@ -44,6 +44,8 @@ pub const ExecCtx = struct { pub fn mkpath(ctx: ExecCtx, receiver: ?Value, _: []const Value) BuiltinError!Value { const path = try receiverPath(ctx.allocator, receiver); if (path.len == 0) return Value{ .nil = {} }; + sandbox.validateDirTarget(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 = {} }; } @@ -213,7 +215,7 @@ pub fn pkgetc(ctx: ExecCtx, receiver: ?Value, _: []const Value) BuiltinError!Val pub fn unlink(ctx: ExecCtx, receiver: ?Value, _: []const Value) BuiltinError!Value { const path = try receiverPath(ctx.allocator, receiver); if (path.len == 0) return Value{ .nil = {} }; - sandbox.validatePath(path, ctx.cellar_path, ctx.malt_prefix) catch + sandbox.validateWriteDir(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; std.Io.Dir.cwd().deleteFile(ctx.io, path) catch {}; return Value{ .nil = {} }; @@ -228,6 +230,9 @@ pub fn unlink(ctx: ExecCtx, receiver: ?Value, _: []const Value) BuiltinError!Val pub fn installSymlink(ctx: ExecCtx, receiver: ?Value, args: []const Value) BuiltinError!Value { const dir = try receiverPath(ctx.allocator, receiver); if (dir.len == 0) return Value{ .nil = {} }; + sandbox.validateDirTarget(ctx.io, dir, ctx.cellar_path, ctx.malt_prefix) catch + return BuiltinError.PathSandboxViolation; + std.Io.Dir.cwd().createDirPath(ctx.io, dir) catch {}; for (args) |arg| try installSymlinkArg(ctx, dir, arg); return Value{ .nil = {} }; } @@ -251,8 +256,8 @@ fn installSymlinkArg(ctx: ExecCtx, dir: []const u8, arg: Value) BuiltinError!voi } } -/// Symlink `/` → ``, sandbox-validated on the link -/// path. Same non-raising fs contract as the rest of this module: a +/// Symlink `/` → ``, sandbox-validated on both paths. +/// Same non-raising fs contract as the rest of this module: a /// failed mkdir/symlink surfaces downstream, not here. fn linkInto(ctx: ExecCtx, dir: []const u8, source: []const u8, name: []const u8) BuiltinError!void { if (source.len == 0 or name.len == 0) return; @@ -261,10 +266,13 @@ fn linkInto(ctx: ExecCtx, dir: []const u8, source: []const u8, name: []const u8) // Overflow means a pathological path — skip rather than truncate. const link = std.fmt.bufPrint(&link_buf, "{s}/{s}", .{ dir, name }) catch return; - sandbox.validatePath(link, ctx.cellar_path, ctx.malt_prefix) catch + sandbox.validateWriteDir(ctx.io, link, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; + sandbox.validateLinkTarget(ctx.allocator, ctx.io, source, link, ctx.cellar_path, ctx.malt_prefix) catch |e| switch (e) { + error.OutOfMemory => return BuiltinError.OutOfMemory, + error.PathSandboxViolation => return BuiltinError.PathSandboxViolation, + }; - std.Io.Dir.cwd().createDirPath(ctx.io, dir) catch {}; std.Io.Dir.cwd().deleteFile(ctx.io, link) catch {}; std.Io.Dir.symLinkAbsolute(ctx.io, source, link, .{}) catch {}; } diff --git a/src/core/dsl/sandbox.zig b/src/core/dsl/sandbox.zig index 5ae11108..398ce440 100644 --- a/src/core/dsl/sandbox.zig +++ b/src/core/dsl/sandbox.zig @@ -190,6 +190,24 @@ pub fn validateDirTarget( try resolvedDirWithinBoundary(io, dir_path, cellar_path, malt_prefix); } +/// Validate the object a new symlink will resolve to. Relative targets use the +/// link's parent directory, matching POSIX symlink semantics. Resolving the +/// target itself, including an existing final component, prevents a link from +/// becoming a doorway through another symlink that already leaves the prefix. +pub fn validateLinkTarget( + allocator: std.mem.Allocator, + io: std.Io, + target: []const u8, + link_path: []const u8, + cellar_path: []const u8, + malt_prefix: []const u8, +) (SandboxError || std.mem.Allocator.Error)!void { + const parent = std.fs.path.dirname(link_path) orelse "/"; + const resolved = try std.fs.path.resolve(allocator, &.{ parent, target }); + defer allocator.free(resolved); + try validateDirTarget(io, resolved, cellar_path, malt_prefix); +} + /// How `openTargetNoFollow` opens the leaf. `write` toggles WRONLY vs RDONLY /// (chmod only needs a handle to `fchmod`); `create`/`truncate` map to /// `O_CREAT`/`O_TRUNC`. diff --git a/tests/dsl_builtins_test.zig b/tests/dsl_builtins_test.zig index cc74a9f8..44f03f00 100644 --- a/tests/dsl_builtins_test.zig +++ b/tests/dsl_builtins_test.zig @@ -81,6 +81,23 @@ test "Pathname.mkpath creates a directory tree under the receiver" { d.close(std.Options.debug_io); } +test "Pathname.mkpath refuses a directory outside the sandbox" { + const root = try uniqueSandbox("mkpath_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("mkpath_escape_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + const escaped = try std.fmt.allocPrint(testing.allocator, "{s}/created", .{outside}); + defer testing.allocator.free(escaped); + + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.mkpath(mkCtx(root), Value{ .pathname = escaped }, &.{}), + ); + try testing.expectError(error.FileNotFound, test_io.accessAbsolute(std.Options.debug_io, escaped, .{})); +} + test "Pathname.exist?, .directory?, .file?, .symlink? classify entries correctly" { const root = try uniqueSandbox("classify"); defer testing.allocator.free(root); @@ -221,6 +238,30 @@ test "Pathname.unlink deletes a file in the sandbox" { try testing.expectError(error.FileNotFound, test_io.openFileAbsolute(std.Options.debug_io, path, .{})); } +test "Pathname.unlink refuses an intermediate symlink outside the sandbox" { + const root = try uniqueSandbox("unlink_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("unlink_escape_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + + const victim = try std.fmt.allocPrint(testing.allocator, "{s}/victim", .{outside}); + defer testing.allocator.free(victim); + (try test_io.createFileAbsolute(std.Options.debug_io, victim, .{})).close(std.Options.debug_io); + const doorway = try std.fmt.allocPrint(testing.allocator, "{s}/door", .{root}); + defer testing.allocator.free(doorway); + try test_io.symLinkAbsolute(std.Options.debug_io, outside, doorway, .{}); + const through = try std.fmt.allocPrint(testing.allocator, "{s}/victim", .{doorway}); + defer testing.allocator.free(through); + + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.unlink(mkCtx(root), Value{ .pathname = through }, &.{}), + ); + try test_io.accessAbsolute(std.Options.debug_io, victim, .{}); +} + test "Pathname.install_symlink (positional) links / -> source" { // Homebrew semantics: receiver is the target directory, arg[0] is the // source. The link lands at /. @@ -294,6 +335,59 @@ test "Pathname.install_symlink (array) links each source by basename" { } } +test "Pathname.install_symlink refuses scalar, array, and hash targets outside the sandbox" { + const root = try uniqueSandbox("install_symlink_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("install_symlink_escape_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + const target = try std.fmt.allocPrint(testing.allocator, "{s}/private", .{outside}); + defer testing.allocator.free(target); + (try test_io.createFileAbsolute(std.Options.debug_io, target, .{})).close(std.Options.debug_io); + const dir = try std.fmt.allocPrint(testing.allocator, "{s}/bin", .{root}); + defer testing.allocator.free(dir); + const ctx = mkCtx(root); + + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.installSymlink(ctx, Value{ .pathname = dir }, &.{Value{ .string = target }}), + ); + const items = [_]Value{Value{ .string = target }}; + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.installSymlink(ctx, Value{ .pathname = dir }, &.{Value{ .array = &items }}), + ); + const pairs = [_]Value.HashPair{.{ .key = Value{ .string = target }, .value = Value{ .string = "alias" } }}; + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.installSymlink(ctx, Value{ .pathname = dir }, &.{Value{ .hash = &pairs }}), + ); +} + +test "Pathname.install_symlink refuses a destination symlink outside the sandbox" { + const root = try uniqueSandbox("install_symlink_dest_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("install_symlink_dest_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + const target = try std.fmt.allocPrint(testing.allocator, "{s}/target", .{root}); + defer testing.allocator.free(target); + (try test_io.createFileAbsolute(std.Options.debug_io, target, .{})).close(std.Options.debug_io); + const doorway = try std.fmt.allocPrint(testing.allocator, "{s}/door", .{root}); + defer testing.allocator.free(doorway); + try test_io.symLinkAbsolute(std.Options.debug_io, outside, doorway, .{}); + const escaped = try std.fmt.allocPrint(testing.allocator, "{s}/target", .{outside}); + defer testing.allocator.free(escaped); + + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.installSymlink(mkCtx(root), Value{ .pathname = doorway }, &.{Value{ .string = target }}), + ); + try testing.expectError(error.FileNotFound, test_io.accessAbsolute(std.Options.debug_io, escaped, .{})); +} + test "Pathname.install_symlink replaces an existing link at the target name" { const root = try uniqueSandbox("install_symlink_replace"); defer testing.allocator.free(root); @@ -701,6 +795,57 @@ test "FileUtils.ln_s/ln_sf create (and force-replace) symlinks" { try testing.expectEqualStrings(target, try test_io.readLinkAbsolute(std.Options.debug_io, link, &buf)); } +test "FileUtils.ln_sf scalar refuses an intermediate symlink outside the sandbox" { + const root = try uniqueSandbox("lnsf_scalar_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("lnsf_scalar_escape_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + + const target = try std.fmt.allocPrint(testing.allocator, "{s}/target", .{root}); + defer testing.allocator.free(target); + (try test_io.createFileAbsolute(std.Options.debug_io, target, .{})).close(std.Options.debug_io); + const doorway = try std.fmt.allocPrint(testing.allocator, "{s}/door", .{root}); + defer testing.allocator.free(doorway); + try test_io.symLinkAbsolute(std.Options.debug_io, outside, doorway, .{}); + const through = try std.fmt.allocPrint(testing.allocator, "{s}/escaped", .{doorway}); + defer testing.allocator.free(through); + const escaped = try std.fmt.allocPrint(testing.allocator, "{s}/escaped", .{outside}); + defer testing.allocator.free(escaped); + + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + fileutils.lnSf(mkCtx(root), null, &.{ Value{ .string = target }, Value{ .string = through } }), + ); + try testing.expectError(error.FileNotFound, test_io.accessAbsolute(std.Options.debug_io, escaped, .{})); +} + +test "FileUtils.ln_sf array refuses a destination symlink outside the sandbox" { + const root = try uniqueSandbox("lnsf_array_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("lnsf_array_escape_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + + const target = try std.fmt.allocPrint(testing.allocator, "{s}/target", .{root}); + defer testing.allocator.free(target); + (try test_io.createFileAbsolute(std.Options.debug_io, target, .{})).close(std.Options.debug_io); + const doorway = try std.fmt.allocPrint(testing.allocator, "{s}/door", .{root}); + defer testing.allocator.free(doorway); + try test_io.symLinkAbsolute(std.Options.debug_io, outside, doorway, .{}); + const items = [_]Value{Value{ .string = target }}; + const escaped = try std.fmt.allocPrint(testing.allocator, "{s}/target", .{outside}); + defer testing.allocator.free(escaped); + + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + fileutils.lnSf(mkCtx(root), null, &.{ Value{ .array = &items }, Value{ .string = doorway } }), + ); + try testing.expectError(error.FileNotFound, test_io.accessAbsolute(std.Options.debug_io, escaped, .{})); +} + test "FileUtils.rm rejects paths outside the sandbox" { const root = try uniqueSandbox("fileutils_violate"); defer testing.allocator.free(root); @@ -712,6 +857,50 @@ test "FileUtils.rm rejects paths outside the sandbox" { ); } +test "FileUtils.rm scalar refuses an intermediate symlink outside the sandbox" { + const root = try uniqueSandbox("rm_scalar_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("rm_scalar_escape_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + const victim = try std.fmt.allocPrint(testing.allocator, "{s}/victim", .{outside}); + defer testing.allocator.free(victim); + (try test_io.createFileAbsolute(std.Options.debug_io, victim, .{})).close(std.Options.debug_io); + const doorway = try std.fmt.allocPrint(testing.allocator, "{s}/door", .{root}); + defer testing.allocator.free(doorway); + try test_io.symLinkAbsolute(std.Options.debug_io, outside, doorway, .{}); + const through = try std.fmt.allocPrint(testing.allocator, "{s}/victim", .{doorway}); + defer testing.allocator.free(through); + + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + fileutils.rm(mkCtx(root), null, &.{Value{ .string = through }}), + ); + try test_io.accessAbsolute(std.Options.debug_io, victim, .{}); +} + +test "FileUtils.rm array skips an intermediate symlink outside the sandbox" { + const root = try uniqueSandbox("rm_array_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("rm_array_escape_outside"); + defer testing.allocator.free(outside); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, outside) catch {}; + const victim = try std.fmt.allocPrint(testing.allocator, "{s}/victim", .{outside}); + defer testing.allocator.free(victim); + (try test_io.createFileAbsolute(std.Options.debug_io, victim, .{})).close(std.Options.debug_io); + const doorway = try std.fmt.allocPrint(testing.allocator, "{s}/door", .{root}); + defer testing.allocator.free(doorway); + try test_io.symLinkAbsolute(std.Options.debug_io, outside, doorway, .{}); + const through = try std.fmt.allocPrint(testing.allocator, "{s}/victim", .{doorway}); + defer testing.allocator.free(through); + const items = [_]Value{Value{ .string = through }}; + + _ = try fileutils.rm(mkCtx(root), null, &.{Value{ .array = &items }}); + try test_io.accessAbsolute(std.Options.debug_io, victim, .{}); +} + test "FileUtils.chmod returns nil for a non-int mode and is a no-op" { const root = try uniqueSandbox("fileutils_chmod_nil"); defer testing.allocator.free(root);