Skip to content

Fix audit findings in mv, touch, rmdir, mktemp #29

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
kelp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix audit findings in mv, touch, rmdir, mktemp #29

Show file tree
Hide file tree
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
98 changes: 74 additions & 24 deletions src/mktemp.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,24 +117,28 @@ pub fn runMktemp(allocator: Allocator, args: []const []const u8, stdout_writer:
// Get template
const raw_template = if (parsed.positionals.len == 1) parsed.positionals[0] else default_template;

// Validate suffix does not contain path separator
const suffix = parsed.suffix orelse "";
if (std.mem.indexOfScalar(u8, suffix, '/') != null) {
// Validate explicit --suffix does not contain path separator
const explicit_suffix = parsed.suffix orelse "";
if (std.mem.indexOfScalar(u8, explicit_suffix, '/') != null) {
if (!parsed.quiet) {
common.printErrorWithProgram(allocator, stderr_writer, prog_name, "invalid suffix '{s}': contains directory separator", .{suffix});
common.printErrorWithProgram(allocator, stderr_writer, prog_name, "invalid suffix '{s}': contains directory separator", .{explicit_suffix});
}
return @intFromEnum(common.ExitCode.general_error);
}

// Count trailing X's in the raw template (before suffix)
const x_count = countTrailingXs(raw_template);
if (x_count < 3) {
// Find X's in the template. Characters after the last run of X's
// are treated as an implicit suffix (GNU mktemp behavior).
const xs = findTemplateXs(raw_template);
if (xs.x_count < 3) {
if (!parsed.quiet) {
common.printErrorWithProgram(allocator, stderr_writer, prog_name, "too few X's in template '{s}'", .{raw_template});
}
return @intFromEnum(common.ExitCode.general_error);
}

// Total suffix = implicit suffix from template + explicit --suffix flag
const total_suffix_len = xs.implicit_suffix_len + explicit_suffix.len;

// Determine the directory to use
const tmpdir = resolveTmpdir(allocator, parsed.tmpdir, parsed.t, raw_template) catch {
if (!parsed.quiet) {
Expand All @@ -146,9 +150,9 @@ pub fn runMktemp(allocator: Allocator, args: []const []const u8, stdout_writer:
// Extract just the filename part of the raw template
const template_basename = std.fs.path.basename(raw_template);

// Build the filename: template_basename + suffix
const filename = if (suffix.len > 0)
std.fmt.allocPrint(allocator, "{s}{s}", .{ template_basename, suffix }) catch {
// Build the filename: template_basename + explicit suffix
const filename = if (explicit_suffix.len > 0)
std.fmt.allocPrint(allocator, "{s}{s}", .{ template_basename, explicit_suffix }) catch {
if (!parsed.quiet) {
common.printErrorWithProgram(allocator, stderr_writer, prog_name, "failed to allocate memory", .{});
}
Expand All @@ -166,8 +170,8 @@ pub fn runMktemp(allocator: Allocator, args: []const []const u8, stdout_writer:
};

// Generate the temporary file or directory
// x_count refers to X's in the template portion (before suffix)
const result_path = generateTemp(allocator, full_template, x_count, suffix.len, parsed.directory, parsed.@"dry-run") catch {
// total_suffix_len covers both implicit (chars after X's) and explicit --suffix
const result_path = generateTemp(allocator, full_template, xs.x_count, total_suffix_len, parsed.directory, parsed.@"dry-run") catch {
if (!parsed.quiet) {
common.printErrorWithProgram(allocator, stderr_writer, prog_name, "failed to create {s} via template '{s}'", .{
if (parsed.directory) "directory" else "file",
Expand All @@ -183,18 +187,59 @@ pub fn runMktemp(allocator: Allocator, args: []const []const u8, stdout_writer:
}

/// Count trailing 'X' characters in the template
fn countTrailingXs(template: []const u8) usize {
var count: usize = 0;
/// Result of parsing X's from a template.
const TemplateXs = struct {
x_count: usize,
/// Number of characters after the X run (implicit suffix).
implicit_suffix_len: usize,
};

/// Find the last consecutive run of X's in the template.
/// Characters after that run are treated as an implicit suffix
/// (GNU mktemp behavior: "myapp.XXXXXXtxt" has 6 X's and
/// implicit suffix "txt").
fn findTemplateXs(template: []const u8) TemplateXs {
// First try strictly trailing X's (most common case)
var trailing: usize = 0;
var i = template.len;
while (i > 0) {
i -= 1;
if (template[i] == 'X') {
count += 1;
trailing += 1;
} else {
break;
}
}
return count;
if (trailing >= 3) {
return .{ .x_count = trailing, .implicit_suffix_len = 0 };
}

// Scan for the last run of 3+ consecutive X's
var best_end: usize = 0;
var best_count: usize = 0;
var pos = template.len;
while (pos > 0) {
pos -= 1;
if (template[pos] == 'X') {
// Count this run backwards
var run_start = pos;
while (run_start > 0 and template[run_start - 1] == 'X') {
run_start -= 1;
}
const run_len = pos - run_start + 1;
if (run_len >= 3) {
best_end = pos + 1;
best_count = run_len;
break;
}
pos = run_start; // skip past this short run
}
}

return .{
.x_count = best_count,
.implicit_suffix_len = if (best_count > 0) template.len - best_end else 0,
};
}

/// Resolve the temporary directory to use
Expand Down Expand Up @@ -337,14 +382,19 @@ fn printVersion(writer: anytype) !void {
// TESTS
// ============================================================================

test "mktemp countTrailingXs" {
try testing.expectEqual(@as(usize, 10), countTrailingXs("tmp.XXXXXXXXXX"));
try testing.expectEqual(@as(usize, 3), countTrailingXs("tmp.XXX"));
try testing.expectEqual(@as(usize, 0), countTrailingXs("tmp.txt"));
try testing.expectEqual(@as(usize, 0), countTrailingXs(""));
try testing.expectEqual(@as(usize, 5), countTrailingXs("XXXXX"));
try testing.expectEqual(@as(usize, 0), countTrailingXs("XXXabc"));
try testing.expectEqual(@as(usize, 3), countTrailingXs("prefixXXX"));
test "mktemp findTemplateXs" {
// Trailing X's (no implicit suffix)
try testing.expectEqual(TemplateXs{ .x_count = 10, .implicit_suffix_len = 0 }, findTemplateXs("tmp.XXXXXXXXXX"));
try testing.expectEqual(TemplateXs{ .x_count = 3, .implicit_suffix_len = 0 }, findTemplateXs("tmp.XXX"));
try testing.expectEqual(TemplateXs{ .x_count = 5, .implicit_suffix_len = 0 }, findTemplateXs("XXXXX"));
try testing.expectEqual(TemplateXs{ .x_count = 3, .implicit_suffix_len = 0 }, findTemplateXs("prefixXXX"));
// No X's at all
try testing.expectEqual(TemplateXs{ .x_count = 0, .implicit_suffix_len = 0 }, findTemplateXs("tmp.txt"));
try testing.expectEqual(TemplateXs{ .x_count = 0, .implicit_suffix_len = 0 }, findTemplateXs(""));
// Implicit suffix (X's not at end)
try testing.expectEqual(TemplateXs{ .x_count = 3, .implicit_suffix_len = 3 }, findTemplateXs("XXXabc"));
try testing.expectEqual(TemplateXs{ .x_count = 6, .implicit_suffix_len = 3 }, findTemplateXs("myapp.XXXXXXtxt"));
try testing.expectEqual(TemplateXs{ .x_count = 6, .implicit_suffix_len = 4 }, findTemplateXs("test.XXXXXX.log"));
}

test "mktemp fillRandom produces alphanumeric characters" {
Expand Down
16 changes: 13 additions & 3 deletions src/mv.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,10 +677,20 @@ fn isDestDirectory(path: []const u8, no_follow_symlink: bool) !bool {

/// Move file or directory with atomic rename or cross-filesystem copy
fn moveFile(allocator: std.mem.Allocator, source: []const u8, dest: []const u8, options: MoveOptions, stdout_writer: anytype, stderr_writer: anytype, hinted_overwrite: *bool) !void {
// Check for same file using fstat to compare both inode and device
// Check for same file using fstat to compare both inode and device.
// If source and dest are hardlinks (same inode, different paths),
// just unlink the source and succeed.
if (common.file_ops.isSameFile(source, dest)) {
common.printErrorWithProgram(allocator, stderr_writer, "mv", "'{s}' and '{s}' are the same file", .{ source, dest });
return error.SameFile;
if (std.mem.eql(u8, source, dest)) {
common.printErrorWithProgram(allocator, stderr_writer, "mv", "'{s}' and '{s}' are the same file", .{ source, dest });
return error.SameFile;
}
// Different names for the same inode (hardlink): remove the source link.
std.fs.cwd().deleteFile(source) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "mv", "cannot remove '{s}': {}", .{ source, err });
return error.SameFile;
};
return;
}

// For no-clobber mode, check if destination exists first
Expand Down
10 changes: 6 additions & 4 deletions src/rmdir.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,16 +210,18 @@ fn removeSingleDirectory(path: []const u8, stdout_writer: anytype, stderr_writer
// stderr_writer unused here, errors handled by caller
_ = stderr_writer;

// Print verbose message before attempting removal (GNU behavior:
// the message indicates the attempt, not success).
if (options.verbose) {
try stdout_writer.print("rmdir: removing directory, '{s}'\n", .{path});
}

std.fs.cwd().deleteDir(path) catch |err| {
return switch (err) {
error.DirNotEmpty => if (options.ignore_fail_on_non_empty) return else err,
else => err,
};
};

if (options.verbose) {
try stdout_writer.print("rmdir: removing directory, '{s}'\n", .{path});
}
}

/// Remove directory with its parent directories.
Expand Down
4 changes: 3 additions & 1 deletion src/touch.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,10 @@ pub fn runTouch(allocator: std.mem.Allocator, args: []const []const u8, stdout_w
// Map long form aliases to short form
const access_only = parsed_args.a;
const modify_only = parsed_args.m;
const no_create = parsed_args.c or parsed_args.no_create;
const no_dereference = parsed_args.h or parsed_args.no_dereference;
// -h/--no-dereference implies -c: creating a regular file when the
// intent is to act on a symlink makes no sense (GNU behavior).
const no_create = parsed_args.c or parsed_args.no_create or no_dereference;

// Create options struct
const options = TouchOptions{
Expand Down
Loading