Fix ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS on windows (#2363)

Makefile.am

@@ -582,6 +582,7 @@ libarchive_test_SOURCES= \
 	libarchive/test/test_write_disk_no_hfs_compression.c \
 	libarchive/test/test_write_disk_perms.c \
 	libarchive/test/test_write_disk_secure.c \
+	libarchive/test/test_write_disk_secure_noabsolutepaths.c \
 	libarchive/test/test_write_disk_secure744.c \
 	libarchive/test/test_write_disk_secure745.c \
 	libarchive/test/test_write_disk_secure746.c \
@@ -1266,6 +1267,7 @@ bsdcpio_test_SOURCES= \
 	cpio/test/test_basic.c \
 	cpio/test/test_cmdline.c \
 	cpio/test/test_extract_cpio_Z.c \
+	cpio/test/test_extract_cpio_absolute_paths.c \
 	cpio/test/test_extract_cpio_bz2.c \
 	cpio/test/test_extract_cpio_grz.c \
 	cpio/test/test_extract_cpio_gz.c \

cpio/test/CMakeLists.txt

@@ -14,6 +14,7 @@ IF(ENABLE_CPIO AND ENABLE_TEST)
     test_basic.c
     test_cmdline.c
     test_extract_cpio_Z.c
+    test_extract_cpio_absolute_paths.c
     test_extract_cpio_bz2.c
     test_extract_cpio_grz.c
     test_extract_cpio_gz.c

cpio/test/test_extract_cpio_absolute_paths.c

@@ -0,0 +1,67 @@
+/*-
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2024 Mostyn Bramley-Moore <mostyn@antipode.se>
+ */
+
+#include "test.h"
+
+#include <stdlib.h>
+
+#if defined(_WIN32) && !defined(__CYGWIN__)
+
+#include <fileapi.h>
+
+#define UNLINK _unlink
+
+#else
+
+#define UNLINK unlink
+
+#endif
+
+DEFINE_TEST(test_extract_cpio_absolute_paths)
+{
+	int r;
+
+#if defined(_WIN32) && !defined(__CYGWIN__)
+	char temp_dir[MAX_PATH + 1];
+	assert(GetTempPathA(MAX_PATH + 1, temp_dir) != 0);
+
+	char temp_file_name[MAX_PATH + 1];
+	char temp_absolute_file_name[MAX_PATH + 1];
+	assert(GetTempFileNameA(temp_dir, "abs", 0, temp_file_name) != 0);
+
+	assert(_fullpath(temp_absolute_file_name, temp_file_name, MAX_PATH) != NULL);
+#else
+	char temp_absolute_file_name[] = "/tmp/cpio-noabs.testXXXXXX";
+	mkstemp(temp_absolute_file_name);
+#endif
+
+	UNLINK(temp_absolute_file_name);
+
+	const char *sample_data = "test file from test_extract_cpio_absolute_paths";
+
+	assertMakeFile("filelist", 0644, temp_absolute_file_name);
+	assertMakeFile(temp_absolute_file_name, 0644, sample_data);
+
+	// Create an archive with the absolute path.
+	r = systemf("%s -o < filelist > archive.cpio 2> stderr1.txt", testprog);
+	UNLINK("filelist");
+	assertEqualInt(r, 0);
+
+	// Ensure that the temp file does not exist.
+	UNLINK(temp_absolute_file_name);
+
+	// We should refuse to create the absolute path without --insecure.
+	r = systemf("%s -i < archive.cpio 2> stderr2.txt", testprog);
+	//assert(r != 0); // Should this command fail?
+	assertFileNotExists(temp_absolute_file_name);
+	UNLINK(temp_absolute_file_name); // Cleanup just in case.
+
+	// But if we specify --insecure then the absolute path should be created.
+	r = systemf("%s -i --insecure < archive.cpio 2> stderr3.txt", testprog);
+	assert(r == 0);
+	assertFileExists(temp_absolute_file_name);
+	UNLINK(temp_absolute_file_name);
+}

libarchive/archive_write_disk_windows.c

@@ -1362,23 +1362,23 @@ archive_write_disk_set_user_lookup(struct archive *_a,
 int64_t
 archive_write_disk_gid(struct archive *_a, const char *name, la_int64_t id)
 {
-       struct archive_write_disk *a = (struct archive_write_disk *)_a;
-       archive_check_magic(&a->archive, ARCHIVE_WRITE_DISK_MAGIC,
-           ARCHIVE_STATE_ANY, "archive_write_disk_gid");
-       if (a->lookup_gid)
-               return (a->lookup_gid)(a->lookup_gid_data, name, id);
-       return (id);
+	struct archive_write_disk *a = (struct archive_write_disk *)_a;
+	archive_check_magic(&a->archive, ARCHIVE_WRITE_DISK_MAGIC,
+		ARCHIVE_STATE_ANY, "archive_write_disk_gid");
+	if (a->lookup_gid)
+		return (a->lookup_gid)(a->lookup_gid_data, name, id);
+	return (id);
 }
  
 int64_t
 archive_write_disk_uid(struct archive *_a, const char *name, la_int64_t id)
 {
-       struct archive_write_disk *a = (struct archive_write_disk *)_a;
-       archive_check_magic(&a->archive, ARCHIVE_WRITE_DISK_MAGIC,
-           ARCHIVE_STATE_ANY, "archive_write_disk_uid");
-       if (a->lookup_uid)
-               return (a->lookup_uid)(a->lookup_uid_data, name, id);
-       return (id);
+	struct archive_write_disk *a = (struct archive_write_disk *)_a;
+	archive_check_magic(&a->archive, ARCHIVE_WRITE_DISK_MAGIC,
+		ARCHIVE_STATE_ANY, "archive_write_disk_uid");
+	if (a->lookup_uid)
+		return (a->lookup_uid)(a->lookup_uid_data, name, id);
+	return (id);
 }
 
 /*
@@ -2243,13 +2243,15 @@ guidword(wchar_t *p, int n)
  * Canonicalize the pathname.  In particular, this strips duplicate
  * '\' characters, '.' elements, and trailing '\'.  It also raises an
  * error for an empty path, a trailing '..' or (if _SECURE_NODOTDOT is
- * set) any '..' in the path.
+ * set) any '..' in the path or (if ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS)
+ * if the path is absolute.
  */
 static int
 cleanup_pathname(struct archive_write_disk *a, wchar_t *name)
 {
 	wchar_t *dest, *src, *p, *top;
 	wchar_t separator = L'\0';
+	BOOL absolute_path = 0;
 
 	p = name;
 	if (*p == L'\0') {
@@ -2271,6 +2273,8 @@ cleanup_pathname(struct archive_write_disk *a, wchar_t *name)
 	if (p[0] == L'\\' && p[1] == L'\\' &&
 	    (p[2] == L'.' || p[2] == L'?') && p[3] ==  L'\\')
 	{
+		absolute_path = 1;
+
 		/* A path begin with "\\?\UNC\" */
 		if (p[2] == L'?' &&
 		    (p[4] == L'U' || p[4] == L'u') &&
@@ -2318,9 +2322,10 @@ cleanup_pathname(struct archive_write_disk *a, wchar_t *name)
 			return (ARCHIVE_FAILED);
 		} else
 			p += 4;
-    /* Network drive path like "\\<server-name>\<share-name>\file" */
-    } else if (p[0] == L'\\' && p[1] == L'\\') {
-        p += 2;
+	/* Network drive path like "\\<server-name>\<share-name>\file" */
+	} else if (p[0] == L'\\' && p[1] == L'\\') {
+		absolute_path = 1;
+		p += 2;
 	}
 
 	/* Skip leading drive letter from archives created
@@ -2333,10 +2338,18 @@ cleanup_pathname(struct archive_write_disk *a, wchar_t *name)
 			    "Path is a drive name");
 			return (ARCHIVE_FAILED);
 		}
+
+		absolute_path = 1;
+
 		if (p[2] == L'\\')
 			p += 2;
 	}
 
+	if (absolute_path && (a->flags & ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS)) {
+		archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, "Path is absolute");
+		return (ARCHIVE_FAILED);
+	}
+
 	top = dest = src = p;
 	/* Rewrite the path name if its character is a unusable. */
 	for (; *p != L'\0'; p++) {
@@ -2829,7 +2842,7 @@ set_fflags(struct archive_write_disk *a)
 			return (ARCHIVE_OK);
 		return (set_fflags_platform(a->name, set, clear));
 
-        }
+	}
 	return (ARCHIVE_OK);
 }
 

libarchive/test/CMakeLists.txt

@@ -226,6 +226,7 @@ IF(ENABLE_TEST)
     test_write_disk_no_hfs_compression.c
     test_write_disk_perms.c
     test_write_disk_secure.c
+    test_write_disk_secure_noabsolutepaths.c
     test_write_disk_secure744.c
     test_write_disk_secure745.c
     test_write_disk_secure746.c

libarchive/test/test_write_disk_secure_noabsolutepaths.c

@@ -0,0 +1,102 @@
+/*-
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2024 Mostyn Bramley-Moore <mostyn@antipode.se>
+ */
+
+#include "test.h"
+
+#if defined(_WIN32) && !defined(__CYGWIN__)
+
+#include <fileapi.h>
+
+#define UNLINK _unlink
+
+#else
+
+#include <stdlib.h>
+#include <unistd.h>
+
+#define UNLINK unlink
+
+#endif
+
+/*
+ * Exercise security checks that should prevent writing absolute paths
+ * when extracting archives.
+ */
+DEFINE_TEST(test_write_disk_secure_noabsolutepaths)
+{
+	struct archive *a, *ad;
+	struct archive_entry *ae;
+
+	char buff[10000];
+
+	size_t used;
+
+	// Create an archive_write object.
+	assert((a = archive_write_new()) != NULL);
+	assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a));
+	assertEqualIntA(a, ARCHIVE_OK, archive_write_add_filter_none(a));
+	assertEqualIntA(a, ARCHIVE_OK, archive_write_open_memory(a, buff, sizeof(buff), &used));
+
+#if defined(_WIN32) && !defined(__CYGWIN__)
+	char temp_dir[MAX_PATH + 1];
+	assert(GetTempPathA(MAX_PATH + 1, temp_dir) != 0);
+
+	char temp_file_name[MAX_PATH + 1];
+	char temp_absolute_file_name[MAX_PATH + 1];
+	assert(GetTempFileNameA(temp_dir, "abs", 0, temp_file_name) != 0);
+
+	assert(_fullpath(temp_absolute_file_name, temp_file_name, MAX_PATH) != NULL);
+
+	// Convert to a unix-style path.
+	for (char *p = temp_absolute_file_name; *p != '\0'; p++)
+		if (*p == '\\') *p = '/';
+
+#else
+	char temp_absolute_file_name[] = "/tmp/noabs.testXXXXXX";
+	mkstemp(temp_absolute_file_name);
+#endif
+
+	// Ensure that the target file does not exist.
+	UNLINK(temp_absolute_file_name);
+
+	// Add a regular file entry with an absolute path.
+	assert((ae = archive_entry_new()) != NULL);
+	archive_entry_copy_pathname(ae, temp_absolute_file_name);
+	archive_entry_set_mode(ae, S_IFREG | 0777);
+	archive_entry_set_size(ae, 6);
+	assertEqualIntA(a, ARCHIVE_OK, archive_write_header(a, ae));
+	archive_entry_free(ae);
+	assertEqualInt(6, archive_write_data(a, "hello", 6));
+	assertEqualIntA(a, ARCHIVE_OK, archive_write_close(a));
+	assertEqualInt(ARCHIVE_OK, archive_write_free(a));
+
+	// Now try to extract the data.
+	assert((a = archive_read_new()) != NULL);
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_all(a));
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_support_filter_all(a));
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_open_memory(a, buff, used));
+
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
+	assertEqualString(temp_absolute_file_name, archive_entry_pathname(ae));
+	assertEqualInt(AE_IFREG, archive_entry_filetype(ae));
+	assertEqualInt(AE_IFREG | 0777, archive_entry_mode(ae));
+	assertEqualInt(6, archive_entry_size(ae));
+
+	// This should succeed.
+	assertEqualInt(ARCHIVE_OK, archive_read_extract(a, ae, 0));
+	UNLINK(temp_absolute_file_name);
+
+	// This should fail, since the archive entry has an absolute path.
+	assert(ARCHIVE_OK != archive_read_extract(a, ae, ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS));
+
+	// This should also fail.
+	assert((ad = archive_write_new()) != NULL);
+	assertEqualInt(ARCHIVE_OK, archive_write_disk_set_options(a, ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS));
+	assert(ARCHIVE_OK != archive_read_extract2(a, ae, ad));
+
+	assertEqualInt(ARCHIVE_OK, archive_write_free(ad));
+	assertEqualInt(ARCHIVE_OK, archive_read_free(a));
+}