8278851: Correct signer logic for jars signed with multiple digestalgs #7056

seanjmullan · 2022-01-12T21:57:22Z

If a JAR is signed with multiple digest algorithms and one of the digest algorithms is disabled, ManifestEntryVerifier.verify() was incorrectly returning null indicating that the jar entry has no signers.

This fixes the issue such that an entry is considered signed if at least one of the digest algorithms is not disabled and the digest match passes. This makes the fix consistent with how multiple digest algorithms are handled in the Signature File. This also fixes an issue in the ManifestEntryVerifier.getParams() method in which it was incorrectly checking the algorithm constraints against all signers of a JAR when it should check them only against the signers of the entry that is being verified.

An additional cache has also been added to avoid checking if the digest algorithm is disabled more than once for entries signed by the same set of signers.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8278851: Correct signer logic for jars signed with multiple digestalgs

Reviewers

Sean Coffey (@coffeys - Reviewer) ⚠️ Review applies to 056bcff
Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7056/head:pull/7056
$ git checkout pull/7056

Update a local copy of the PR:
$ git checkout pull/7056
$ git pull https://git.openjdk.java.net/jdk pull/7056/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7056

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7056.diff

bridgekeeper · 2022-01-12T21:58:40Z

👋 Welcome back mullan! 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 · 2022-01-12T22:01:09Z

@seanjmullan 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 · 2022-01-12T22:04:54Z

Webrevs

01: Full - Incremental (b551c2b9)
00: Full (056bcff2)

coffeys · 2022-01-13T12:33:53Z

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java

-        JarConstraintsParameters params =
-            getParams(verifiedSigners, sigFileSigners);
+        CodeSigner[] entrySigners = sigFileSigners.get(name);
+        Map<String, Boolean> permittedAlgs =


maybe permittedAlgsChecker as variable name ? the Map contains both permitted and non-permitted algs.

Checker sounds like it going to do something. But I agree the name could be better. I was mostly being consistent with the permittedAlgs variable in SignatureFileVerifier. Maybe algsPermittedStatus?

yes, algsPermittedStatus sounds better. Thanks.

coffeys · 2022-01-13T12:35:36Z

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java

            }
+
+            // A non-disabled algorithm was used.
+            disabledAlgs = false;


this usage doesn't seem right. I think it's always set to false no matter what algs are detected.

If all algs are disabled, it will never get here, because it will either continue on line 231 or 234.

Ah yes - I was reading the scope of for loop incorrectly. Thanks for clarifying!

coffeys · 2022-01-13T15:26:59Z

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java

            }
+
+            // A non-disabled algorithm was used.
+            disabledAlgs = false;


Ah yes - I was reading the scope of for loop incorrectly. Thanks for clarifying!

coffeys · 2022-01-13T15:27:39Z

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java

-        JarConstraintsParameters params =
-            getParams(verifiedSigners, sigFileSigners);
+        CodeSigner[] entrySigners = sigFileSigners.get(name);
+        Map<String, Boolean> permittedAlgs =


yes, algsPermittedStatus sounds better. Thanks.

openjdk · 2022-01-13T15:29:34Z

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

8278851: Correct signer logic for jars signed with multiple digestalgs

Reviewed-by: coffeys, 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 19 new commits pushed to the master branch:

35172cd: 8278951: containers/cgroup/PlainRead.java fails on Ubuntu 21.10
237f861: 8273143: Transition to _thread_in_vm when handling a polling page exception
9209e6d: 8279877: Document IDEA IDE setup in docs/ide.md
0a839b4: 8279801: EC KeyFactory and KeyPairGenerator do not have aliases for OID format
6fcaa32: 8262442: (windows) Use all proxy configuration sources when java.net.useSystemProxies=true
c17a012: 8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java
b61a4af: 8259774: Deprecate -XX:FlightRecorderOptions:samplethreads
6933934: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java
4851948: 8279903: Redundant modulo operation in ECDHKeyAgreement
67e3d51: Merge
... and 9 more: https://git.openjdk.java.net/jdk/compare/ddddec7d74745905230282124524a0dbdd1bd1c1...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.

wangweij · 2022-01-13T16:46:52Z

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java

+                        params = new JarConstraintsParameters(entrySigners);
                    }
-                    return null;
+                    if (!checkConstraints(digestAlg, permittedAlgs, params)) {


Can we move the permittedAlgs::put call from inside the checkConstraints method to here? You can even call computeIfAbsent to make the intention clearer.

Yes, I can do that. However, I'm a bit wary of using lambdas in this code which may get exercised early in app startup. WDYT?

Maybe, I don't know how problematic if lambda is used this early.

Anyway, I still prefer moving the update of permittedAlgs here. Updating it inside the method seems too faraway.

Changed as suggested in latest revision.

wangweij · 2022-01-13T16:50:58Z

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java

-            } else {
-                return new JarConstraintsParameters(
-                    verifiedSigners.get(manifestFileName));
+    // Gets the permitted algs for the signers of this entry.


This can probably be another computeIfAbsent.

wangweij · 2022-01-13T16:55:08Z

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java


-        JarConstraintsParameters params =
-            getParams(verifiedSigners, sigFileSigners);
+        CodeSigner[] entrySigners = sigFileSigners.get(name);


What if we return here if entrySigners == null? It seems the hash comparison will be skipped, but at the end there is no difference in verifiedSigners.

The algorithm constraints check will be skipped (because permittedAlgs will be null) but the hash check will not be skipped.

I don't think null would be returned in a normal case. The only case I can think of is if there was an entry in the Manifest, but no corresponding entry in the SF file. I suppose we could still do a constraints check as if there were no signers. However, it doesn't seem that useful since technically the entry is not protected by the signature and the hash check is still done, unless I am missing something.

Or maybe the key/signature algorithm is weak and ignored. I was only thinking it's not worth calculating the hash anymore. Of course there will be a behavior change. If there's a hash mismatch, it used to be a security exception, but if ignored, it's just a plain unsigned entry.

It will never get here if all of the signers are using disabled algorithms (or for some other reason cannot be parsed/verified) as JarVerifier.nothingToVerify() will return true. But I think it's possible if one of the signers is ok. But I'd prefer not to make that change because of the change in behavior. And I think in most cases, JARs will have a single signer.

Move put methods out of checkConstraints().

wangweij

No more comment. Approved.

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java

seanjmullan · 2022-01-14T15:21:26Z

/integrate

openjdk · 2022-01-14T15:22:47Z

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

35734ad: 8279545: Buffer overrun in reverse_words of sharedRuntime_x86_64.cpp:3517
e8f494c: 8279825: JFR: JFCModel shouldn't need FilePermission to read predefined .jfc files
9f30ec1: 8278398: jwebserver: Add test to confirm maximum request time
dd76a28: 8280000: Remove unused CardTable::find_covering_region_containing
84976b4: 8278549: UNIX sun/font coding misses SUSE distro detection on recent distro SUSE 15
965c64b: 8279699: Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
35172cd: 8278951: containers/cgroup/PlainRead.java fails on Ubuntu 21.10
237f861: 8273143: Transition to _thread_in_vm when handling a polling page exception
9209e6d: 8279877: Document IDEA IDE setup in docs/ide.md
0a839b4: 8279801: EC KeyFactory and KeyPairGenerator do not have aliases for OID format
... and 15 more: https://git.openjdk.java.net/jdk/compare/ddddec7d74745905230282124524a0dbdd1bd1c1...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-14T15:23:08Z

@seanjmullan Pushed as commit 61b8944.

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

Initial revision.

056bcff

openjdk bot added the rfr Pull request is ready for review label Jan 12, 2022

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

coffeys reviewed Jan 13, 2022

View reviewed changes

coffeys approved these changes Jan 13, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 13, 2022

wangweij reviewed Jan 13, 2022

View reviewed changes

Change permittedAlgs variable name.

b551c2b

Move put methods out of checkConstraints().

wangweij approved these changes Jan 13, 2022

View reviewed changes

isopov reviewed Jan 14, 2022

View reviewed changes

src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java Show resolved Hide resolved

openjdk bot added the integrated Pull request has been integrated label Jan 14, 2022

openjdk bot closed this Jan 14, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 14, 2022

8278851: Correct signer logic for jars signed with multiple digestalgs #7056

8278851: Correct signer logic for jars signed with multiple digestalgs #7056

Uh oh!

Conversation

seanjmullan commented Jan 12, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 12, 2022

Uh oh!

openjdk bot commented Jan 12, 2022

Uh oh!

mlbridge bot commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanjmullan commented Jan 14, 2022

Uh oh!

openjdk bot commented Jan 14, 2022

Uh oh!

openjdk bot commented Jan 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

seanjmullan commented Jan 12, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 12, 2022 •

edited

Loading

openjdk bot commented Jan 13, 2022 •

edited

Loading