diff --git a/src/core/dsl/builtins/inreplace.zig b/src/core/dsl/builtins/inreplace.zig index 7177cf53..7d174a40 100644 --- a/src/core/dsl/builtins/inreplace.zig +++ b/src/core/dsl/builtins/inreplace.zig @@ -38,8 +38,14 @@ pub fn inreplace(ctx: ExecCtx, _: ?Value, args: []const Value) BuiltinError!Valu sandbox.validateWriteDir(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch return BuiltinError.PathSandboxViolation; - // Read file contents - const content = read.readFileAllAbsolute(ctx.io, ctx.allocator, path, 4 * 1024 * 1024) catch { + // Open the source itself without following a final symlink. The descriptor + // keeps the read bound to the object that passed the confinement check. + const file = sandbox.openSourceNoFollow(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch |e| switch (e) { + error.PathSandboxViolation => return BuiltinError.PathSandboxViolation, + else => return Value{ .nil = {} }, + }; + defer file.close(ctx.io); + const content = read.readFileAll(ctx.io, ctx.allocator, file, 4 * 1024 * 1024) catch { return Value{ .nil = {} }; }; @@ -146,3 +152,40 @@ test "atomic-write failure message names the failing step" { try std.testing.expect(std.mem.indexOf(u8, rename_msg, "RenameFailed") != null); try std.testing.expect(std.mem.indexOf(u8, rename_msg, "CrossDeviceLink") != null); } + +test "inreplace refuses to read through a source symlink outside the keg" { + const io = std.Options.debug_io; + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + const base = try std.fmt.allocPrint(alloc, "/tmp/malt_inreplace_source_{d}", .{std.c.getpid()}); + std.Io.Dir.cwd().deleteTree(io, base) catch {}; + defer std.Io.Dir.cwd().deleteTree(io, base) catch {}; + + const keg = try std.fmt.allocPrint(alloc, "{s}/keg", .{base}); + const victim = try std.fmt.allocPrint(alloc, "{s}/victim", .{base}); + const link = try std.fmt.allocPrint(alloc, "{s}/config", .{keg}); + try std.Io.Dir.cwd().createDirPath(io, keg); + { + const f = try std.Io.Dir.createFileAbsolute(io, victim, .{}); + defer f.close(io); + try f.writeStreamingAll(io, "PRIVATE"); + } + try std.Io.Dir.symLinkAbsolute(io, victim, link, .{}); + + const ctx: ExecCtx = .{ + .allocator = alloc, + .io = io, + .environ = .empty, + .cellar_path = keg, + .malt_prefix = keg, + }; + try std.testing.expectError( + BuiltinError.PathSandboxViolation, + inreplace(ctx, null, &.{ + Value{ .string = link }, + Value{ .string = "PRIVATE" }, + Value{ .string = "PRIVATE" }, + }), + ); +} diff --git a/src/core/dsl/builtins/pathname.zig b/src/core/dsl/builtins/pathname.zig index 911933ad..861ac916 100644 --- a/src/core/dsl/builtins/pathname.zig +++ b/src/core/dsl/builtins/pathname.zig @@ -102,7 +102,12 @@ pub fn write(ctx: ExecCtx, receiver: ?Value, args: []const Value) BuiltinError!V pub fn read(ctx: ExecCtx, receiver: ?Value, _: []const Value) BuiltinError!Value { const path = try receiverPath(ctx.allocator, receiver); if (path.len == 0) return Value{ .string = "" }; - const content = fs_read.readFileAllAbsolute(ctx.io, ctx.allocator, path, 1024 * 1024) catch { + const file = sandbox.openSourceNoFollow(ctx.io, path, ctx.cellar_path, ctx.malt_prefix) catch |e| switch (e) { + error.PathSandboxViolation => return BuiltinError.PathSandboxViolation, + else => return Value{ .string = "" }, + }; + defer file.close(ctx.io); + const content = fs_read.readFileAll(ctx.io, ctx.allocator, file, 1024 * 1024) catch { return Value{ .string = "" }; }; return Value{ .string = content }; diff --git a/src/core/dsl/sandbox.zig b/src/core/dsl/sandbox.zig index 5ae11108..8166e492 100644 --- a/src/core/dsl/sandbox.zig +++ b/src/core/dsl/sandbox.zig @@ -227,6 +227,18 @@ pub fn openTargetNoFollow( return .{ .handle = fd, .flags = .{ .nonblocking = false } }; } +/// Open a confined source without following its final component. Keeping the +/// returned descriptor open binds subsequent reads to the checked object and +/// closes the leaf-symlink and check-then-open windows. +pub fn openSourceNoFollow( + io: std.Io, + source_path: []const u8, + cellar_path: []const u8, + malt_prefix: []const u8, +) (SandboxError || std.posix.OpenError)!std.Io.File { + return openTargetNoFollow(io, source_path, cellar_path, malt_prefix, .{ .write = false }); +} + const fs_test_io = std.Options.debug_io; var scratch_seq: std.atomic.Value(u32) = .init(0); diff --git a/src/core/post_install_steps.zig b/src/core/post_install_steps.zig index fa8c1cb9..13ebde46 100644 --- a/src/core/post_install_steps.zig +++ b/src/core/post_install_steps.zig @@ -15,6 +15,7 @@ const sandbox_macos = @import("sandbox/macos.zig"); const fallback_log = @import("dsl/fallback_log.zig"); const atomic = @import("../fs/atomic.zig"); const clonefile = @import("../fs/clonefile.zig"); +const fs_read = @import("../fs/read.zig"); const text_replace = @import("../text_replace.zig"); pub const FallbackLog = fallback_log.FallbackLog; @@ -732,25 +733,14 @@ fn stepInreplace(ctx: StepsCtx, obj: std.json.ObjectMap) bool { return false; } - const file = std.Io.Dir.openFileAbsolute(ctx.io, path, .{}) catch return false; - const stat = file.stat(ctx.io) catch { - file.close(ctx.io); - return false; - }; - if (stat.size > 4 * 1024 * 1024) { - file.close(ctx.io); - return false; - } - const content = ctx.allocator.alloc(u8, stat.size) catch { - file.close(ctx.io); - return false; - }; - const read = file.readPositionalAll(ctx.io, content, 0) catch { - file.close(ctx.io); + const file = sandbox.openSourceNoFollow(ctx.io, path, ctx.keg_path, ctx.prefix) catch |e| { + if (e == error.PathSandboxViolation) logViolation(ctx, path); return false; }; - file.close(ctx.io); - if (read < content.len) return false; + defer file.close(ctx.io); + const stat = file.stat(ctx.io) catch return false; + if (stat.size > 4 * 1024 * 1024) return false; + const content = fs_read.readFileAll(ctx.io, ctx.allocator, file, 4 * 1024 * 1024) catch return false; const updated = if (getFlag(obj, "regexp")) blk: { const literal = anchoredLineLiteral(before) orelse { @@ -1482,6 +1472,36 @@ test "execute substitutes a literal inreplace and resolves the invoking user" { try testing.expectEqualStrings("username: \"ada\"\n", try readBack(&h, path)); } +test "execute refuses inreplace through a source symlink outside the prefix" { + var h = try TestHarness.init(); + defer h.deinit(); + const a = h.arena.allocator(); + + var outside = try Scratch.init("steps_inreplace_source"); + defer outside.deinit(); + try std.Io.Dir.cwd().createDirPath(h.io, outside.base); + const victim = outside.p("/victim"); + { + const f = try std.Io.Dir.createFileAbsolute(h.io, victim, .{}); + defer f.close(h.io); + try f.writeStreamingAll(h.io, "PRIVATE"); + } + + const etc = try std.fmt.allocPrint(a, "{s}/etc", .{h.prefix}); + try std.Io.Dir.cwd().createDirPath(h.io, etc); + const link = try std.fmt.allocPrint(a, "{s}/leak.conf", .{etc}); + try std.Io.Dir.symLinkAbsolute(h.io, victim, link, .{}); + + _ = execute(h.ctx(), try testFormulaJson(&h, + \\[{"type":"inreplace","path":{"base":"etc","path":"leak.conf"}, + \\ "before":"PRIVATE","after":"PRIVATE"}] + )); + + try testing.expect(h.flog.hasFatal()); + var target_buf: [std.fs.max_path_bytes]u8 = undefined; + _ = try std.Io.Dir.readLinkAbsolute(h.io, link, &target_buf); +} + test "execute skips an inreplace whose if_exists guard is unmet" { // The guard exists because the target is a user config that may never // have been poured; creating it here would invent configuration. diff --git a/src/fs/read.zig b/src/fs/read.zig index f5c93418..a651f23e 100644 --- a/src/fs/read.zig +++ b/src/fs/read.zig @@ -13,6 +13,13 @@ const std = @import("std"); pub fn readFileAllAbsolute(io: std.Io, allocator: std.mem.Allocator, abs_path: []const u8, max_bytes: usize) ![]u8 { const f = try std.Io.Dir.openFileAbsolute(io, abs_path, .{}); defer f.close(io); + return readFileAll(io, allocator, f, max_bytes); +} + +/// Read from an already-open file descriptor. Security-sensitive callers use +/// this after opening with `O_NOFOLLOW`, keeping the read bound to the object +/// that passed their confinement check. +pub fn readFileAll(io: std.Io, allocator: std.mem.Allocator, f: std.Io.File, max_bytes: usize) ![]u8 { const st = try f.stat(io); const size = @min(@as(u64, max_bytes), st.size); const buf = try allocator.alloc(u8, @intCast(size)); diff --git a/tests/dsl_builtins_test.zig b/tests/dsl_builtins_test.zig index cc74a9f8..a2d06980 100644 --- a/tests/dsl_builtins_test.zig +++ b/tests/dsl_builtins_test.zig @@ -154,10 +154,65 @@ test "Pathname.read returns empty string for a missing file" { defer testing.allocator.free(root); defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; const ctx = mkCtx(root); - const out = try pathname.read(ctx, Value{ .pathname = "/tmp/malt_dsl_read_missing_xyz" }, &.{}); + const missing = try std.fmt.allocPrint(testing.allocator, "{s}/missing", .{root}); + defer testing.allocator.free(missing); + const out = try pathname.read(ctx, Value{ .pathname = missing }, &.{}); try testing.expectEqualStrings("", out.string); } +test "Pathname.read refuses a source outside the sandbox" { + const root = try uniqueSandbox("read_escape_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("read_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}/private", .{outside}); + defer testing.allocator.free(victim); + { + const f = try test_io.createFileAbsolute(std.Options.debug_io, victim, .{}); + defer f.close(std.Options.debug_io); + try f.writeStreamingAll(std.Options.debug_io, "PRIVATE"); + } + + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + var ctx = mkCtx(root); + ctx.allocator = arena.allocator(); + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.read(ctx, Value{ .pathname = victim }, &.{}), + ); +} + +test "Pathname.read refuses a final symlink outside the sandbox" { + const root = try uniqueSandbox("read_symlink_root"); + defer testing.allocator.free(root); + defer test_io.deleteTreeAbsolute(std.Options.debug_io, root) catch {}; + const outside = try uniqueSandbox("read_symlink_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}/private", .{outside}); + defer testing.allocator.free(victim); + const link = try std.fmt.allocPrint(testing.allocator, "{s}/config", .{root}); + defer testing.allocator.free(link); + { + const f = try test_io.createFileAbsolute(std.Options.debug_io, victim, .{}); + defer f.close(std.Options.debug_io); + try f.writeStreamingAll(std.Options.debug_io, "PRIVATE"); + } + try test_io.symLinkAbsolute(std.Options.debug_io, victim, link, .{}); + + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + var ctx = mkCtx(root); + ctx.allocator = arena.allocator(); + try testing.expectError( + pathname.BuiltinError.PathSandboxViolation, + pathname.read(ctx, Value{ .pathname = link }, &.{}), + ); +} + test "Pathname.children returns array of children; empty for missing dir" { const root = try uniqueSandbox("children"); defer testing.allocator.free(root);