Issue #1055: Ignore padding in Zip extra field data

PKWare's APPNOTE.TXT simply documents the extra field
as a concatenation of <header><data> objects.  It does
not explicitly allow nor prohibit additional padding.

Libarchive originally treated any extraneous data in the
extra field as an indication of corruption and issued an
error.

However, some Zip writers do appear to include additional zero
bytes in the extra field and expect them to be ignored.
This works if:

 * We consider a field type of 0000 to be a padding field.
 * In particular, 0000 0000 is then a valid field with type zero
   and zero bytes of data.
 * If there are fewer than 4 bytes left at the end of the extra
   data (too few to be a valid extra field object), ignore it.

With the above conventions, zero padding can occur anywhere in
the extra field as long as the padding occurs in a multiple of
four bytes.  Any number of bytes can occur at the end:  groups
of four bytes will be skipped as valid empty fields; any final
group of less than four bytes will be simply ignored.

The test exercises all of the above options; the extra field data for
the file in this archive has:
 * Four zero bytes at the beginning
 * A valid UX field
 * A pad field of type zero with a non-zero length
 * An additional three final bytes

By verifying that the UX data was read correctly and that there
were no errors, we can be confident that all of the padding here
was handled correctly.

diff --git a/Makefile.am b/Makefile.am
index 80a4e17d1b719694b706b61abc283634d71c6cd0..ea6b2dc00ee7eaa6f648eb62eed3108e40f3e2f3 100644 (file)
--- a/Makefile.am
+++ b/Makefile.am
@@ -512,6 +512,7 @@ libarchive_test_SOURCES= \
        libarchive/test/test_read_format_zip_encryption_data.c \
        libarchive/test/test_read_format_zip_encryption_partially.c \
        libarchive/test/test_read_format_zip_encryption_header.c \
+       libarchive/test/test_read_format_zip_extra_padding.c \
        libarchive/test/test_read_format_zip_filename.c \
        libarchive/test/test_read_format_zip_high_compression.c \
        libarchive/test/test_read_format_zip_jar.c \
@@ -839,6 +840,7 @@ libarchive_test_EXTRA_DIST=\
        libarchive/test/test_read_format_zip_encryption_data.zip.uu \
        libarchive/test/test_read_format_zip_encryption_header.zip.uu \
        libarchive/test/test_read_format_zip_encryption_partially.zip.uu \
+       libarchive/test/test_read_format_zip_extra_padding.zip.uu \
        libarchive/test/test_read_format_zip_filename_cp866.zip.uu \
        libarchive/test/test_read_format_zip_filename_cp932.zip.uu \
        libarchive/test/test_read_format_zip_filename_koi8r.zip.uu \

diff --git a/libarchive/archive_read_support_format_zip.c b/libarchive/archive_read_support_format_zip.c
index 420004dbabbc3a7c92c411b655aca0a657e2d37b..55752084c8a704a559238c5687d1e2e2d9e04bdc 100644 (file)
--- a/libarchive/archive_read_support_format_zip.c
+++ b/libarchive/archive_read_support_format_zip.c
@@ -750,12 +750,6 @@ process_extra(struct archive_read *a, const char *p, size_t extra_length, struct
                }
                offset += datasize;
        }
-       if (offset != extra_length) {
-               archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT,
-                   "Malformed extra data: Consumed %d bytes of %d bytes",
-                   (int)offset, (int)extra_length);
-               return ARCHIVE_FAILED;
-       }
        return ARCHIVE_OK;
 }
 

diff --git a/libarchive/test/CMakeLists.txt b/libarchive/test/CMakeLists.txt
index 3927748576c6a56e44d96e67240cebae702f876b..b18fc8a88401e00e4d71ccb7e90eb2c1a9a51886 100644 (file)
--- a/libarchive/test/CMakeLists.txt
+++ b/libarchive/test/CMakeLists.txt
@@ -170,6 +170,7 @@ IF(ENABLE_TEST)
     test_read_format_zip_encryption_data.c
     test_read_format_zip_encryption_header.c
     test_read_format_zip_encryption_partially.c
+    test_read_format_zip_extra_padding.c
     test_read_format_zip_filename.c
     test_read_format_zip_high_compression.c
     test_read_format_zip_jar.c

diff --git a/libarchive/test/test_read_format_zip_extra_padding.c b/libarchive/test/test_read_format_zip_extra_padding.c
new file mode 100644 (file)
index 0000000..8dbb0c5
--- /dev/null
+++ b/libarchive/test/test_read_format_zip_extra_padding.c
@@ -0,0 +1,93 @@
+/*-
+ * Copyright (c) 2003-2018 Tim Kientzle
+ * 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 archive verifies that we ignore padding in the extra field.
+ *
+ * APPNOTE.txt does not provide any provision for padding the extra
+ * field, so libarchive used to error when there were unconsumed
+ * bytes.  Apparently, some Zip writers do routinely zero padding
+ * in the extra field.
+ *
+ * The extra fields in this test (for both the local file header
+ * and the central directory entry) are formatted as follows:
+ *
+ *   0000 0000 - unrecognized field with type zero, zero bytes
+ *   5554 0900 03d258155cdb58155c - UX field with length 9
+ *   0000 0400 00000000 - unrecognized field with type zero, four bytes
+ *   000000 - three bytes padding
+ *
+ * The two valid type zero fields should be skipped and ignored, as
+ * should the three bytes padding (which is too short to be a valid
+ * extra data object).  If there were no errors and we read the UX
+ * field correctly, then we've correctly handled all of the padding
+ * fields above.
+ */
+
+
+static void verify(struct archive *a) {
+       struct archive_entry *ae;
+
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
+       assertEqualString("a", archive_entry_pathname(ae));
+       assertEqualInt(AE_IFREG | 0664, archive_entry_mode(ae));
+       assertEqualInt(0x5c1558db, archive_entry_mtime(ae));
+       assertEqualInt(0, archive_entry_ctime(ae));
+       assertEqualInt(0x5c1558db, archive_entry_atime(ae));
+
+       assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header(a, &ae));
+}
+
+DEFINE_TEST(test_read_format_zip_extra_padding)
+{
+       const char *refname = "test_read_format_zip_extra_padding.zip";
+       struct archive *a;
+       char *p;
+       size_t s;
+
+       extract_reference_file(refname);
+
+       /* Verify with seeking reader. */
+       assert((a = archive_read_new()) != NULL);
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_support_filter_all(a));
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_all(a));
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_open_filename(a, refname, 7));
+       verify(a, 0);
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_close(a));
+       assertEqualInt(ARCHIVE_OK, archive_read_free(a));
+
+       /* Verify with streaming reader. */
+       p = slurpfile(&s, refname);
+       assert((a = archive_read_new()) != NULL);
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_support_filter_all(a));
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_all(a));
+       assertEqualIntA(a, ARCHIVE_OK, read_open_memory(a, p, s, 3));
+       verify(a, 1);
+       assertEqualIntA(a, ARCHIVE_OK, archive_read_close(a));
+       assertEqualInt(ARCHIVE_OK, archive_read_free(a));
+
+       free(p);
+}

diff --git a/libarchive/test/test_read_format_zip_extra_padding.zip.uu b/libarchive/test/test_read_format_zip_extra_padding.zip.uu
new file mode 100644 (file)
index 0000000..496bd3c
--- /dev/null
+++ b/libarchive/test/test_read_format_zip_extra_padding.zip.uu
@@ -0,0 +1,7 @@
+begin 644 test_read_format_zip_extra_padding.zip
+M4$L#!`H``````"-=CTW$\L?V`@````(````!`!P`80````!55`D``])8%5S;
+M6!5<```$``````````!B"E!+`0(>`PH``````"-=CTW$\L?V`@````(````!
+M`!@```````$```"D@0````!A`````%54"0`#TE@57-M8%5P```0``````$L%
+3!@`````!``$`1P```#T`````````
+`
+end