8336025: Improve ZipOutputSream validation of MAX CEN Header field limits

Reviewed-by: alanb

src/java.base/share/classes/java/util/zip/ZipEntry.java

@@ -78,7 +78,7 @@ public class ZipEntry implements ZipConstants, Cloneable {
     /**
      * Approximately 128 years, in milliseconds (ignoring leap years etc).
      *
-     * This establish an approximate high-bound value for DOS times in
+     * This establishes an approximate high-bound value for DOS times in
      * milliseconds since epoch, used to enable an efficient but
      * sufficient bounds check to avoid generating extended last modified
      * time entries.

src/java.base/share/classes/java/util/zip/ZipOutputStream.java

@@ -378,6 +378,10 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
      * Finishes writing the contents of the ZIP output stream without closing
      * the underlying stream. Use this method when applying multiple filters
      * in succession to the same output stream.
+     * <p>
+     * A ZipException will be thrown if the combined length, after encoding,
+     * of the entry name, the extra field data, the entry comment and
+     * {@linkplain #CENHDR CEN Header size}, exceeds 65,535 bytes.
      * @throws    ZipException if a ZIP file error has occurred
      * @throws    IOException if an I/O exception has occurred
      */
@@ -398,7 +402,9 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
     }
 
     /**
-     * Closes the ZIP output stream as well as the stream being filtered.
+     * Closes the underlying stream and the stream being filtered after
+     * the contents of the ZIP output stream are fully written.
+     *
      * @throws    ZipException if a ZIP file error has occurred
      * @throws    IOException if an I/O error has occurred
      */
@@ -589,7 +595,8 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         writeInt(csize);            // compressed size
         writeInt(size);             // uncompressed size
         byte[] nameBytes = zc.getBytes(e.name);
-        writeShort(nameBytes.length);
+        int nlen = nameBytes.length;
+        writeShort(nlen);
 
         int elen = getExtraLen(e.extra);
         if (hasZip64) {
@@ -626,20 +633,19 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
             }
         }
         writeShort(elen);
-        byte[] commentBytes;
+        byte[] commentBytes = null;
+        int clen = 0;
         if (e.comment != null) {
             commentBytes = zc.getBytes(e.comment);
-            writeShort(Math.min(commentBytes.length, 0xffff));
+            clen = Math.min(commentBytes.length, 0xffff);
-        } else {
-            commentBytes = null;
-            writeShort(0);
         }
+        writeShort(clen);              // file comment length
         writeShort(0);              // starting disk number
         writeShort(0);              // internal file attributes (unused)
         // extra file attributes, used for storing posix permissions etc.
         writeInt(e.externalFileAttributes > 0 ? e.externalFileAttributes << 16 : 0);
         writeInt(offset);           // relative offset of local header
-        writeBytes(nameBytes, 0, nameBytes.length);
+        writeBytes(nameBytes, 0, nlen);
 
         // take care of EXTID_ZIP64 and EXTID_EXTT
         if (hasZip64) {
@@ -679,9 +685,17 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
                 }
             }
         }
+
+        // CEN header size + name length + comment length + extra length
+        // should not exceed 65,535 bytes per the PKWare APP.NOTE
+        // 4.4.10, 4.4.11, & 4.4.12.
+        long headerSize = (long)CENHDR + nlen + clen + elen;
+        if (headerSize > 0xFFFF ) {
+            throw new ZipException("invalid CEN header (bad header size)");
+        }
         writeExtra(e.extra);
         if (commentBytes != null) {
-            writeBytes(commentBytes, 0, Math.min(commentBytes.length, 0xffff));
+            writeBytes(commentBytes, 0, clen);
         }
     }
 

test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -34,9 +34,7 @@ import java.io.*;
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
 import java.nio.channels.FileChannel;
-import java.nio.charset.StandardCharsets;
 import java.time.LocalDateTime;
-import java.util.Arrays;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipException;
 import java.util.zip.ZipFile;
@@ -46,6 +44,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class CenSizeTooLarge {
+
+    // Entry names produced in this test are fixed-length
+    public static final int NAME_LENGTH = 10;
+
     // Maximum allowed CEN size allowed by the ZipFile implementation
     static final int MAX_CEN_SIZE = Integer.MAX_VALUE - ZipFile.ENDHDR - 1;
 
@@ -59,11 +61,10 @@ public class CenSizeTooLarge {
      *        fields respectively.  The combined length of any
      *        directory record and these three fields SHOULD NOT
      *        generally exceed 65,535 bytes.
-     *
+     *.
-     *  Since ZipOutputStream does not enforce the 'combined length' clause,
+     * Create a maximum extra field which does not exceed 65,535 bytes
-     *  we simply use 65,535 (0xFFFF) for the purpose of this test.
      */
-    static final int MAX_EXTRA_FIELD_SIZE = 65_535;
+    static final int MAX_EXTRA_FIELD_SIZE = 65_535 - ZipFile.CENHDR - NAME_LENGTH;
 
     // Data size (unsigned short)
     // Field size minus the leading header 'tag' and 'data size' fields (2 bytes each)
@@ -72,9 +73,6 @@ public class CenSizeTooLarge {
     // Tag for the 'unknown' field type, specified in APPNOTE.txt 'Third party mappings'
     static final short UNKNOWN_ZIP_TAG = (short) 0x9902;
 
-    // Entry names produced in this test are fixed-length
-    public static final int NAME_LENGTH = 10;
-
     // Use a shared LocalDateTime on all entries to save processing time
     static final LocalDateTime TIME_LOCAL = LocalDateTime.now();
 
@@ -84,16 +82,16 @@ public class CenSizeTooLarge {
     // The number of entries needed to exceed the MAX_CEN_SIZE
     static final int NUM_ENTRIES = (MAX_CEN_SIZE / CEN_HEADER_SIZE) + 1;
 
-    // Helps SparseOutputStream detect write of the last CEN entry
-    private static final String LAST_CEN_COMMENT = "LastCEN";
-    private static final byte[] LAST_CEN_COMMENT_BYTES = LAST_CEN_COMMENT.getBytes(StandardCharsets.UTF_8);
-
     // Expected ZipException message when the CEN does not fit in a Java byte array
     private static final String CEN_TOO_LARGE_MESSAGE = "invalid END header (central directory size too large)";
 
     // Zip file to create for testing
     private File hugeZipFile;
 
+    private static final byte[] EXTRA_BYTES = makeLargeExtraField();
+    // Helps SparseOutputStream detect write of the last CEN entry
+    private static final byte[] LAST_EXTRA_BYTES = makeLargeExtraField();
+
     /**
      * Create a zip file with a CEN size which does not fit within a Java byte array
      */
@@ -125,23 +123,18 @@ public class CenSizeTooLarge {
                 // Set the time/date field for faster processing
                 entry.setTimeLocal(TIME_LOCAL);
 
-                if (i == NUM_ENTRIES -1) {
-                    // Help SparseOutputStream detect the last CEN entry write
-                    entry.setComment(LAST_CEN_COMMENT);
-                }
                 // Add the entry
                 zip.putNextEntry(entry);
-
-
             }
             // Finish writing the last entry
             zip.closeEntry();
 
             // Before the CEN headers are written, set the extra data on each entry
-            byte[] extra = makeLargeExtraField();
             for (ZipEntry entry : entries) {
-                entry.setExtra(extra);
+                entry.setExtra(EXTRA_BYTES);
             }
+            // Help SparseOutputSream detect the last entry
+            entries[entries.length-1].setExtra(LAST_EXTRA_BYTES);
         }
     }
 
@@ -165,7 +158,7 @@ public class CenSizeTooLarge {
      * Data Size  (Two byte short)
      * Data Block (Contents depend on field type)
      */
-    private byte[] makeLargeExtraField() {
+    private static byte[] makeLargeExtraField() {
         // Make a maximally sized extra field
         byte[] extra = new byte[MAX_EXTRA_FIELD_SIZE];
         // Little-endian ByteBuffer for updating the header fields
@@ -203,7 +196,7 @@ public class CenSizeTooLarge {
                 // but instead simply advance the position, creating a sparse file
                 channel.position(position);
                 // Check for last CEN record
-                if (Arrays.equals(LAST_CEN_COMMENT_BYTES, 0, LAST_CEN_COMMENT_BYTES.length, b, off, len)) {
+                if (b == LAST_EXTRA_BYTES) {
                     // From here on, write actual bytes
                     sparse = false;
                 }

test/jdk/java/util/zip/ZipOutputStream/ZipOutputStreamMaxCenHdrTest.java

@@ -0,0 +1,169 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8336025
+ * @summary Verify that ZipOutputStream throws a ZipException when the
+ * CEN header size + name length + comment length + extra length exceeds
+ * 65,535 bytes
+ * @run junit ZipOutputStreamMaxCenHdrTest
+ */
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.io.*;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipException;
+import java.util.zip.ZipFile;
+import java.util.zip.ZipOutputStream;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+public class ZipOutputStreamMaxCenHdrTest {
+
+    // CEN header size + name length + comment length + extra length
+    // should not exceed 65,535 bytes per the PKWare APP.NOTE
+    // 4.4.10, 4.4.11, & 4.4.12.
+    static final int MAX_COMBINED_CEN_HEADER_SIZE = 0xFFFF;
+
+    // Maximum possible size of name length + comment length + extra length
+    // for entries in order to not exceed 65,489 bytes minus 46 bytes for the CEN
+    // header length
+    static final int MAX_NAME_COMMENT_EXTRA_SIZE =
+            MAX_COMBINED_CEN_HEADER_SIZE - ZipFile.CENHDR;
+
+    // Tag for the 'unknown' field type, specified in APPNOTE.txt 'Third party mappings'
+    static final short UNKNOWN_ZIP_TAG = (short) 0x9902;
+
+    // ZIP file to be used by the tests
+    static final Path ZIP_FILE = Path.of("maxCENHdrTest.zip");
+
+    /**
+     * Clean up prior to test run
+     *
+     * @throws IOException if an error occurs
+     */
+    @BeforeEach
+    public void startUp() throws IOException {
+        Files.deleteIfExists(ZIP_FILE);
+    }
+
+    /**
+     * Validate a ZipException is thrown when the combined CEN Header, name
+     * length, comment length, and extra data length exceeds 65,535 bytes when
+     * the ZipOutputStream is closed.
+     */
+    @ParameterizedTest
+    @ValueSource(ints = {MAX_COMBINED_CEN_HEADER_SIZE,
+            MAX_COMBINED_CEN_HEADER_SIZE - 1,
+            MAX_NAME_COMMENT_EXTRA_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE - 1})
+    void setCommentTest(int length) throws IOException {
+        boolean expectZipException = length > MAX_NAME_COMMENT_EXTRA_SIZE;
+        final byte[] bytes = new byte[length];
+        Arrays.fill(bytes, (byte) 'a');
+        ZipEntry zipEntry = new ZipEntry("");
+        // The comment length will trigger the ZipException
+        zipEntry.setComment(new String(bytes, StandardCharsets.UTF_8));
+        boolean receivedException = writeZipEntry(zipEntry, expectZipException);
+        assertEquals(receivedException, expectZipException);
+    }
+
+    /**
+     * Validate an ZipException is thrown when the combined CEN Header, name
+     * length, comment length, and extra data length exceeds 65,535 bytes when
+     * the ZipOutputStream is closed.
+     */
+    @ParameterizedTest
+    @ValueSource(ints = {MAX_COMBINED_CEN_HEADER_SIZE,
+            MAX_COMBINED_CEN_HEADER_SIZE - 1,
+            MAX_NAME_COMMENT_EXTRA_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE - 1})
+    void setNameTest(int length) throws IOException {
+        boolean expectZipException = length > MAX_NAME_COMMENT_EXTRA_SIZE;
+        final byte[] bytes = new byte[length];
+        Arrays.fill(bytes, (byte) 'a');
+        // The name length will trigger the ZipException
+        ZipEntry zipEntry = new ZipEntry(new String(bytes, StandardCharsets.UTF_8));
+        boolean receivedException = writeZipEntry(zipEntry, expectZipException);
+        assertEquals(receivedException, expectZipException);
+    }
+
+    /**
+     * Validate an ZipException is thrown when the combined CEN Header, name
+     * length, comment length, and extra data length exceeds 65,535 bytes when
+     * the ZipOutputStream is closed.
+     */
+    @ParameterizedTest
+    @ValueSource(ints = {MAX_COMBINED_CEN_HEADER_SIZE,
+            MAX_COMBINED_CEN_HEADER_SIZE - 1,
+            MAX_NAME_COMMENT_EXTRA_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE - 1})
+    void setExtraTest(int length) throws IOException {
+        boolean expectZipException = length > MAX_NAME_COMMENT_EXTRA_SIZE;
+        final byte[] bytes = new byte[length];
+        // Little-endian ByteBuffer for updating the header fields
+        ByteBuffer buffer = ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN);
+        // We use the 'unknown' tag, specified in APPNOTE.TXT, 4.6.1 Third party mappings'
+        buffer.putShort(UNKNOWN_ZIP_TAG);
+        // Size of the actual (empty) data
+        buffer.putShort((short) (length - 2 * Short.BYTES));
+        ZipEntry zipEntry = new ZipEntry("");
+        // The extra data length will trigger the ZipException
+        zipEntry.setExtra(bytes);
+        boolean receivedException = writeZipEntry(zipEntry, expectZipException);
+        assertEquals(receivedException, expectZipException);
+    }
+
+    /**
+     * Write a single Zip entry using ZipOutputStream
+     * @param zipEntry the ZipEntry to write
+     * @param expectZipException true if a ZipException is expected, false otherwse
+     * @return true if a ZipException was thrown
+     * @throws IOException if an error occurs
+     */
+    private static boolean writeZipEntry(ZipEntry zipEntry, boolean expectZipException)
+            throws IOException {
+        boolean receivedException = false;
+        try (ZipOutputStream zos = new ZipOutputStream(
+                new BufferedOutputStream(Files.newOutputStream(ZIP_FILE)))) {
+            zos.putNextEntry(zipEntry);
+            if (expectZipException) {
+                ZipException ex = assertThrows(ZipException.class, zos::close);
+                assertTrue(ex.getMessage().matches(".*bad header size.*"),
+                        "Unexpected ZipException message: " + ex.getMessage());
+                receivedException = true;
+            }
+        } catch (Exception e) {
+            throw new RuntimeException("Received Unexpected Exception", e);
+        }
+        return receivedException;
+    }
+}