8341625: Improve ZipFile validation of the END header

[eirbjo]

Please review this PR which adds validation of the 'total entries' value when fetched from the 'ZIP64 End of Central Directory' header.

We should reject this value under the following conditions:

1. It is too large to fit within the specified CEN size (considering each CEN header encodes as at least 46 bytes each)
2. It is too large to create the int[] entries array safely (max value is ArraysSupport.SOFT_MAX_ARRAY_LENGTH / 3)

I claim that condition 2 here is already implicitly validated by the current maximum CEN size validation. (A CEN encoding such a large number of entries would exceed the maximum CEN size a lot and would already be rejected)

This change aims to protect the integrity of the implementation against specially crafted ZIP files. No sane ZIP tool will produce such files.

Testing:

This PR adds a test EndOfCenValidation.shouldRejectBadTotalEntries which exercises the first condition above.

ZIP tests run locally. GHA results pending.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8341625: Improve ZipFile validation of the END header (Enhancement - P4)

Reviewers

• Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21384/head:pull/21384
$ git checkout pull/21384

Update a local copy of the PR:
$ git checkout pull/21384
$ git pull https://git.openjdk.org/jdk.git pull/21384/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21384

View PR using the GUI difftool:
$ git pr show -t 21384

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21384.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@eirbjo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8341625: Improve ZipFile validation of the END header

Reviewed-by: lancea

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 122 new commits pushed to the master branch:

• ff2f39f: 8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop
• ecc77a5: 8336702: C2 compilation fails with "all memory state should have been processed" assert
• d936556: 8341633: StatSampler::assert_system_property: Print the keys and values of the assert
• 3fba170: 8340786: Introduce Predicate classes with predicate iterators and visitors for simplified walking
• 047c2d7: 8341141: Optimize DirectCodeBuilder
• d636e0d: 8341688: Aarch64: Generate comments in -XX:+PrintInterpreter to link to source code
• d3f3c6a: 8330157: C2: Add a stress flag for bailouts
• d809bc0: 8341658: RISC-V: Test DateFormatProviderTest.java run timeouted
• de90204: 8341588: Remove CollectionUsageThreshold.java from ProblemList-Xcomp for debugging
• f276f58: 8341803: ProblemList containers/docker/TestJcmdWithSideCar.java on linux-x64
• ... and 112 more: https://git.openjdk.org/jdk/compare/03149735e59b7d1d409a6e29ee05ae0537e03d53...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@eirbjo The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (0f0801c4)
• 01: Full - Incremental (ccfa56e4)
• 00: Full (8e8cb818)

[LanceAndersen]

It might be beneficial to future readers to provide the steps used to create the hex string in the format you have provided.

[eirbjo]

It might be beneficial to future readers to provide the steps used to create the hex string in the format you have provided.

The test vector was crafted using a ZIP test library not available in OpenJDK.

I have replaced that with a test vector which is easily reproduced using InfoZIP. Take a look at the updated test.

[eirbjo]

@LanceAndersen

I figured it would make sense to also reject "negative" values. These are not actually negative, just values larger than Long.MAX_VALUE can represent.

See the 11a275d for the conversion to JUnit 5 and 0f0801c which makes the test parameterized to verify that a bunch of bad entry count values are rejected.

[LanceAndersen]

@LanceAndersen

I figured it would make sense to also reject "negative" values. These are not actually negative, just values larger than Long.MAX_VALUE can represent.

See the 11a275d for the conversion to JUnit 5 and 0f0801c which makes the test parameterized to verify that a bunch of bad entry count values are rejected.

No harm, with the extra validation IMHO

[eirbjo]

/integrate

[openjdk]

Going to push as commit 950e3a7.
Since your change was applied there have been 124 commits pushed to the master branch:

• e704c05: 8340547: Starting many threads can delay safepoints
• c30ad01: 8325495: C2: implement optimization for series of Add of unique value
• ff2f39f: 8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop
• ecc77a5: 8336702: C2 compilation fails with "all memory state should have been processed" assert
• d936556: 8341633: StatSampler::assert_system_property: Print the keys and values of the assert
• 3fba170: 8340786: Introduce Predicate classes with predicate iterators and visitors for simplified walking
• 047c2d7: 8341141: Optimize DirectCodeBuilder
• d636e0d: 8341688: Aarch64: Generate comments in -XX:+PrintInterpreter to link to source code
• d3f3c6a: 8330157: C2: Add a stress flag for bailouts
• d809bc0: 8341658: RISC-V: Test DateFormatProviderTest.java run timeouted
• ... and 114 more: https://git.openjdk.org/jdk/compare/03149735e59b7d1d409a6e29ee05ae0537e03d53...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eirbjo Pushed as commit 950e3a7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.