Fix bugs related to unreadable directories.

1. Don't try to open ".." for reading as part of the process of
ascending out of an initially specified directory; it's wrong, and if
the directory is not readable it causes a spurious error.

2. If opening "." initially for reading fails, then open it for
execute instead, if O_EXEC exists. This avoids spurious and unhelpful
failures when the current directory is not readable.

Add test cases for the above.

At least the first of these issues is ancient; it was reported against
FreeBSD in 2014.

diff --git a/libarchive/archive_read_disk_posix.c b/libarchive/archive_read_disk_posix.c
index 31d2429c41a5c3ff94ee0e1526fed96250300596..385f8e6e450a62f8244e2d789f5cfe564e9084b2 100644 (file)
--- a/libarchive/archive_read_disk_posix.c
+++ b/libarchive/archive_read_disk_posix.c
@@ -909,7 +909,7 @@ next_entry(struct archive_read_disk *a, struct tree *t,
                            }
                        }
                        break;
-               }       
+               }
        } while (lst == NULL);
 
 #ifdef __APPLE__
@@ -1295,10 +1295,23 @@ archive_read_disk_descend(struct archive *_a)
        if (t->visit_type != TREE_REGULAR || !t->descend)
                return (ARCHIVE_OK);
 
+       /*
+        * We must not treat the initial specified path as a physical dir,
+        * because if we do then we will try and ascend out of it by opening
+        * ".." which is (a) wrong and (b) causes spurious permissions errors
+        * if ".." is not readable by us. Instead, treat it as if it were a
+        * symlink. (This uses an extra fd, but it can only happen once at the
+        * top level of a traverse.) But we can't necessarily assume t->st is
+        * valid here (though t->lst is), which complicates the logic a
+        * little.
+        */
        if (tree_current_is_physical_dir(t)) {
                tree_push(t, t->basename, t->current_filesystem_id,
                    t->lst.st_dev, t->lst.st_ino, &t->restore_time);
-               t->stack->flags |= isDir;
+               if (t->stack->parent->parent != NULL)
+                       t->stack->flags |= isDir;
+               else
+                       t->stack->flags |= isDirLink;
        } else if (tree_current_is_dir(t)) {
                tree_push(t, t->basename, t->current_filesystem_id,
                    t->st.st_dev, t->st.st_ino, &t->restore_time);
@@ -2172,6 +2185,15 @@ tree_reopen(struct tree *t, const char *path, int restore_time)
        t->stack->flags = needsFirstVisit;
        t->maxOpenCount = t->openCount = 1;
        t->initial_dir_fd = open(".", O_RDONLY | O_CLOEXEC);
+#ifdef O_EXEC
+       /*
+        * Most likely reason to fail opening "." is that it's not readable,
+        * so try again for execute. The consequences of not opening this are
+        * unhelpful and unnecessary errors later.
+        */
+       if (t->initial_dir_fd < 0)
+               t->initial_dir_fd = open(".", O_EXEC | O_CLOEXEC);
+#endif
        __archive_ensure_cloexec_flag(t->initial_dir_fd);
        t->working_dir_fd = tree_dup(t->initial_dir_fd);
        return (t);

diff --git a/libarchive/test/test_read_disk_directory_traversals.c b/libarchive/test/test_read_disk_directory_traversals.c
index 2990b50884321d4c44405cd7975f2c99b3b6f991..01760df69b113f29e1b191beeeeed93d46afa4a2 100644 (file)
--- a/libarchive/test/test_read_disk_directory_traversals.c
+++ b/libarchive/test/test_read_disk_directory_traversals.c
@@ -1567,6 +1567,228 @@ test_nodump(void)
        archive_entry_free(ae);
 }
 
+static void
+test_parent(void)
+{
+       struct archive *a;
+       struct archive_entry *ae;
+       const void *p;
+       size_t size;
+       int64_t offset;
+       int file_count;
+       int match_count;
+
+       assertMakeDir("lock", 0311);
+       assertMakeDir("lock/dir1", 0755);
+       assertMakeFile("lock/dir1/f1", 0644, "0123456789");
+       assertMakeDir("lock/lock2", 0311);
+       assertMakeDir("lock/lock2/dir1", 0755);
+       assertMakeFile("lock/lock2/dir1/f1", 0644, "0123456789");
+
+       assert((ae = archive_entry_new()) != NULL);
+       assert((a = archive_read_disk_new()) != NULL);
+
+       /*
+        * Test1: Traverse lock/dir1 as .
+        */
+       assertChdir("lock/dir1");
+
+       failure("Directory traversals should work as well");
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_disk_open(a, "."));
+
+       file_count = 2;
+       match_count = 0;
+       while (file_count--) {
+               archive_entry_clear(ae);
+               assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header2(a, ae));
+               if (strcmp(archive_entry_pathname(ae), ".") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFDIR);
+                       ++match_count;
+               } else if (strcmp(archive_entry_pathname(ae), "./f1") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFREG);
+                       assertEqualInt(archive_entry_size(ae), 10);
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 10);
+                       assertEqualInt((int)offset, 0);
+                       assertEqualMem(p, "0123456789", 10);
+                       assertEqualInt(ARCHIVE_EOF,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 0);
+                       assertEqualInt((int)offset, 10);
+                       ++match_count;
+               }
+               if (archive_read_disk_can_descend(a)) {
+                       /* Descend into the current object */
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_disk_descend(a));
+               }
+       }
+       failure("Did not match expected filenames");
+       assertEqualInt(match_count, 2);
+       /*
+        * There is no entry. This will however fail if the directory traverse
+        * tries to ascend past the initial directory, since it lacks permission
+        * to do so.
+        */
+       failure("There should be no entry and no error");
+       assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header2(a, ae));
+
+       /* Close the disk object. */
+       assertEqualInt(ARCHIVE_OK, archive_read_close(a));
+
+       assertChdir("../..");
+
+       /*
+        * Test2: Traverse lock/dir1 directly
+        */
+       failure("Directory traversals should work as well");
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_disk_open(a, "lock/dir1"));
+
+       file_count = 2;
+       match_count = 0;
+       while (file_count--) {
+               archive_entry_clear(ae);
+               assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header2(a, ae));
+               if (strcmp(archive_entry_pathname(ae), "lock/dir1") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFDIR);
+                       ++match_count;
+               } else if (strcmp(archive_entry_pathname(ae), "lock/dir1/f1") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFREG);
+                       assertEqualInt(archive_entry_size(ae), 10);
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 10);
+                       assertEqualInt((int)offset, 0);
+                       assertEqualMem(p, "0123456789", 10);
+                       assertEqualInt(ARCHIVE_EOF,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 0);
+                       assertEqualInt((int)offset, 10);
+                       ++match_count;
+               }
+               if (archive_read_disk_can_descend(a)) {
+                       /* Descend into the current object */
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_disk_descend(a));
+               }
+       }
+       failure("Did not match expected filenames");
+       assertEqualInt(match_count, 2);
+       /*
+        * There is no entry. This will however fail if the directory traverse
+        * tries to ascend past the initial directory, since it lacks permission
+        * to do so.
+        */
+       failure("There should be no entry and no error");
+       assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header2(a, ae));
+
+       /* Close the disk object. */
+       assertEqualInt(ARCHIVE_OK, archive_read_close(a));
+
+       /*
+        * Test3: Traverse lock/dir1/.
+        */
+       failure("Directory traversals should work as well");
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_disk_open(a, "lock/dir1/."));
+
+       file_count = 2;
+       match_count = 0;
+       while (file_count--) {
+               archive_entry_clear(ae);
+               assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header2(a, ae));
+               if (strcmp(archive_entry_pathname(ae), "lock/dir1/.") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFDIR);
+                       ++match_count;
+               } else if (strcmp(archive_entry_pathname(ae), "lock/dir1/./f1") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFREG);
+                       assertEqualInt(archive_entry_size(ae), 10);
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 10);
+                       assertEqualInt((int)offset, 0);
+                       assertEqualMem(p, "0123456789", 10);
+                       assertEqualInt(ARCHIVE_EOF,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 0);
+                       assertEqualInt((int)offset, 10);
+                       ++match_count;
+               }
+               if (archive_read_disk_can_descend(a)) {
+                       /* Descend into the current object */
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_disk_descend(a));
+               }
+       }
+       failure("Did not match expected filenames");
+       assertEqualInt(match_count, 2);
+       /*
+        * There is no entry. This will however fail if the directory traverse
+        * tries to ascend past the initial directory, since it lacks permission
+        * to do so.
+        */
+       failure("There should be no entry and no error");
+       assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header2(a, ae));
+
+       /* Close the disk object. */
+       assertEqualInt(ARCHIVE_OK, archive_read_close(a));
+
+       /*
+        * Test4: Traverse lock/lock2/dir1 from inside lock.
+        */
+       assertChdir("lock");
+
+       failure("Directory traversals should work as well");
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_disk_open(a, "lock2/dir1"));
+
+       file_count = 2;
+       match_count = 0;
+       while (file_count--) {
+               archive_entry_clear(ae);
+               assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header2(a, ae));
+               if (strcmp(archive_entry_pathname(ae), "lock2/dir1") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFDIR);
+                       ++match_count;
+               } else if (strcmp(archive_entry_pathname(ae), "lock2/dir1/f1") == 0) {
+                       assertEqualInt(archive_entry_filetype(ae), AE_IFREG);
+                       assertEqualInt(archive_entry_size(ae), 10);
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 10);
+                       assertEqualInt((int)offset, 0);
+                       assertEqualMem(p, "0123456789", 10);
+                       assertEqualInt(ARCHIVE_EOF,
+                           archive_read_data_block(a, &p, &size, &offset));
+                       assertEqualInt((int)size, 0);
+                       assertEqualInt((int)offset, 10);
+                       ++match_count;
+               }
+               if (archive_read_disk_can_descend(a)) {
+                       /* Descend into the current object */
+                       assertEqualIntA(a, ARCHIVE_OK,
+                           archive_read_disk_descend(a));
+               }
+       }
+       failure("Did not match expected filenames");
+       assertEqualInt(match_count, 2);
+       /*
+        * There is no entry. This will however fail if the directory traverse
+        * tries to ascend past the initial directory, since it lacks permission
+        * to do so.
+        */
+       failure("There should be no entry and no error");
+       assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header2(a, ae));
+
+       /* Close the disk object. */
+       assertEqualInt(ARCHIVE_OK, archive_read_close(a));
+
+       assertChdir("..");
+
+       /* Destroy the disk object. */
+       assertEqualInt(ARCHIVE_OK, archive_read_free(a));
+       archive_entry_free(ae);
+}
+
 DEFINE_TEST(test_read_disk_directory_traversals)
 {
        /* Basic test. */
@@ -1583,4 +1805,6 @@ DEFINE_TEST(test_read_disk_directory_traversals)
        test_callbacks();
        /* Test nodump. */
        test_nodump();
+       /* Test parent overshoot. */
+       test_parent();
 }