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

[Lance Andersen]

On Fri, 18 Oct 2024 13:24:08 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:
>> 
>>   Added additional validation to ZipEntry
>
> src/java.base/share/classes/java/util/zip/ZipEntry.java line 106:
> 
>> 104:      * @throws NullPointerException if the entry name is null
>> 105:      * @throws IllegalArgumentException if the combined length of the entry name
>> 106:      * and {@linkplain #CENHDR CEN Header size} exceeds 65,535 bytes.
> 
> English is not my native language, but should 'CEN header size" be preceded by the definite article "the", here and in setExtra/setComment ?

Not really needed but added anyways

> src/java.base/share/classes/java/util/zip/ZipEntry.java line 652:
> 
>> 650:      * @param comment the comment string
>> 651:      * @throws IllegalArgumentException if the combined length
>> 652:      * of the specified entry comment, {@linkplain #getName() entry name},
> 
> 'entry name' is preceded by the definite article 'the' in `setExtra`, but not here. Would be good to be consistent.

Done

> src/java.base/share/classes/java/util/zip/ZipEntry.java line 723:
> 
>> 721:      * @return true if valid CEN Header size; false otherwise
>> 722:      */
>> 723:     private static  boolean isCENHeaderValid(String name, byte[] extra, String comment) {
> 
> Extra space before `boolean`
> 
> Suggestion:
> 
>     private static boolean isCENHeaderValid(String name, byte[] extra, String comment) {

fixed

> src/java.base/share/classes/java/util/zip/ZipEntry.java line 726:
> 
>> 724:         int clen = comment == null ? 0 : comment.length();
>> 725:         int elen = extra == null ? 0 : extra.length;
>> 726:         int headerSize = CENHDR + name.length() + clen + elen;
> 
> `headerSize` may overflow `int`, here and in `ZipOutputStream`

Changed to 
`long headerSize = (long)CENHDR + name.length() + clen + elen;`

> src/java.base/share/classes/java/util/zip/ZipEntry.java line 727:
> 
>> 725:         int elen = extra == null ? 0 : extra.length;
>> 726:         int headerSize = CENHDR + name.length() + clen + elen;
>> 727:         return headerSize <= MAX_COMBINED_CEN_HEADER_SIZE ? true : false;
> 
> Unless this was a stylistic choice, this might be simpler:
> 
> Suggestion:
> 
>         return headerSize <= MAX_COMBINED_CEN_HEADER_SIZE;

done

> src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 268:
> 
>> 266:         // should not exceed 65,535 bytes per the PKWare APP.NOTE
>> 267:         // 4.4.10, 4.4.11, & 4.4.12.
>> 268:         int clen = e.comment == null ? 0 : e.comment.length();
> 
> If `ZipEntry.isCENHeaderValid` was package-protected, it seems you could use it from here, like:
> 
> `ZipEntry.isCENHeaderValid(e.name, e.extra, e.comment)`

done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806507539
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806507648
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806509705
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806509453
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806508512
PR Review Comment: https://git.openjdk.org/jdk/pull/21544#discussion_r1806507936