Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8314891: Additional Zip64 extra header validation #15650

Closed
wants to merge 9 commits into from

Conversation

LanceAndersen
Copy link
Contributor

@LanceAndersen LanceAndersen commented Sep 9, 2023

Please review this PR which improves the Zip64 extra header validation:

  • Throw a ZipException If the extra len field is 0 and :
    -- size, csize, or loc offset are set to 0xFFFFFFFF
    -- disk starting number is set to 0xFFFF

  • We have a valid size for the Zip64 extra header but we are missing the csize or loc fields if they are expected to be part of the header

Mach5 tiers 1-3 are clean


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-8314891: Additional Zip64 extra header validation (Bug - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15650

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

Using diff file

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

Webrev

Link to Webrev Comment

@LanceAndersen LanceAndersen marked this pull request as ready for review September 9, 2023 14:34
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 9, 2023

👋 Welcome back lancea! 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 openjdk bot added the rfr Pull request is ready for review label Sep 9, 2023
@openjdk
Copy link

openjdk bot commented Sep 9, 2023

@LanceAndersen The following labels will be automatically applied to this pull request:

  • core-libs
  • nio

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

@openjdk openjdk bot added nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Sep 9, 2023
@mlbridge
Copy link

mlbridge bot commented Sep 9, 2023

* and the CEN size, csize,LOC offset fields are set to 0xFFFFFFFF, the disk
* starting number is set to 0xFFFF or when we have a valid Zip64 Extra header
* size but missing the corresponding field.
* @run junit MissingZIP64EntriesTest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment accurate? I think we should check 3 cases when the header extra len == 0, len == 8 and len ==16, but still do not contain all required information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified the comment to make it a bit clearer and also added additional tests

* Value to set the Disk Start number offset CEN field to when the
* actual value is stored in the Zip64 Extra Header
*/
private static final int ZIP64_MAGICCOUNT = 0xFFFF;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final int ZIP64_MAGICCOUNT = 0xFFFF;
private static final int ZIP64_MAGICCOUNT = 0xFFFF;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the extra space. thank you for pointing it out

@openjdk openjdk bot removed the rfr Pull request is ready for review label Sep 14, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 14, 2023
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 16, 2023

@LanceAndersen This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Copy link
Contributor

@coffeys coffeys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me Lance. I note that the extra checks aren't reversed via any sort of system property but given that this fix isn't planned for JDK update releases, that seems fine to me.

early testing by frameworks which create/modify custom zip files will be important.

@openjdk
Copy link

openjdk bot commented Oct 18, 2023

@LanceAndersen 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:

8314891: Additional Zip64 extra header validation

Reviewed-by: coffeys

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 openjdk bot added the ready Pull request is ready to be integrated label Oct 18, 2023
@LanceAndersen
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 7, 2023

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

  • 0dcd730: 8318594: NMT: VM.native_memory crashes on assert if functionality isn't supported by OS
  • 45e68ae: 8319532: jshell - Non-sealed declarations sometimes break a snippet evaluation
  • 4a0ad46: 8317937: @sealedGraph: Links to inner classes fails in links
  • 134c382: 8319560: Reformat method parameters in the FFM API
  • ef8c840: 8319607: FFM: Review the language in the FFM documentation
  • bf9a93d: 8319204: G1: Change G1CMTask::_termination_time_ms to wallclock time
  • b2504a0: 8319525: RISC-V: Rename *_riscv64.ad files to *_riscv.ad under riscv/gc
  • bfafb27: 8319615: IGV incomplete gitignore
  • c760097: 8319541: G1: Inline G1RemoveSelfForwardsTask into RestoreRetainedRegionsTask
  • 85e4cde: 8319620: Parallel: Remove unused PSPromotionManager::*_is_full getters and setters
  • ... and 187 more: https://git.openjdk.org/jdk/compare/8d9a4b43f4fff30fd217dab2c224e641cb913c18...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 7, 2023
@openjdk openjdk bot closed this Nov 7, 2023
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Nov 7, 2023
@openjdk openjdk bot removed the rfr Pull request is ready for review label Nov 7, 2023
@openjdk
Copy link

openjdk bot commented Nov 7, 2023

@LanceAndersen Pushed as commit 8274713.

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

@eirbjo
Copy link
Contributor

eirbjo commented Nov 8, 2023

@LanceAndersen

I noticed that this PR did not update ZipInputStream.readLOC to perform consistency validation between expected and actual extra field size and values. Any particular reason why processing of LOC headers was not made consistent with CEN?

@LanceAndersen
Copy link
Contributor Author

@LanceAndersen

I noticed that this PR did not update ZipInputStream.readLOC to perform consistency validation between expected and actual extra field size and values. Any particular reason why processing of LOC headers was not made consistent with CEN?

Intentional, as this was a follow on to the updates which were done previously to the CEN work in August, this is follow on cleanup.

Updates to ZipInputStream would be done separately under a separate PR or could be done via your work on 8303866

@Foorcee
Copy link

Foorcee commented May 13, 2024

Hey @LanceAndersen,

It was a common practice in obfuscation, to create zips with invalid headers. This change leads to a behavioral change that affects existing work processes. Would it be possible to add an system property to restore the old behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated nio nio-dev@openjdk.org
6 participants