From 2fd35b0fdb517ef4c78f779be4bba019f5b27a86 Mon Sep 17 00:00:00 2001 From: Mostyn Bramley-Moore Date: Fri, 4 Oct 2024 17:36:56 +0200 Subject: [PATCH] Fix ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS on windows (#2363) --- Makefile.am | 2 + cpio/test/CMakeLists.txt | 1 + cpio/test/test_extract_cpio_absolute_paths.c | 67 ++++++++++++ libarchive/archive_write_disk_windows.c | 47 +++++--- libarchive/test/CMakeLists.txt | 1 + .../test_write_disk_secure_noabsolutepaths.c | 102 ++++++++++++++++++ 6 files changed, 203 insertions(+), 17 deletions(-) create mode 100644 cpio/test/test_extract_cpio_absolute_paths.c create mode 100644 libarchive/test/test_write_disk_secure_noabsolutepaths.c diff --git a/Makefile.am b/Makefile.am index c978508d0..64af9afd9 100644 --- a/Makefile.am +++ b/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 \ diff --git a/cpio/test/CMakeLists.txt b/cpio/test/CMakeLists.txt index 2c3fbb0a8..dfbf48679 100644 --- a/cpio/test/CMakeLists.txt +++ b/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 diff --git a/cpio/test/test_extract_cpio_absolute_paths.c b/cpio/test/test_extract_cpio_absolute_paths.c new file mode 100644 index 000000000..e7a3a8a5a --- /dev/null +++ b/cpio/test/test_extract_cpio_absolute_paths.c @@ -0,0 +1,67 @@ +/*- + * SPDX-License-Identifier: BSD-2-Clause + * + * Copyright (c) 2024 Mostyn Bramley-Moore + */ + +#include "test.h" + +#include + +#if defined(_WIN32) && !defined(__CYGWIN__) + +#include + +#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); +} diff --git a/libarchive/archive_write_disk_windows.c b/libarchive/archive_write_disk_windows.c index 774151ae2..f0de73131 100644 --- a/libarchive/archive_write_disk_windows.c +++ b/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 "\\\\file" */ - } else if (p[0] == L'\\' && p[1] == L'\\') { - p += 2; + /* Network drive path like "\\\\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); } diff --git a/libarchive/test/CMakeLists.txt b/libarchive/test/CMakeLists.txt index 4b7e21fc2..7aaa58379 100644 --- a/libarchive/test/CMakeLists.txt +++ b/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 diff --git a/libarchive/test/test_write_disk_secure_noabsolutepaths.c b/libarchive/test/test_write_disk_secure_noabsolutepaths.c new file mode 100644 index 000000000..5782d21ea --- /dev/null +++ b/libarchive/test/test_write_disk_secure_noabsolutepaths.c @@ -0,0 +1,102 @@ +/*- + * SPDX-License-Identifier: BSD-2-Clause + * + * Copyright (c) 2024 Mostyn Bramley-Moore + */ + +#include "test.h" + +#if defined(_WIN32) && !defined(__CYGWIN__) + +#include + +#define UNLINK _unlink + +#else + +#include +#include + +#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)); +} -- 2.47.2