Fix following symlinks when processing the fixup list

The previous fix in b41daecb5 was incomplete. Fixup entries are
given the original path without calling cleanup_pathname().
To make sure we don't follow a symlink, we must strip trailing
slashes from the path.

The fixup entries are always directories. Make sure we try to modify
only directories by providing O_DIRECTORY to open() (if supported)
and if it fails to check directory via lstat().

Fixes #1566

diff --git a/libarchive/archive_write_disk_posix.c b/libarchive/archive_write_disk_posix.c
index fcd733aff51ee43ebff384f7fb56902138341701..aadc58718f9b69c7d47de74e667076ab8dc83c94 100644 (file)
--- a/libarchive/archive_write_disk_posix.c
+++ b/libarchive/archive_write_disk_posix.c
@@ -2462,6 +2462,7 @@ _archive_write_disk_close(struct archive *_a)
        struct archive_write_disk *a = (struct archive_write_disk *)_a;
        struct fixup_entry *next, *p;
        struct stat st;
+       char *c;
        int fd, ret;
 
        archive_check_magic(&a->archive, ARCHIVE_WRITE_DISK_MAGIC,
@@ -2475,24 +2476,49 @@ _archive_write_disk_close(struct archive *_a)
        while (p != NULL) {
                fd = -1;
                a->pst = NULL; /* Mark stat cache as out-of-date. */
-               if (p->fixup &
-                   (TODO_TIMES | TODO_MODE_BASE | TODO_ACLS | TODO_FFLAGS)) {
-                       fd = open(p->name,
-                           O_WRONLY | O_BINARY | O_NOFOLLOW | O_CLOEXEC);
+
+               /* We must strip trailing slashes from the path to avoid
+                  dereferencing symbolic links to directories */
+               c = p->name;
+               while (*c != '\0')
+                       c++;
+               while (c != p->name && *(c - 1) == '/') {
+                       c--;
+                       *c = '\0';
+               }
+
+               if (p->fixup == 0)
+                       goto skip_fixup_entry;
+               else {
+                       fd = open(p->name, O_BINARY | O_NOFOLLOW | O_RDONLY
+#if defined(O_DIRECTORY)
+                           | O_DIRECTORY
+#endif
+                           | O_CLOEXEC);
+                       /*
+                `       * If we don't support O_DIRECTORY,
+                        * or open() has failed, we must stat()
+                        * to verify that we are opening a directory
+                        */
+#if defined(O_DIRECTORY)
                        if (fd == -1) {
-                               /* If we cannot lstat, skip entry */
-                               if (lstat(p->name, &st) != 0)
+                               if (lstat(p->name, &st) != 0 ||
+                                   !S_ISDIR(st.st_mode)) {
                                        goto skip_fixup_entry;
-                               /*
-                                * If we deal with a symbolic link, mark
-                                * it in the fixup mode to ensure no
-                                * modifications are made to its target.
-                                */
-                               if (S_ISLNK(st.st_mode)) {
-                                       p->mode &= ~S_IFMT;
-                                       p->mode |= S_IFLNK;
                                }
                        }
+#else
+#if HAVE_FSTAT
+                       if (fd > 0 && (
+                           fstat(fd, &st) != 0 || !S_ISDIR(st.st_mode))) {
+                               goto skip_fixup_entry;
+                       } else
+#endif
+                       if (lstat(p->name, &st) != 0 ||
+                           !S_ISDIR(st.st_mode)) {
+                               goto skip_fixup_entry;
+                       }
+#endif
                }
                if (p->fixup & TODO_TIMES) {
                        set_times(a, fd, p->mode, p->name,
@@ -2504,14 +2530,13 @@ _archive_write_disk_close(struct archive *_a)
                if (p->fixup & TODO_MODE_BASE) {
 #ifdef HAVE_FCHMOD
                        if (fd >= 0)
-                               fchmod(fd, p->mode);
+                               fchmod(fd, p->mode & 07777);
                        else
 #endif
 #ifdef HAVE_LCHMOD
-                       lchmod(p->name, p->mode);
+                       lchmod(p->name, p->mode & 07777);
 #else
-                       if (!S_ISLNK(p->mode))
-                               chmod(p->name, p->mode);
+                       chmod(p->name, p->mode & 07777);
 #endif
                }
                if (p->fixup & TODO_ACLS)
@@ -2664,7 +2689,6 @@ new_fixup(struct archive_write_disk *a, const char *pathname)
        fe->next = a->fixup_list;
        a->fixup_list = fe;
        fe->fixup = 0;
-       fe->mode = 0;
        fe->name = strdup(pathname);
        return (fe);
 }

diff --git a/libarchive/test/test_write_disk_fixup.c b/libarchive/test/test_write_disk_fixup.c
index c399c9842e46fa0425f9a6db8826d36f868b0971..b83b73079290db0f40175ec0a5c000d6cdcd0f20 100644 (file)
--- a/libarchive/test/test_write_disk_fixup.c
+++ b/libarchive/test/test_write_disk_fixup.c
@@ -47,26 +47,50 @@ DEFINE_TEST(test_write_disk_fixup)
        /*
         * Create a file
         */
-       assertMakeFile("victim", 0600, "a");
+       assertMakeFile("file", 0600, "a");
+
+       /*
+        * Create a directory
+        */
+       assertMakeDir("dir", 0700);
 
        /*
         * Create a directory and a symlink with the same name
         */
 
-       /* Directory: dir */
+       /* Directory: dir1 */
+        assert((ae = archive_entry_new()) != NULL);
+        archive_entry_copy_pathname(ae, "dir1/");
+        archive_entry_set_mode(ae, AE_IFDIR | 0555);
+       assertEqualIntA(ad, 0, archive_write_header(ad, ae));
+       assertEqualIntA(ad, 0, archive_write_finish_entry(ad));
+        archive_entry_free(ae);
+
+       /* Directory: dir2 */
         assert((ae = archive_entry_new()) != NULL);
-        archive_entry_copy_pathname(ae, "dir");
-        archive_entry_set_mode(ae, AE_IFDIR | 0606);
+        archive_entry_copy_pathname(ae, "dir2/");
+        archive_entry_set_mode(ae, AE_IFDIR | 0555);
        assertEqualIntA(ad, 0, archive_write_header(ad, ae));
        assertEqualIntA(ad, 0, archive_write_finish_entry(ad));
         archive_entry_free(ae);
 
-       /* Symbolic Link: dir -> foo */
+       /* Symbolic Link: dir1 -> dir */
+       assert((ae = archive_entry_new()) != NULL);
+       archive_entry_copy_pathname(ae, "dir1");
+       archive_entry_set_mode(ae, AE_IFLNK | 0777);
+       archive_entry_set_size(ae, 0);
+       archive_entry_copy_symlink(ae, "dir");
+       assertEqualIntA(ad, 0, r = archive_write_header(ad, ae));
+       if (r >= ARCHIVE_WARN)
+               assertEqualIntA(ad, 0, archive_write_finish_entry(ad));
+       archive_entry_free(ae);
+
+       /* Symbolic Link: dir2 -> file */
        assert((ae = archive_entry_new()) != NULL);
-       archive_entry_copy_pathname(ae, "dir");
+       archive_entry_copy_pathname(ae, "dir2");
        archive_entry_set_mode(ae, AE_IFLNK | 0777);
        archive_entry_set_size(ae, 0);
-       archive_entry_copy_symlink(ae, "victim");
+       archive_entry_copy_symlink(ae, "file");
        assertEqualIntA(ad, 0, r = archive_write_header(ad, ae));
        if (r >= ARCHIVE_WARN)
                assertEqualIntA(ad, 0, archive_write_finish_entry(ad));
@@ -75,7 +99,9 @@ DEFINE_TEST(test_write_disk_fixup)
        assertEqualInt(ARCHIVE_OK, archive_write_free(ad));
 
        /* Test the entries on disk. */
-       assertIsSymlink("dir", "victim", 0);
-       assertFileMode("victim", 0600);
+       assertIsSymlink("dir1", "dir", 0);
+       assertIsSymlink("dir2", "file", 0);
+       assertFileMode("dir", 0700);
+       assertFileMode("file", 0600);
 #endif
 }