From: Andrew Gierth Date: Fri, 29 Mar 2019 08:22:46 +0000 (+0000) Subject: Fix bugs related to unreadable directories. X-Git-Tag: v3.4.0~93^2~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9c775ab1a2ed12113e03cbe97fbac80ce522c2ac;p=thirdparty%2Flibarchive.git 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 31d2429c4..385f8e6e4 100644 --- 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 2990b5088..01760df69 100644 --- 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(); }