8340553: ZipEntry field validation does not take into account the size of a CEN header

Reviewed-by: jpai, redestad, eirbjo

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

@@ -91,7 +91,10 @@ public class ZipEntry implements ZipConstants, Cloneable {
      */
     private static final long UPPER_DOSTIME_BOUND =
             128L * 365 * 24 * 60 * 60 * 1000;
-
+    // 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.
+    private static final int MAX_COMBINED_CEN_HEADER_SIZE = 0xFFFF;
     /**
      * Creates a new ZIP entry with the specified name.
      *
@@ -99,12 +102,12 @@ public class ZipEntry implements ZipConstants, Cloneable {
      *         The entry name
      *
      * @throws NullPointerException if the entry name is null
-     * @throws IllegalArgumentException if the entry name is longer than
+     * @throws IllegalArgumentException if the combined length of the entry name
-     *         0xFFFF bytes
+     * and the {@linkplain #CENHDR CEN Header size} exceeds 65,535 bytes.
      */
     public ZipEntry(String name) {
         Objects.requireNonNull(name, "name");
-        if (name.length() > 0xFFFF) {
+        if (!isCENHeaderValid(name, null, null)) {
             throw new IllegalArgumentException("entry name too long");
         }
         this.name = name;
@@ -519,8 +522,10 @@ public class ZipEntry implements ZipConstants, Cloneable {
      * @param  extra
      *         The extra field data bytes
      *
-     * @throws IllegalArgumentException if the length of the specified
+     * @throws IllegalArgumentException if the combined length of the specified
-     *         extra field data is greater than 0xFFFF bytes
+     * extra field data, the {@linkplain #getName() entry name},
+     * the {@linkplain #getComment() entry comment}, and the
+     * {@linkplain #CENHDR CEN Header size} exceeds 65,535 bytes.
      *
      * @see #getExtra()
      */
@@ -541,7 +546,7 @@ public class ZipEntry implements ZipConstants, Cloneable {
      */
     void setExtra0(byte[] extra, boolean doZIP64, boolean isLOC) {
         if (extra != null) {
-            if (extra.length > 0xFFFF) {
+            if (!isCENHeaderValid(name, extra, comment)) {
                 throw new IllegalArgumentException("invalid extra field length");
             }
             // extra fields are in "HeaderID(2)DataSize(2)Data... format
@@ -642,16 +647,19 @@ public class ZipEntry implements ZipConstants, Cloneable {
 
     /**
      * Sets the optional comment string for the entry.
-     *
-     * <p>ZIP entry comments have maximum length of 0xffff. If the length of the
-     * specified comment string is greater than 0xFFFF bytes after encoding, only
-     * the first 0xFFFF bytes are output to the ZIP file entry.
-     *
      * @param comment the comment string
-     *
+     * @throws IllegalArgumentException if the combined length
+     * of the specified entry comment, the {@linkplain #getName() entry name},
+     * the {@linkplain #getExtra() extra field data}, and the
+     * {@linkplain #CENHDR CEN Header size} exceeds 65,535 bytes.
      * @see #getComment()
      */
     public void setComment(String comment) {
+        if (comment != null) {
+            if (!isCENHeaderValid(name, extra, comment)) {
+                throw new IllegalArgumentException("entry comment too long");
+            }
+        }
         this.comment = comment;
     }
 
@@ -702,4 +710,22 @@ public class ZipEntry implements ZipConstants, Cloneable {
             throw new InternalError(e);
         }
     }
+
+    /**
+     * Initial validation that the CEN header size + name length + comment length
+     * + extra length do not exceed 65,535 bytes per the PKWare APP.NOTE
+     * 4.4.10, 4.4.11, & 4.4.12.   Prior to writing out the CEN Header,
+     * ZipOutputStream::writeCEN will do an additional validation  of the combined
+     * length of the fields after encoding the name and comment to a byte array.
+     * @param name Zip entry name
+     * @param extra Zip extra data
+     * @param comment Zip entry comment
+     * @return true if valid CEN Header size; false otherwise
+     */
+     static boolean isCENHeaderValid(String name, byte[] extra, String comment) {
+        int clen = comment == null ? 0 : comment.length();
+        int elen = extra == null ? 0 : extra.length;
+        long headerSize = (long)CENHDR + name.length() + clen + elen;
+        return headerSize <= MAX_COMBINED_CEN_HEADER_SIZE;
+    }
 }

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

@@ -32,6 +32,7 @@ import java.util.Objects;
 import java.util.Vector;
 import java.util.HashSet;
 import static java.util.zip.ZipConstants64.*;
+import static java.util.zip.ZipEntry.isCENHeaderValid;
 import static java.util.zip.ZipUtils.*;
 import sun.nio.cs.UTF_8;
 import sun.security.action.GetBooleanAction;
@@ -262,6 +263,12 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         }
         if (zc.isUTF8())
             e.flag |= USE_UTF8;
+        // 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.
+        if (!isCENHeaderValid(e.name, e.extra, e.comment) ) {
+            throw new ZipException("invalid CEN header (bad header size)");
+        }
         current = new XEntry(e, written);
         xentries.add(current);
         writeLOC(current);
@@ -602,6 +609,22 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         if (hasZip64) {
             elen += (elenZIP64 + 4);// + headid(2) + datasize(2)
         }
+
+        int clen = 0;
+        byte[] commentBytes = null;
+        if (e.comment != null) {
+            commentBytes = zc.getBytes(e.comment);
+            clen = commentBytes.length;
+        }
+
+        // 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)");
+        }
+
         // cen info-zip extended timestamp only outputs mtime
         // but set the flag for a/ctime, if present in loc
         int flagEXTT = 0;
@@ -633,12 +656,6 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
             }
         }
         writeShort(elen);
-        byte[] commentBytes = null;
-        int clen = 0;
-        if (e.comment != null) {
-            commentBytes = zc.getBytes(e.comment);
-            clen = Math.min(commentBytes.length, 0xffff);
-        }
         writeShort(clen);              // file comment length
         writeShort(0);              // starting disk number
         writeShort(0);              // internal file attributes (unused)
@@ -686,13 +703,6 @@ 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, clen);

test/jdk/java/util/zip/ZipEntry/MaxZipEntryFieldSizeTest.java

@@ -0,0 +1,174 @@
+/*
+ * 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 8340553
+ * @summary Verify that ZipEntry(String), ZipEntry::setComment, and
+ * ZipEntry::setExtra throws a IllegalArgumentException when the
+ * combined length of the fields, including the size of the CEN Header,
+ * exceeds 65,535 bytes
+ * @run junit MaxZipEntryFieldSizeTest
+ */
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class MaxZipEntryFieldSizeTest {
+
+    // 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
+    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 Entry name used by tests
+    static final String ENTRY_NAME = "EntryName";
+    // Max length minus the size of the ENTRY_NAME or ENTRY_COMMENT
+    static final int MAX_FIELD_LEN_MINUS_ENTRY_NAME =
+            MAX_NAME_COMMENT_EXTRA_SIZE - 9;
+
+    /**
+     * Validate an IllegalArgumentException is thrown when the
+     * combined length of the entry name, entry comment, entry extra data,
+     * and CEN Header size exceeds 65,535 bytes.
+     */
+    @ParameterizedTest
+    @ValueSource(ints = {30000, 35000})
+    void combinedLengthTest(int length) {
+        String comment = "a".repeat(length);
+        byte[] bytes = creatExtraData(length);
+        int combinedLength = ENTRY_NAME.length() + comment.length() + bytes.length;
+        boolean expectException = combinedLength > MAX_COMBINED_CEN_HEADER_SIZE;
+        System.out.printf("Combined Len= %s, exception: %s%n", combinedLength, expectException);
+        ZipEntry zipEntry = new ZipEntry(ENTRY_NAME);
+        zipEntry.setComment(comment);
+        // The extra data length will trigger the IllegalArgumentException
+        if (expectException) {
+            assertThrows(IllegalArgumentException.class, () ->
+                    zipEntry.setExtra(bytes));
+        } else {
+            zipEntry.setExtra(bytes);
+        }
+    }
+
+    /**
+     * Validate an IllegalArgumentException is thrown when the comment
+     * length exceeds 65,489 bytes.
+     */
+    @ParameterizedTest
+    @ValueSource(ints = {MAX_COMBINED_CEN_HEADER_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE + 1,
+            MAX_FIELD_LEN_MINUS_ENTRY_NAME,
+            MAX_FIELD_LEN_MINUS_ENTRY_NAME - 1})
+    void setCommentLengthTest(int length) {
+        boolean expectException = length >= MAX_NAME_COMMENT_EXTRA_SIZE;
+        ZipEntry zipEntry = new ZipEntry(ENTRY_NAME);
+        String comment = "a".repeat(length);
+        System.out.printf("Comment Len= %s, exception: %s%n", comment.length(), expectException);
+        // The comment length will trigger the IllegalArgumentException
+        if (expectException) {
+            assertThrows(IllegalArgumentException.class, () ->
+                    zipEntry.setComment(comment));
+        } else {
+            zipEntry.setComment(comment);
+        }
+    }
+
+    /**
+     * Validate an IllegalArgumentException is thrown when the name
+     * length exceeds 65,489 bytes.
+     */
+    @ParameterizedTest
+    @ValueSource(ints = {MAX_COMBINED_CEN_HEADER_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE + 1,
+            MAX_FIELD_LEN_MINUS_ENTRY_NAME,
+            MAX_FIELD_LEN_MINUS_ENTRY_NAME - 1})
+    void nameLengthTest(int length) {
+        boolean expectException = length > MAX_NAME_COMMENT_EXTRA_SIZE;
+        String name = "a".repeat(length);
+        System.out.printf("name Len= %s, exception: %s%n", name.length(), expectException);
+        // The name length will trigger the IllegalArgumentException
+        if (expectException) {
+            assertThrows(IllegalArgumentException.class, () -> new ZipEntry(name));
+        } else {
+            new ZipEntry(name);
+        }
+    }
+
+    /**
+     * Validate an IllegalArgumentException is thrown when the extra data
+     * length exceeds 65,489 bytes.
+     */
+    @ParameterizedTest
+    @ValueSource(ints = {MAX_COMBINED_CEN_HEADER_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE,
+            MAX_NAME_COMMENT_EXTRA_SIZE + 1,
+            MAX_FIELD_LEN_MINUS_ENTRY_NAME,
+            MAX_FIELD_LEN_MINUS_ENTRY_NAME - 1})
+    void setExtraLengthTest(int length) {
+        boolean expectException = length >= MAX_NAME_COMMENT_EXTRA_SIZE;
+        byte[] bytes = creatExtraData(length);
+        ZipEntry zipEntry = new ZipEntry(ENTRY_NAME);
+        System.out.printf("extra Len= %s, exception: %s%n", bytes.length, expectException);
+        // The extra data length will trigger the IllegalArgumentException
+        if (expectException) {
+            assertThrows(IllegalArgumentException.class, () -> zipEntry.setExtra(bytes));
+        } else {
+            zipEntry.setExtra(bytes);
+        }
+    }
+
+    /**
+     * Create the extra field data which will be passed to ZipEntry::setExtra
+     * @param length size of the extra data
+     * @return byte array containing the extra data
+     */
+    private static byte[] creatExtraData(int length) {
+        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));
+        return bytes;
+    }
+}

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

@@ -1,169 +0,0 @@
-/*
- * 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;
-    }
-}