8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories #11976

eirbjo · 2023-01-12T18:44:26Z

Some call sites of SignatureFileVerifier.isBlockOrSF fails to check that files reside in META-INF directly, and not in a subdirectory of META-INF.

The mentioned call sites needs updates to check and ignore such files.

A new test IgnoreUnrelatedSignatureFiles is added which verifies that [*.SF, *.RSA] files in META-INF/ subdirectories are indeed ignored.

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-8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11976

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

Using diff file

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

…s signature related

bridgekeeper · 2023-01-12T18:47:05Z

👋 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 · 2023-01-12T18:49:37Z

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

core-libs
security

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.

mlbridge · 2023-01-13T20:56:18Z

Webrevs

wangweij

Just some comments before the end of week.

wangweij · 2023-01-13T22:31:28Z

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

@@ -1745,8 +1745,27 @@ private boolean isSignatureRelated(int off, int len) {
            assert(signatureRelated == SignatureFileVerifier
                .isBlockOrSF(new String(name, off, len, UTF_8.INSTANCE)
                    .toUpperCase(Locale.ENGLISH)));
+


How about updating SignatureFileVerifier.isBlockOrSF so that it only returns true for files inside META-INF/. This way it's consistent to this method.

I started there, but ran into some problems:

SignatureFileVerifier.isSigningRelated calls isBlockOrSF, but it removes the "META-INF/" prefix from the path first. So we can't assume that input to isBlockOrSF is the full path.

I could update SignatureFileVerifier.isSigningRelated to send the full path, but we still have another problem:

JarSigner.sign0 puts META-INF/ files in a vector, such that it can output them first. We want to update this method such that it outputs only files which live directly in META-INF/ first. So we still need to check for "directness" outside isBlockOrSF.

isBlockOrSF has 8 call sites, most of them in security sensitive and tricky code. In the end, I felt safer leaving isBlockOrSF alone and just fix the bug-relevant call sites instead. Also, being a new contributor to security-dev, I wanted to keep the PR relatively simple and easy to review.

WDYT?

In almost every call to isBlockOrSF, there is already an isInMetaInf (or equivalent) call right before it so it's always safe. The only exception is in jarsigner's verifyJar method:

hasSignature = hasSignature || SignatureFileVerifier.isBlockOrSF(name);

and it makes a subtle difference. The unrelated-signature-files-modified.jar your test created has SF/block files inside a sub-directory. Try verifying it with jarsigner. Here hasSignature is true but the SF/block is not considered signature-related, an error message will show Signature is either not parsable or not verifiable. If isBlockOrSF is modified to return false, it will be simply jar is unsigned.

Good catch!

I've added a guarding call to SignatureFileVerifier.isSigningRelated here, similar to the earlier call site of isBlockOrSf in this methid. I've also added a new check to the test to verify that there is no WARNING when calling jarsigner with the modified jar.

wangweij · 2023-01-13T22:36:01Z

src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java

+    /**
+     * Returns true iff the entry resides directly in the META-INF/ directory
+     */
+    private boolean isInMetaInf(ZipEntry ze) {


Maybe move this method and the one in JarVerifier to a common place like sun.security.util.SignatureFileVerifier?

This duplicated check annoyed me also, but the existing checks have different behavior:

JarVerifier.beginEntry normalizes the path to uppercase, them checks that it starts with "META-INF/" or "/META-INF/"

JarSigner.sign0 does not normalize to uppercase , then checks that the path starts with "META-INF/"

Introducing a common method would need change behaviour of one of these methods. This change of behaviour would not be relevant to the bug being fixed in this PR.

Since I'm cautious of changing behaviour, I decided to keep the two methods.

While JarSigner has not normalize to uppercase, the check is the same. As for /META-INF/, it must be very broken now since JarFile::maybeInstantiateVerifier is using JUZFA.getManifestName(this,true) to read the manifest and since ZipFile cannot see the signature-related files starting with /META-INF/ this method will always return null.

I agree this deserves a cleanup, and since we're already deep into it it can make sense to fix this in this PR.

I have introduced SignatureFileVerifier.isInMetaInf as a common method to replace duplicated logic in JarVerifier and JarSigner. I also noticed that SignatureFileVerifier.isSigningRelated performs the same check, so I updated that method to also call the isInMetaInf.

This introduces two subtle behavioural changes:

JarSigner is relaxed to ignore case when checking isInMetaInf, meaning "meta-inf/" files will now be moved to the beginning of the signed JAR

JarVerifier is tightened to reject /META-INF/ as not in META-INF/. This is believed to have little practical impact, since ZipFile.Source.isMetaName already rejects such paths.

When introducing the call to isInMetaInf in isSigningRelated, I accidentally broke the matching of MANIFEST.MF and SIG-* files.

When fixing this regression, I now match against the full path instead of the existing prefix stripping substring. (A nice side effect of this is that isBlockOrSF is now always called with the full path)

Since the regression was not caught by any existing test, I'm also adding a sanity check that a basic signed JAR has the expected sections in MANIFEST.MF. (The regression introduced a section for META-INF/MANIFEST.MF which seemed to not be caught by tests)

On a similar note, I added test covering for the matching of custom SIG-* files in SignatureFileVerifier.isSigningRelated.

The test now checks both valid and invalid SIG- file extensions and directory locations inside/outside META-INF

Added test cases for:

META-INF/unrelated.txt (Unrelated file in META-INF)

META-INF/SIG-CUSTOM (No extension is ok)

META-INF/SIG-CUSTOM2. (Extension too short)

META-INF/SIG-CUSTOM2.ABCD (Extension too long)

Reading the Jar File Specification, I cannot see that it explicitly allows no extension for SIG- files, but that's what the current code does. It feels safest to leave it that way.

wangweij · 2023-01-13T22:36:47Z

test/jdk/java/util/jar/JarFile/VerifyUnrelatedSignatureFiles.java

+
+        File j = createJarFile();
+        File s = signJarFile(j, "signed");
+        File m = moveSignatureRelated(s);


Try sign it again to a different file. Let's see if the moved files are also signed.

Nice, I added a check which verifies that a JAR containing non signature related files is signed as expected

I also added a check verifying that JarSigner does not move unrelated signature files to the beginning of the signed JAR

… "this path"

…ar (non signature related) files during signing

…ginning of signed JARs

…er is actually being exercised by our tests, add unrelated signature files also at the beginning of the test JAR

…n of JARs created during the test

…is for try,while,if.Removed final accessors for some local variables.

…lace duplicated logic in JarVerifier, JarSigner and SignatureFileVerifier.isSigningRelated. This introduces two subtle behavioural changes: - JarSigner is relaxed to ignore case when checking isInMetaInf, meaning "meta-inf/" files will now be moved to the beginning of the signed JAR - JarVerifier is tightened to reject /META-INF/ as not in META-INF/. This is believed to have little impact, since ZipFile.Source.isMetaName already rejects such paths.

eirbjo · 2023-01-18T09:53:28Z

I added a few style fixes to the test which primarily adds whitespace between try,for,while,if and their following left parenthesis.

…hat unrelated files should be ignored.

…Inf accidentally removed "META-INF/" prefix stripping. This broke some checks in the remainder of this method. I chose to restore current behaviour by comparing MANIFEST.MF and "SIG-" files using the full path. This saves the allocation caused by substring and also makes sure isBlockOrSF now always called with the full path.

…d by SignatureFileVerifier.isSigningRelated revealed was not caught by any test. Adding a sanity check to IgnoreUnrelatedSignatureFiles which verifies that a basic signed JAR has the expected manifest sections.

…Verifier.isSigningRelated

…ding META-INF/unrelated.txt and META-INF/SIG- files with invalid as well as no file extension

wangweij · 2023-01-24T18:54:59Z

The change looks fine. I'll run some test.

Precisely ZipFile::isSignatureRelated should also contain those SIG- files. The feature is not used so I cannot say if it's wrong.

eirbjo · 2023-01-24T19:04:45Z

Precisely ZipFile::isSignatureRelated should also contain those SIG- files.

Should they though? These files are ultimately read by JarFile.initializeVerifier, which I guess only cares about signature/block files it actually knows how to verify, currently EC, RSA, DSA?

The feature is not used so I cannot say if it's wrong.

The JAR File Specification is a bit short on the purpose of these files. I assume they are expected to be verified by code external to the JDK?

wangweij · 2023-01-27T22:16:18Z

Yes, you're right. ZipFile:: isSignatureRelated collects them for verification but SignatureFileVerifier:: isSigningRelated is about do not re-sign them while signing.

Maybe we can rename ZipFile::isSignatureRelated to ZipFile::isBlockOrSF as well?

I'll approved this PR. Thanks.

openjdk · 2023-01-27T22:18:22Z

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

8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories

Reviewed-by: weijun

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 251 new commits pushed to the master branch:

ae0e76d: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections
b8e5abc: 8301097: Update GHA XCode to 12.5.1
9c4bc2c: 8301132: Test update for deprecated sprintf in Xcode 14
7f05d57: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module
22c976a: 8177418: NPE is not apparent for methods in java.util.TimeZone API docs
7aaf76c: 8300924: Method::invoke throws wrong exception type when passing wrong number of arguments to method with 4 or more parameters
49ff520: 8300241: Replace NULL with nullptr in share/classfile/
f52d35c: 8300240: Replace NULL with nullptr in share/ci/
5c1ec82: 8301077: Replace NULL with nullptr in share/services/
dff4131: 8285850: [AIX] unreachable code in basic_tools.m4 -> BASIC_CHECK_TAR
... and 241 more: https://git.openjdk.org/jdk/compare/4be94e435002d9d6cb594a393e9e35d650a9a0c9...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@wangweij) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

eirbjo · 2023-01-27T22:42:16Z

Maybe we can rename ZipFile::isSignatureRelated to ZipFile::isBlockOrSF as well?

The term "signature related" seems to be used quite extensively around ZipFile and also in JavaUtilZipFileAccess. Semantics are very similar, but not exactly equal. Because of this, I'm a bit reluctant to rename it.

I'll approved this PR. Thanks.

Thank for your help and guidance in reviewing this PR!

eirbjo · 2023-01-27T22:42:27Z

/integrate

openjdk · 2023-01-27T22:43:59Z

@eirbjo
Your change (at version 408d222) is now ready to be sponsored by a Committer.

wangweij · 2023-01-27T22:46:39Z

/sponsor

openjdk · 2023-01-27T22:48:01Z

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

5c59de5: Merge
e5860ef: 8301206: Fix issue with LocaleData after JDK-8300719
b22e521: 8300953: ClassDesc::ofInternalName missing @SInCE tag
a67b1e7: 8300719: JDK 20 RDP2 L10n resource files update
ae0e76d: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections
b8e5abc: 8301097: Update GHA XCode to 12.5.1
9c4bc2c: 8301132: Test update for deprecated sprintf in Xcode 14
7f05d57: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module
22c976a: 8177418: NPE is not apparent for methods in java.util.TimeZone API docs
7aaf76c: 8300924: Method::invoke throws wrong exception type when passing wrong number of arguments to method with 4 or more parameters
... and 245 more: https://git.openjdk.org/jdk/compare/4be94e435002d9d6cb594a393e9e35d650a9a0c9...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-27T22:48:15Z

@wangweij @eirbjo Pushed as commit 5dfc4ec.

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

eirbjo added 2 commits January 12, 2023 19:31

Files residing in subdirectories of META-INF/ should not be treated a…

d219304

…s signature related

Merge branch 'master' into signature-related-subdirs

342b3c2

eirbjo changed the title ~~Signature related subdirs~~ Files residing in subdirectories of META-INF/ should not be treated as signature related Jan 12, 2023

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jan 12, 2023

eirbjo changed the title ~~Files residing in subdirectories of META-INF/ should not be treated as signature related~~ Files residing in subdirectories of META-INF should not be treated as signature related Jan 13, 2023

eirbjo changed the title ~~Files residing in subdirectories of META-INF should not be treated as signature related~~ 8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories Jan 13, 2023

eirbjo marked this pull request as ready for review January 13, 2023 20:51

openjdk bot added the rfr Pull request is ready for review label Jan 13, 2023

wangweij reviewed Jan 13, 2023

View reviewed changes

eirbjo added 12 commits January 14, 2023 12:17

Use @modules tags instead of explicit --add-exports args

95380a6

Simplify comment by removing a negative and changing "these paths" to…

b1b9d27

… "this path"

Merge branch 'master' into signature-related-subdirs

9813bbb

Check that non-signature related .SF, .RSA files are treated as regul…

828e083

…ar (non signature related) files during signing

Check that JarSigner does not move unrelated [SF,RSA] files to the be…

209d12e

…ginning of signed JARs

To ensure that the "unrelated signature files" code path of JarVerifi…

f264547

…er is actually being exercised by our tests, add unrelated signature files also at the beginning of the test JAR

Tag the test with the bug number

6d0f690

Fix copyright year of newly added test

808d1f4

Replacing File.createTempFile with Path.of lets jtreg handle retentio…

d64d64b

…n of JARs created during the test

Style: Added whitespace separators between keyword and left parenthes…

beb7455

…is for try,while,if.Removed final accessors for some local variables.

jarsigner -verify should also ignore unrelated signature files

e9128f8

eirbjo added 4 commits January 18, 2023 10:57

Renaming the test to IgnoreUnrelatedSignatureFiles to make it clear t…

0cd4cc4

…hat unrelated files should be ignored.

Update copyright year of SignatureFileVerifier.java

e028356

Update copyright year of s.s.t.jarsigner.Main.java

c3a1997

eirbjo added 6 commits January 18, 2023 13:51

The accidental regression where MANIFEST.MF was incorrecly not matche…

3487362

…d by SignatureFileVerifier.isSigningRelated revealed was not caught by any test. Adding a sanity check to IgnoreUnrelatedSignatureFiles which verifies that a basic signed JAR has the expected manifest sections.

Add test coverage for matching of custom SIG-* files in SignatureFile…

b27d1a1

…Verifier.isSigningRelated

Link to a more recent version of the JAR File Specification

de3c678

Style: Add whitespace between if and left parenthesis

7aeef1c

Improve test coverage of SignatureFileVerifier.isSigningRelated by ad…

cf93bfc

…ding META-INF/unrelated.txt and META-INF/SIG- files with invalid as well as no file extension

Add whitespace after keywords if and for

408d222

wangweij approved these changes Jan 27, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 27, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 27, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 27, 2023

openjdk bot closed this Jan 27, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories #11976

8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories #11976

eirbjo commented Jan 12, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 12, 2023

openjdk bot commented Jan 12, 2023

mlbridge bot commented Jan 13, 2023 •

edited

Loading

wangweij left a comment

wangweij Jan 13, 2023

eirbjo Jan 14, 2023

wangweij Jan 17, 2023

eirbjo Jan 18, 2023

wangweij Jan 13, 2023

eirbjo Jan 14, 2023

wangweij Jan 17, 2023 •

edited

Loading

eirbjo Jan 18, 2023

eirbjo Jan 18, 2023 •

edited

Loading

eirbjo Jan 18, 2023

eirbjo Jan 18, 2023

wangweij Jan 13, 2023

eirbjo Jan 14, 2023

eirbjo Jan 15, 2023

eirbjo commented Jan 18, 2023

wangweij commented Jan 24, 2023

eirbjo commented Jan 24, 2023

wangweij commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

eirbjo commented Jan 27, 2023

eirbjo commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

wangweij commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories #11976

8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories #11976

Conversation

eirbjo commented Jan 12, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 12, 2023

openjdk bot commented Jan 12, 2023

mlbridge bot commented Jan 13, 2023 • edited Loading

Webrevs

wangweij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirbjo Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirbjo commented Jan 18, 2023

wangweij commented Jan 24, 2023

eirbjo commented Jan 24, 2023

wangweij commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

eirbjo commented Jan 27, 2023

eirbjo commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

wangweij commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

eirbjo commented Jan 12, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 13, 2023 •

edited

Loading

wangweij Jan 17, 2023 •

edited

Loading

eirbjo Jan 18, 2023 •

edited

Loading