CVE-2021-31566: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: An improper link resolution flaw can occur while extracting an archive leading to changing modes, times, access control lists, and flags of a file outside of the archive. An attacker may provide a malicious archive to a victim user, who would trigger this flaw when trying to extract the archive. A local attacker may use this flaw to gain more privileges in a system.
CWE description: The product attempts to access a file based on the filename, but it does not properly prevent that filename from identifying a link or shortcut that resolves to an unintended resource.
CVSS v3 base score: 7.8
Published: 2022-08-23T16:15Z
Repository: https://github.com/libarchive/libarchive
Language: C
Files changed: Makefile.am, libarchive/archive_write_disk_posix.c, libarchive/test/CMakeLists.txt, libarchive/test/test_write_disk_fixup.c
Commit date: 2021-08-22 06:13:54 +0200
Commit message: Do not follow symlinks when processing the fixup list

Use lchmod() instead of chmod() and tell the remaining functions that the
real file to be modified is a symbolic link.

Fixes #1566

Diff: diff --git a/Makefile.am b/Makefile.am
index 58edb74e33..c93a82e922 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -558,10 +558,11 @@ libarchive_test_SOURCES= \
 	libarchive/test/test_ustar_filename_encoding.c \
 	libarchive/test/test_warn_missing_hardlink_target.c \
 	libarchive/test/test_write_disk.c \
 	libarchive/test/test_write_disk_appledouble.c \
 	libarchive/test/test_write_disk_failures.c \
+	libarchive/test/test_write_disk_fixup.c \
 	libarchive/test/test_write_disk_hardlink.c \
 	libarchive/test/test_write_disk_hfs_compression.c \
 	libarchive/test/test_write_disk_lookup.c \
 	libarchive/test/test_write_disk_mac_metadata.c \
 	libarchive/test/test_write_disk_no_hfs_compression.c \
diff --git a/libarchive/archive_write_disk_posix.c b/libarchive/archive_write_disk_posix.c
index 8474617eb5..fcd733aff5 100644
--- a/libarchive/archive_write_disk_posix.c
+++ b/libarchive/archive_write_disk_posix.c
@@ -2459,10 +2459,11 @@ create_filesystem_object(struct archive_write_disk *a)
 static int
 _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;
 	int fd, ret;
 
 	archive_check_magic(&a->archive, ARCHIVE_WRITE_DISK_MAGIC,
 	    ARCHIVE_STATE_HEADER | ARCHIVE_STATE_DATA,
 	    "archive_write_disk_close");
@@ -2476,10 +2477,24 @@ _archive_write_disk_close(struct archive *_a)
 		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);
+			if (fd == -1) {
+				/* If we cannot lstat, skip entry */
+				if (lstat(p->name, &st) != 0)
+					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;
+				}
+			}
 		}
 		if (p->fixup & TODO_TIMES) {
 			set_times(a, fd, p->mode, p->name,
 			    p->atime, p->atime_nanos,
 			    p->birthtime, p->birthtime_nanos,
@@ -2490,21 +2505,27 @@ _archive_write_disk_close(struct archive *_a)
 #ifdef HAVE_FCHMOD
 			if (fd >= 0)
 				fchmod(fd, p->mode);
 			else
 #endif
-			chmod(p->name, p->mode);
+#ifdef HAVE_LCHMOD
+			lchmod(p->name, p->mode);
+#else
+			if (!S_ISLNK(p->mode))
+				chmod(p->name, p->mode);
+#endif
 		}
 		if (p->fixup & TODO_ACLS)
 			archive_write_disk_set_acls(&a->archive, fd,
 			    p->name, &p->acl, p->mode);
 		if (p->fixup & TODO_FFLAGS)
 			set_fflags_platform(a, fd, p->name,
 			    p->mode, p->fflags_set, 0);
 		if (p->fixup & TODO_MAC_METADATA)
 			set_mac_metadata(a, p->name, p->mac_metadata,
 					 p->mac_metadata_size);
+skip_fixup_entry:
 		next = p->next;
 		archive_acl_clear(&p->acl);
 		free(p->mac_metadata);
 		free(p->name);
 		if (fd >= 0)
@@ -2641,10 +2662,11 @@ new_fixup(struct archive_write_disk *a, const char *pathname)
 		return (NULL);
 	}
 	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/CMakeLists.txt b/libarchive/test/CMakeLists.txt
index b26f679cf0..53cc3e2256 100644
--- a/libarchive/test/CMakeLists.txt
+++ b/libarchive/test/CMakeLists.txt
@@ -207,10 +207,11 @@ IF(ENABLE_TEST)
     test_ustar_filenames.c
     test_warn_missing_hardlink_target.c
     test_write_disk.c
     test_write_disk_appledouble.c
     test_write_disk_failures.c
+    test_write_disk_fixup.c
     test_write_disk_hardlink.c
     test_write_disk_hfs_compression.c
     test_write_disk_lookup.c
     test_write_disk_mac_metadata.c
     test_write_disk_no_hfs_compression.c
diff --git a/libarchive/test/test_write_disk_fixup.c b/libarchive/test/test_write_disk_fixup.c
new file mode 100644
index 0000000000..153cc3a9c6
--- /dev/null
+++ b/libarchive/test/test_write_disk_fixup.c
@@ -0,0 +1,77 @@
+/*-
+ * Copyright (c) 2021 Martin Matuska
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR(S) ``AS IS'' AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+ * IN NO EVENT SHALL THE AUTHOR(S) BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include "test.h"
+
+/*
+ * Test fixup entries don't follow symlinks
+ */
+DEFINE_TEST(test_write_disk_fixup)
+{
+	struct archive *ad;
+	struct archive_entry *ae;
+	int r;
+
+	if (!canSymlink()) {
+		skipping("Symlinks not supported");
+		return;
+	}
+
+	/* Write entries to disk. */
+	assert((ad = archive_write_disk_new()) != NULL);
+
+	/*
+	 * Create a file
+	 */
+	assertMakeFile("victim", 0600, "a");
+
+	/*
+	 * Create a directory and a symlink with the same name
+	 */
+
+	/* Directory: dir */
+        assert((ae = archive_entry_new()) != NULL);
+        archive_entry_copy_pathname(ae, "dir");
+        archive_entry_set_mode(ae, AE_IFDIR | 0606);
+	assertEqualIntA(ad, 0, archive_write_header(ad, ae));
+	assertEqualIntA(ad, 0, archive_write_finish_entry(ad));
+        archive_entry_free(ae);
+
+	/* Symbolic Link: dir -> foo */
+	assert((ae = archive_entry_new()) != NULL);
+	archive_entry_copy_pathname(ae, "dir");
+	archive_entry_set_mode(ae, AE_IFLNK | 0777);
+	archive_entry_set_size(ae, 0);
+	archive_entry_copy_symlink(ae, "victim");
+	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);
+
+	assertEqualInt(ARCHIVE_OK, archive_write_free(ad));
+
+	/* Test the entries on disk. */
+	assertIsSymlink("dir", "victim", 0);
+	assertFileMode("victim", 0600);
+}

Vulnerable code: 			chmod(p->name, p->mode);
Fixed code: 	libarchive/test/test_write_disk_fixup.c \
	struct stat st;
			if (fd == -1) {
				/* If we cannot lstat, skip entry */
				if (lstat(p->name, &st) != 0)
					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;
				}
			}
#ifdef HAVE_LCHMOD
			lchmod(p->name, p->mode);
#else
			if (!S_ISLNK(p->mode))
				chmod(p->name, p->mode);
#endif
skip_fixup_entry:
	fe->mode = 0;
    test_write_disk_fixup.c
/*-
 * Copyright (c) 2021 Martin Matuska
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 *
 * THIS SOFTWARE IS PROVIDED BY THE AUTHOR(S) ``AS IS'' AND ANY EXPRESS OR
 * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
 * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
 * IN NO EVENT SHALL THE AUTHOR(S) BE LIABLE FOR ANY DIRECT, INDIRECT,
 * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
 * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
 * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
 * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */
#include "test.h"

/*
 * Test fixup entries don't follow symlinks
 */
DEFINE_TEST(test_write_disk_fixup)
{
	struct archive *ad;
	struct archive_entry *ae;
	int r;

	if (!canSymlink()) {
		skipping("Symlinks not supported");
		return;
	}

	/* Write entries to disk. */
	assert((ad = archive_write_disk_new()) != NULL);

	/*
	 * Create a file
	 */
	assertMakeFile("victim", 0600, "a");

	/*
	 * Create a directory and a symlink with the same name
	 */

	/* Directory: dir */
        assert((ae = archive_entry_new()) != NULL);
        archive_entry_copy_pathname(ae, "dir");
        archive_entry_set_mode(ae, AE_IFDIR | 0606);
	assertEqualIntA(ad, 0, archive_write_header(ad, ae));
	assertEqualIntA(ad, 0, archive_write_finish_entry(ad));
        archive_entry_free(ae);

	/* Symbolic Link: dir -> foo */
	assert((ae = archive_entry_new()) != NULL);
	archive_entry_copy_pathname(ae, "dir");
	archive_entry_set_mode(ae, AE_IFLNK | 0777);
	archive_entry_set_size(ae, 0);
	archive_entry_copy_symlink(ae, "victim");
	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);

	assertEqualInt(ARCHIVE_OK, archive_write_free(ad));

	/* Test the entries on disk. */
	assertIsSymlink("dir", "victim", 0);
	assertFileMode("victim", 0600);
}