From 27588eba5077c1dd311924c51063289c0cbe4db3 Mon Sep 17 00:00:00 2001 From: Jessica Clarke Date: Tue, 7 Jan 2025 20:59:09 +0000 Subject: [PATCH] Fix replacing a regular file with a dir for ARCHIVE_EXTRACT_SAFE_WRITES The outer if checks !S_ISDIR(a->st.st_mode), so we know that the file being overwritten is not a directory, and thus we can rename(2) over it if we want to, but whether we can use a temporary regular file is a property of the file being extracted. Otherwise, when replacing a regular file with a directory, we end up in this case and create a temporary regular file for the new directory, but with the permissions of the directory (which likely includes x), and rename it over the top at the end. Depending on where the archive_entry came from, it may have a non-zero size that also isn't ovewritten with 0 (e.g. if it came from stat(2)) and so the API user may then try to copy data (thus failing if read(2) of directories isn't permitted, or writing the raw directory contents if it is), but if the size is zero as is the case for this tar test then it will end up not writing any data and "successfully" overwrite the file with an empty file, not a directory. --- libarchive/archive_write_disk_posix.c | 2 +- tar/test/test_option_safe_writes.c | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/libarchive/archive_write_disk_posix.c b/libarchive/archive_write_disk_posix.c index 6b6ded739..443d88e7d 100644 --- a/libarchive/archive_write_disk_posix.c +++ b/libarchive/archive_write_disk_posix.c @@ -2202,7 +2202,7 @@ restore_entry(struct archive_write_disk *a) (void)clear_nochange_fflags(a); if ((a->flags & ARCHIVE_EXTRACT_SAFE_WRITES) && - S_ISREG(a->st.st_mode)) { + S_ISREG(a->mode)) { /* Use a temporary file to extract */ if ((a->fd = la_mktemp(a)) == -1) { archive_set_error(&a->archive, errno, diff --git a/tar/test/test_option_safe_writes.c b/tar/test/test_option_safe_writes.c index b88479bc5..d30b9a745 100644 --- a/tar/test/test_option_safe_writes.c +++ b/tar/test/test_option_safe_writes.c @@ -16,11 +16,12 @@ DEFINE_TEST(test_option_safe_writes) assertMakeFile("d", 0644, "c"); assertMakeFile("fs", 0644, "d"); assertMakeFile("ds", 0644, "e"); + assertMakeDir("fd", 0755); assertEqualInt(0, chdir("..")); /* Tar files up */ assertEqualInt(0, - systemf("%s -c -C in -f t.tar f fh d fs ds " + systemf("%s -c -C in -f t.tar f fh d fs ds fd " ">pack.out 2>pack.err", testprog)); /* Verify that nothing went to stdout or stderr. */ @@ -32,6 +33,7 @@ DEFINE_TEST(test_option_safe_writes) assertEqualInt(0, chdir("out")); assertMakeFile("f", 0644, "a"); assertMakeHardlink("fh", "f"); + assertMakeFile("fd", 0644, "b"); assertMakeDir("d", 0755); if (canSymlink()) { assertMakeSymlink("fs", "f", 0); @@ -55,4 +57,5 @@ DEFINE_TEST(test_option_safe_writes) assertTextFileContents("c","d"); assertTextFileContents("d","fs"); assertTextFileContents("e","ds"); + assertIsDir("fd", 0755); } -- 2.47.3