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

[Lance Andersen]

On Thu, 17 Oct 2024 11:28:33 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> Lance Andersen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add back missing putNextEntry call
>
> src/java.base/share/classes/java/util/zip/ZipEntry.java line 97:
> 
>> 95:     // for entries in order to not exceed 65,489 bytes minus 46 bytes for the CEN
>> 96:     // header length
>> 97:     private static final int MAX_NAME_COMMENT_EXTRA_SIZE =
> 
> Would it make sense to validate the actual combined length here, instead of just pretending that all other components of the combined lenghts are zero-length?
> 
> We could introduce a method like this:
> 
> 
> /* 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 void checkCombinedLength(String name, byte[] extra, String comment, String message) {
>     
>     int elen = extra == null ? 0 : extra.length;
>     int clen = comment == null ? 0 : comment.length();
> 
>     long headerLength = ZipEntry.CENHDR + name.length() + elen + clen;
> 
>     if (headerLength > 0xFFFF) {
>         throw new IllegalArgumentException(message);
>     }
> }
> 
> 
> 
> Then the constructor could use it like this:
> 
> checkCombinedLength(name, null, null, "entry name too long");
> 
> 
> setExtra:
> 
> 
> checkCombinedLength(name, extra, comment, "invalid extra field length");
> 
> 
> setComment:
> 
> checkCombinedLength(name, extra, comment, "entry comment too long");
> 
> 
> 
> With this solution, I suppose`ZipEntry` enforces the invariant fully, and repeating the validation in `ZipOutputStream` should be unneccessary.
> 
> Or did I miss something?

I had thought about that and decided to keep the changes as they are.  I am not opposed to revisiting this in a follow on PR.  Any additional changes would require more javadoc updates to address the overall change in validation. 

So after we fork JDK 24, happyt to revisit.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1804839093