potential invalid filename/comment buffer access in zipCloseFileInZipRaw*()

When creating a new zip entry via zipOpenNewFileInZip*()/zipWriteInFileInZip()/zipCloseFileInZipRaw*() compatibility API trio, the filename pointer passed in zipOpenNewFileInZip*() gets copied to a local structure, that is kept across calls (with the filename pointer in it), and then used again in the close operation. The problem arises when said buffer is no longer there (has different content/has been freed) at the time close is called. In such case, close will access an invalid pointer, calculates strlen() on said pointer and writes that invalid buffer content to the central zip directory. The same is true for the per-file comment pointer.

An explicit requirement to retain buffers between entry creation and close may be a solution, but the original minizip API didn't have such requirement, so the compatible (and also API-user-friendly) solution seems to be making a copy of both filename and comment buffers, and keep those pointers in memory till close is called (and free the buffers on close). This allows the caller to release those buffers right after the zipOpenNewFileInZip*() call, just like with the original minizip lib.

Here's a rough patch to fix this (also for the MZ API):

--- a/mz_zip.c
+++ b/mz_zip.c
@@ -1437,6 +1437,28 @@ extern int32_t mz_zip_entry_write_open(void *handle, const mz_zip_file *file_inf
     if (err == MZ_OK)
         err = mz_zip_entry_open_int(handle, raw, compress_level, password);
 
+    zip->file_info.filename = NULL;
+    zip->file_info.comment = NULL;
+
+    if (err == MZ_OK)
+    {
+        size_t len;
+        if (file_info->filename != NULL)
+        {
+            len = strlen(file_info->filename);
+            zip->file_info.filename = (char *)MZ_ALLOC(len + 1);
+            memset(zip->file_info.filename, 0, len + 1);
+            memcpy(zip->file_info.filename, file_info->filename, len);
+        }
+        if (file_info->comment != NULL)
+        {
+            len = strlen(file_info->comment);
+            zip->file_info.comment = (char *)MZ_ALLOC(len + 1);
+            memset(zip->file_info.comment, 0, len + 1);
+            memcpy(zip->file_info.comment, file_info->comment, len);
+        }
+    }
+
     return err;
 }
 
@@ -1579,6 +1601,17 @@ extern int32_t mz_zip_entry_close_raw(void *handle, uint64_t uncompressed_size,
         zip->number_entry += 1;
     }
 
+    if (zip->file_info.filename != NULL)
+    {
+        MZ_FREE(zip->file_info.filename);
+        zip->file_info.filename = NULL;
+    }
+    if (zip->file_info.comment != NULL)
+    {
+        MZ_FREE(zip->file_info.comment);
+        zip->file_info.comment = NULL;
+    }
+
     zip->entry_opened = 0;
 
     return err;

Patch issues:

filename/comment can now be NULL if the open operation didn't succeed. API bits will need to made prepared for that. (Or, allocate early, but in such case caller may not call close, so freeing them needs to be done somewhere else.)
make a better effort at releasing these buffers, e.g. along with other freed objects, possibly without the user having to call close at all.

Fixed potential invalid filename/comment buffer. #295

Alright I made a change. Let me know if that works for you.

Thank you! mz_stream_write_chars()/mz_stream_write() calls assigns the size written to err, making subsequent instructions fail. After removing those three assignments, the issue is fixed (and much more elegantly so than with my rough patch.)

e.g.:

err = mz_stream_write_chars(zip->file_info_stream, file_info->filename, 1);

→

mz_stream_write_chars(zip->file_info_stream, file_info->filename, 1);

BTW, there is a similar value check issue in this existing call in mz_zip.c:

if (mz_stream_write(stream, file_info->comment, file_info->comment_size) != MZ_OK)

this should fix it:

if (mz_stream_write(stream, file_info->comment, file_info->comment_size) != file_info->comment_size)

Fixed wrong check for writing to stream. #295

I have made the changes. Nice find on the bugs. Thanks for helping make this software better.

You're welcome and no worries! My thanks for your hard work on the revitalised minizip — it's very useful and nicely executed.

Navigation Menu

Repository navigation

potential invalid filename/comment buffer access in zipCloseFileInZipRaw*() #295

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

potential invalid filename/comment buffer access in zipCloseFileInZipRaw*() #295

Description

Activity

vszakats commented on Aug 14, 2018

nmoinvaz commented on Aug 15, 2018

vszakats commented on Aug 15, 2018

nmoinvaz commented on Aug 15, 2018

vszakats commented on Aug 15, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions