Skip to content

fix(archive): validate GNU tar metadata #838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rustytrees wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(archive): validate GNU tar metadata #838

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 119 additions & 18 deletions src/fs/archive.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,9 @@ fn applyHardLinks(io: std.Io, dir: std.Io.Dir, links: []const HardLink) !void {

/// Walk the tar archive at raw 512-byte blocks to enforce path safety
/// AND recover hard-link pairs the std iterator silently drops.
/// Pax extended headers ('x') are interpreted for `linkpath=`/`path=` so
/// the pre-scan validates the *effective* symlink target the extractor
/// will use, not the stale ustar field. GNU long-name extensions remain
/// uninterpreted: a hardlink whose name lives in a GNU header is not found
/// here. Homebrew bottles use plain ustar paths, so that gap is acceptable.
/// Pax extended headers ('x') and GNU long-name/long-link records ('L'/'K')
/// are interpreted so the pre-scan validates the *effective* name and symlink
/// target the extractor will use, not stale placeholder fields.
fn preScanTarGz(
io: std.Io,
arena: std.mem.Allocator,
Expand Down Expand Up @@ -215,11 +213,11 @@ fn preScanTarGz(
// "one level up" hops compose into an unbounded escape.
var symlink_names: std.StringHashMapUnmanaged(void) = .empty;

// Pax overrides apply to the single entry that follows the 'x' header,
// mirroring std.tar: `path=` replaces the name, `linkpath=` the target.
// Both reset once consumed (or when a new 'x' supersedes them).
var pax_name: ?[]const u8 = null;
var pax_link: ?[]const u8 = null;
// Metadata overrides apply to the next real entry. A pax header replaces
// the whole pending record, while GNU L/K records replace one field. This
// mirrors std.tar.Iterator's File accumulator exactly.
var override_name: ?[]const u8 = null;
var override_link: ?[]const u8 = null;

while (true) {
var header: [512]u8 = undefined;
Expand All @@ -240,14 +238,23 @@ fn preScanTarGz(
// fields, so validating the raw ustar field here would check bytes
// the extractor never uses. Each 'x' supersedes the previous one.
if (kind_byte == 'x') {
pax_name = null;
pax_link = null;
try parsePaxOverrides(r, arena, size, &pax_name, &pax_link);
override_name = null;
override_link = null;
try parsePaxOverrides(r, arena, size, &override_name, &override_link);
// The parse consumed exactly `size` bytes; skip only the block pad.
const pad: u64 = (512 - (size % 512)) % 512;
if (pad > 0) r.discardAll64(pad) catch return error.ExtractionFailed;
continue;
}
// GNU metadata stores the next entry's effective name or link target
// as a NUL-terminated payload. std.tar applies these records before it
// validates or extracts the following entry, so the security scan must
// consume the same bytes.
if (kind_byte == 'L' or kind_byte == 'K') {
const value = try parseGnuOverride(r, arena, size);
if (kind_byte == 'L') override_name = value else override_link = value;
continue;
}
// Global pax header is never applied to later entries (std.tar
// discards it); skip its payload and keep any pending overrides.
if (kind_byte == 'g') {
Expand All @@ -266,11 +273,11 @@ fn preScanTarGz(
if (!isSafeEntryPath(ustar_name)) return error.ExtractionFailed;
// A pax `path=` override must clear the same bar; std.tar sanitises
// names regardless, but the pre-scan should not silently diverge.
if (pax_name) |pn| if (!isSafeEntryPath(pn)) return error.ExtractionFailed;
const name = pax_name orelse ustar_name;
if (override_name) |pn| if (!isSafeEntryPath(pn)) return error.ExtractionFailed;
const name = override_name orelse ustar_name;

const ustar_link = nullSlice(header[157..257]);
const link_name = pax_link orelse ustar_link;
const link_name = override_link orelse ustar_link;

// No entry may be reached through a symlink this archive created —
// that is the step that turns a bounded, dangling link into a write
Expand Down Expand Up @@ -304,8 +311,8 @@ fn preScanTarGz(
}
// Pending overrides are consumed by this entry whatever its kind;
// reset so they can never leak forward to an unrelated entry.
pax_name = null;
pax_link = null;
override_name = null;
override_link = null;

// Advance past the data payload (rounded up to the next 512-byte
// boundary). Symlinks/hardlinks/dirs report size 0 so this is a
Expand All @@ -319,6 +326,18 @@ fn preScanTarGz(
return hardlinks.toOwnedSlice(arena);
}

/// Read one GNU `L`/`K` payload and its block padding. Zig's iterator buffers
/// at most `max_path_bytes` and uses the bytes before the first NUL.
fn parseGnuOverride(r: *std.Io.Reader, arena: std.mem.Allocator, size: u64) ![]const u8 {
const len = std.math.cast(usize, size) orelse return error.ExtractionFailed;
if (len > std.Io.Dir.max_path_bytes) return error.ExtractionFailed;
const payload = arena.alloc(u8, len) catch return error.ExtractionFailed;
r.readSliceAll(payload) catch return error.ExtractionFailed;
const pad: u64 = (512 - (size % 512)) % 512;
if (pad > 0) r.discardAll64(pad) catch return error.ExtractionFailed;
return nullSlice(payload);
}

/// Parse a pax extended-header payload (`size` bytes from the current block)
/// for `path=`/`linkpath=`, returning the last value seen for each — std.tar
/// applies these over the ustar fields of the following entry. The returned
Expand Down Expand Up @@ -690,6 +709,20 @@ const TestTar = struct {
self.len += (rec.len + 511) / 512 * 512;
}

/// Append a GNU long-name/long-link metadata record. Zig's tar iterator
/// applies this value to the next entry, so the security pre-scan must
/// validate the same effective bytes rather than the following ustar
/// header's placeholder field.
fn gnuString(self: *TestTar, typeflag: u8, value: []const u8) void {
const payload_len = value.len + 1; // GNU payload is NUL-terminated.
const h = testTarHeader("././@LongLink", typeflag, "", payload_len);
@memcpy(self.buf[self.len..][0..512], h[0..]);
self.len += 512;
@memcpy(self.buf[self.len..][0..value.len], value);
self.buf[self.len + value.len] = 0;
self.len += (payload_len + 511) / 512 * 512;
}

/// Append a regular-file entry with `data` as its payload.
fn file(self: *TestTar, name: []const u8, data: []const u8) void {
const h = testTarHeader(name, '0', "", data.len);
Expand Down Expand Up @@ -731,6 +764,74 @@ test "extractTarGz rejects a pax linkpath that escapes the destination" {
try std.testing.expectError(error.ExtractionFailed, testExtract(io, &s, t.bytes()));
}

test "extractTarGz rejects an escaping GNU long-link target" {
var threaded: std.Io.Threaded = .init(std.testing.allocator, .{});
defer threaded.deinit();
const io = threaded.io();

var s = try Scratch.init("gnu_long_link_escape");
defer s.deinit();
try s.dir.createDirPath(io, "dest");

var raw: [4096]u8 = undefined;
var t = TestTar.init(&raw);
t.gnuString('K', "../../../../../../tmp/malt-gnu-link-escape");
t.entry("alias", '2', "benign");

const result = testExtract(io, &s, t.bytes());
var link_buf: [std.fs.max_path_bytes]u8 = undefined;
if (std.Io.Dir.readLinkAbsolute(io, s.p("/dest/alias"), &link_buf)) |_| {
return error.TestUnexpectedResult;
} else |err| try std.testing.expectEqual(error.FileNotFound, err);
try std.testing.expectError(error.ExtractionFailed, result);
}

test "extractTarGz rejects a write through a GNU long-named symlink" {
var threaded: std.Io.Threaded = .init(std.testing.allocator, .{});
defer threaded.deinit();
const io = threaded.io();

var s = try Scratch.init("gnu_long_name_symlink");
defer s.deinit();
try s.dir.createDirPath(io, "dest");
try s.dir.createDirPath(io, "outside");

var raw: [8192]u8 = undefined;
var t = TestTar.init(&raw);
t.gnuString('L', "door");
t.entry("placeholder", '2', "../outside");
t.file("door/escaped", "owned");

const result = testExtract(io, &s, t.bytes());
try std.testing.expectError(
error.FileNotFound,
std.Io.Dir.accessAbsolute(io, s.p("/outside/escaped"), .{}),
);
try std.testing.expectError(error.ExtractionFailed, result);
}

test "extractTarGz accepts safe GNU long-name and long-link metadata" {
var threaded: std.Io.Threaded = .init(std.testing.allocator, .{});
defer threaded.deinit();
const io = threaded.io();

var s = try Scratch.init("gnu_long_safe");
defer s.deinit();
try s.dir.createDirPath(io, "dest");

var raw: [8192]u8 = undefined;
var t = TestTar.init(&raw);
t.file("target", "payload");
t.gnuString('L', "nested/alias");
t.gnuString('K', "../target");
t.entry("placeholder", '2', "placeholder");

try testExtract(io, &s, t.bytes());
var link_buf: [std.fs.max_path_bytes]u8 = undefined;
const link_len = try std.Io.Dir.readLinkAbsolute(io, s.p("/dest/nested/alias"), &link_buf);
try std.testing.expectEqualStrings("../target", link_buf[0..link_len]);
}

test "extractTarGz accepts a pax linkpath within the destination" {
var threaded: std.Io.Threaded = .init(std.testing.allocator, .{});
defer threaded.deinit();
Expand Down
Loading