RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]

[Weijun Wang]

On Thu, 13 Jan 2022 19:54:44 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line 211:
>> 
>>> 209:         }
>>> 210: 
>>> 211:         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.

>> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line 230:
>> 
>>> 228:                         params = new JarConstraintsParameters(entrySigners);
>>> 229:                     }
>>> 230:                     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.

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

PR: https://git.openjdk.java.net/jdk/pull/7056