From 1326b7e04f6592195d3bc377ec602fd2e120a336 Mon Sep 17 00:00:00 2001 From: Mack <86566939+Macksaur@users.noreply.github.com> Date: Tue, 6 Dec 2022 17:40:46 +0000 Subject: [PATCH] Fix buffer over-read and memory leaks when using long filepaths in a zip archive and improved robustness of long filepaths and reading files. --- core/io/zip_io.cpp | 50 +++++++++++++++++-- core/io/zip_io.h | 7 +++ modules/zip/doc_classes/ZIPPacker.xml | 3 ++ modules/zip/zip_packer.cpp | 20 ++++---- modules/zip/zip_packer.h | 2 +- modules/zip/zip_reader.cpp | 71 ++++++++++++++++----------- modules/zip/zip_reader.h | 2 +- 7 files changed, 110 insertions(+), 45 deletions(-) diff --git a/core/io/zip_io.cpp b/core/io/zip_io.cpp index 7f600395788..a0e6bd62de8 100644 --- a/core/io/zip_io.cpp +++ b/core/io/zip_io.cpp @@ -30,6 +30,48 @@ #include "zip_io.h" +#include "core/templates/local_vector.h" + +int godot_unzip_get_current_file_info(unzFile p_zip_file, unz_file_info64 &r_file_info, String &r_filepath) { + const uLong short_file_path_buffer_size = 16384ul; + char short_file_path_buffer[short_file_path_buffer_size]; + + int err = unzGetCurrentFileInfo64(p_zip_file, &r_file_info, short_file_path_buffer, short_file_path_buffer_size, nullptr, 0, nullptr, 0); + if (unlikely((err != UNZ_OK) || (r_file_info.size_filename > short_file_path_buffer_size))) { + LocalVector long_file_path_buffer; + long_file_path_buffer.resize(r_file_info.size_filename); + + err = unzGetCurrentFileInfo64(p_zip_file, &r_file_info, long_file_path_buffer.ptr(), long_file_path_buffer.size(), nullptr, 0, nullptr, 0); + if (err != UNZ_OK) { + return err; + } + r_filepath = String::utf8(long_file_path_buffer.ptr(), r_file_info.size_filename); + } else { + r_filepath = String::utf8(short_file_path_buffer, r_file_info.size_filename); + } + + return err; +} + +int godot_unzip_locate_file(unzFile p_zip_file, String p_filepath, bool p_case_sensitive) { + int err = unzGoToFirstFile(p_zip_file); + while (err == UNZ_OK) { + unz_file_info64 current_file_info; + String current_filepath; + err = godot_unzip_get_current_file_info(p_zip_file, current_file_info, current_filepath); + if (err == UNZ_OK) { + bool filepaths_are_equal = p_case_sensitive ? (p_filepath == current_filepath) : (p_filepath.nocasecmp_to(current_filepath) == 0); + if (filepaths_are_equal) { + return UNZ_OK; + } + err = unzGoToNextFile(p_zip_file); + } + } + return err; +} + +// + void *zipio_open(voidpf opaque, const char *p_fname, int mode) { Ref *fa = reinterpret_cast *>(opaque); ERR_FAIL_COND_V(fa == nullptr, nullptr); @@ -38,17 +80,17 @@ void *zipio_open(voidpf opaque, const char *p_fname, int mode) { fname.parse_utf8(p_fname); int file_access_mode = 0; - if (mode & ZLIB_FILEFUNC_MODE_WRITE) { - file_access_mode |= FileAccess::WRITE; - } if (mode & ZLIB_FILEFUNC_MODE_READ) { file_access_mode |= FileAccess::READ; } + if (mode & ZLIB_FILEFUNC_MODE_WRITE) { + file_access_mode |= FileAccess::WRITE; + } if (mode & ZLIB_FILEFUNC_MODE_CREATE) { file_access_mode |= FileAccess::WRITE_READ; } - (*fa) = FileAccess::open(fname, file_access_mode); + (*fa) = FileAccess::open(fname, file_access_mode); if (fa->is_null()) { return nullptr; } diff --git a/core/io/zip_io.h b/core/io/zip_io.h index 094d490bcf6..c59b9813730 100644 --- a/core/io/zip_io.h +++ b/core/io/zip_io.h @@ -39,6 +39,13 @@ #include "thirdparty/minizip/unzip.h" #include "thirdparty/minizip/zip.h" +// Get the current file info and safely convert the full filepath to a String. +int godot_unzip_get_current_file_info(unzFile p_zip_file, unz_file_info64 &r_file_info, String &r_filepath); +// Try to locate the file in the archive specified by the filepath (works with large paths and Unicode). +int godot_unzip_locate_file(unzFile p_zip_file, String p_filepath, bool p_case_sensitive = true); + +// + void *zipio_open(voidpf opaque, const char *p_fname, int mode); uLong zipio_read(voidpf opaque, voidpf stream, void *buf, uLong size); uLong zipio_write(voidpf opaque, voidpf stream, const void *buf, uLong size); diff --git a/modules/zip/doc_classes/ZIPPacker.xml b/modules/zip/doc_classes/ZIPPacker.xml index 47ea50f0317..5d5bedb7739 100644 --- a/modules/zip/doc_classes/ZIPPacker.xml +++ b/modules/zip/doc_classes/ZIPPacker.xml @@ -63,10 +63,13 @@ + Create a new zip archive at the given path. + Append a new zip archive to the end of the already existing file at the given path. + Add new files to the existing zip archive at the given path. diff --git a/modules/zip/zip_packer.cpp b/modules/zip/zip_packer.cpp index 65f451a3f91..c8b4fb4e77f 100644 --- a/modules/zip/zip_packer.cpp +++ b/modules/zip/zip_packer.cpp @@ -39,17 +39,19 @@ Error ZIPPacker::open(String p_path, ZipAppend p_append) { } zlib_filefunc_def io = zipio_create_io(&fa); - zf = zipOpen2(p_path.utf8().get_data(), p_append, NULL, &io); - return zf != NULL ? OK : FAILED; + zf = zipOpen2(p_path.utf8().get_data(), p_append, nullptr, &io); + return zf != nullptr ? OK : FAILED; } Error ZIPPacker::close() { ERR_FAIL_COND_V_MSG(fa.is_null(), FAILED, "ZIPPacker cannot be closed because it is not open."); - Error err = zipClose(zf, NULL) == ZIP_OK ? OK : FAILED; + Error err = zipClose(zf, nullptr) == ZIP_OK ? OK : FAILED; if (err == OK) { - zf = NULL; + DEV_ASSERT(fa == nullptr); + zf = nullptr; } + return err; } @@ -60,18 +62,18 @@ Error ZIPPacker::start_file(String p_path) { OS::DateTime time = OS::get_singleton()->get_datetime(); + zipfi.tmz_date.tm_sec = time.second; + zipfi.tmz_date.tm_min = time.minute; zipfi.tmz_date.tm_hour = time.hour; zipfi.tmz_date.tm_mday = time.day; - zipfi.tmz_date.tm_min = time.minute; zipfi.tmz_date.tm_mon = time.month - 1; - zipfi.tmz_date.tm_sec = time.second; zipfi.tmz_date.tm_year = time.year; zipfi.dosDate = 0; - zipfi.external_fa = 0; zipfi.internal_fa = 0; + zipfi.external_fa = 0; - int ret = zipOpenNewFileInZip(zf, p_path.utf8().get_data(), &zipfi, NULL, 0, NULL, 0, NULL, Z_DEFLATED, Z_DEFAULT_COMPRESSION); - return ret == ZIP_OK ? OK : FAILED; + int err = zipOpenNewFileInZip(zf, p_path.utf8().get_data(), &zipfi, nullptr, 0, nullptr, 0, nullptr, Z_DEFLATED, Z_DEFAULT_COMPRESSION); + return err == ZIP_OK ? OK : FAILED; } Error ZIPPacker::write_file(Vector p_data) { diff --git a/modules/zip/zip_packer.h b/modules/zip/zip_packer.h index d9d2602d333..142d0fddbff 100644 --- a/modules/zip/zip_packer.h +++ b/modules/zip/zip_packer.h @@ -40,7 +40,7 @@ class ZIPPacker : public RefCounted { GDCLASS(ZIPPacker, RefCounted); Ref fa; - zipFile zf; + zipFile zf = nullptr; protected: static void _bind_methods(); diff --git a/modules/zip/zip_reader.cpp b/modules/zip/zip_reader.cpp index 898c36a12dd..2684875e1c5 100644 --- a/modules/zip/zip_reader.cpp +++ b/modules/zip/zip_reader.cpp @@ -40,38 +40,35 @@ Error ZIPReader::open(String p_path) { zlib_filefunc_def io = zipio_create_io(&fa); uzf = unzOpen2(p_path.utf8().get_data(), &io); - return uzf != NULL ? OK : FAILED; + return uzf != nullptr ? OK : FAILED; } Error ZIPReader::close() { ERR_FAIL_COND_V_MSG(fa.is_null(), FAILED, "ZIPReader cannot be closed because it is not open."); - return unzClose(uzf) == UNZ_OK ? OK : FAILED; + Error err = unzClose(uzf) == UNZ_OK ? OK : FAILED; + if (err == OK) { + DEV_ASSERT(fa == nullptr); + uzf = nullptr; + } + + return err; } PackedStringArray ZIPReader::get_files() { ERR_FAIL_COND_V_MSG(fa.is_null(), PackedStringArray(), "ZIPReader must be opened before use."); + int err = unzGoToFirstFile(uzf); + ERR_FAIL_COND_V(err != UNZ_OK, PackedStringArray()); + List s; - - if (unzGoToFirstFile(uzf) != UNZ_OK) { - return PackedStringArray(); - } - do { unz_file_info64 file_info; - char filename[256]; // Note filename is a path ! - int err = unzGetCurrentFileInfo64(uzf, &file_info, filename, sizeof(filename), NULL, 0, NULL, 0); + String filepath; + + err = godot_unzip_get_current_file_info(uzf, file_info, filepath); if (err == UNZ_OK) { - s.push_back(filename); - } else { - // Assume filename buffer was too small - char *long_filename_buff = (char *)memalloc(file_info.size_filename); - int err2 = unzGetCurrentFileInfo64(uzf, NULL, long_filename_buff, sizeof(long_filename_buff), NULL, 0, NULL, 0); - if (err2 == UNZ_OK) { - s.push_back(long_filename_buff); - memfree(long_filename_buff); - } + s.push_back(filepath); } } while (unzGoToNextFile(uzf) == UNZ_OK); @@ -87,23 +84,37 @@ PackedStringArray ZIPReader::get_files() { PackedByteArray ZIPReader::read_file(String p_path, bool p_case_sensitive) { ERR_FAIL_COND_V_MSG(fa.is_null(), PackedByteArray(), "ZIPReader must be opened before use."); - int cs = p_case_sensitive ? 1 : 2; - if (unzLocateFile(uzf, p_path.utf8().get_data(), cs) != UNZ_OK) { - ERR_FAIL_V_MSG(PackedByteArray(), "File does not exist in zip archive: " + p_path); - } - if (unzOpenCurrentFile(uzf) != UNZ_OK) { - ERR_FAIL_V_MSG(PackedByteArray(), "Could not open file within zip archive."); - } + int err = UNZ_OK; + // Locate and open the file. + err = godot_unzip_locate_file(uzf, p_path, p_case_sensitive); + ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "File does not exist in zip archive: " + p_path); + err = unzOpenCurrentFile(uzf); + ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "Could not open file within zip archive."); + + // Read the file info. unz_file_info info; - unzGetCurrentFileInfo(uzf, &info, NULL, 0, NULL, 0, NULL, 0); + err = unzGetCurrentFileInfo(uzf, &info, nullptr, 0, nullptr, 0, nullptr, 0); + ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "Unable to read file information from zip archive."); + ERR_FAIL_COND_V_MSG(info.uncompressed_size > INT_MAX, PackedByteArray(), "File contents too large to read from zip archive (>2 GB)."); + + // Read the file data. PackedByteArray data; data.resize(info.uncompressed_size); + uint8_t *buffer = data.ptrw(); + int to_read = data.size(); + while (to_read > 0) { + int bytes_read = unzReadCurrentFile(uzf, buffer, to_read); + ERR_FAIL_COND_V_MSG(bytes_read < 0, PackedByteArray(), "IO/zlib error reading file from zip archive."); + ERR_FAIL_COND_V_MSG(bytes_read == UNZ_EOF && to_read != 0, PackedByteArray(), "Incomplete file read from zip archive."); + DEV_ASSERT(bytes_read <= to_read); + buffer += bytes_read; + to_read -= bytes_read; + } - uint8_t *w = data.ptrw(); - unzReadCurrentFile(uzf, &w[0], info.uncompressed_size); - - unzCloseCurrentFile(uzf); + // Verify the data and return. + err = unzCloseCurrentFile(uzf); + ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "CRC error reading file from zip archive."); return data; } diff --git a/modules/zip/zip_reader.h b/modules/zip/zip_reader.h index 8227208eedb..c074197eb47 100644 --- a/modules/zip/zip_reader.h +++ b/modules/zip/zip_reader.h @@ -40,7 +40,7 @@ class ZIPReader : public RefCounted { GDCLASS(ZIPReader, RefCounted) Ref fa; - unzFile uzf; + unzFile uzf = nullptr; protected: static void _bind_methods();