Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]
On Thu, 13 Jan 2022 21:57:57 GMT, Sean Mullan wrote: >> 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. > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Change permittedAlgs variable name. > Move put methods out of checkConstraints(). Each `CodeSigner[]` reference uniquely represents the signers of an entry. Multiple entries can map to the same `CodeSigner[]` reference. We only need reference equality for the keys as each JAR entry is already mapped to an array of `CodeSigner`. It's basically an `IdentityHashMap` but we don't need to specifically use that. - PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]
On Thu, 13 Jan 2022 21:57:57 GMT, Sean Mullan wrote: >> 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. > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Change permittedAlgs variable name. > Move put methods out of checkConstraints(). No more comment. Approved. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]
On Thu, 13 Jan 2022 21:54:17 GMT, Weijun Wang wrote: >> 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. >> 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. - PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]
On Thu, 13 Jan 2022 19:54:44 GMT, Sean Mullan 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
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]
> 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. Sean Mullan has updated the pull request incrementally with one additional commit since the last revision: Change permittedAlgs variable name. Move put methods out of checkConstraints(). - Changes: - all: https://git.openjdk.java.net/jdk/pull/7056/files - new: https://git.openjdk.java.net/jdk/pull/7056/files/056bcff2..b551c2b9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7056=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7056=00-01 Stats: 13 lines in 1 file changed: 3 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7056/head:pull/7056 PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
On Thu, 13 Jan 2022 16:55:08 GMT, Weijun Wang wrote:
>> 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.
>
> 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.
> 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?
-
PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
On Wed, 12 Jan 2022 21:57:22 GMT, Sean Mullan wrote:
> 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.
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`.
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.
src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line
272:
> 270: }
> 271:
> 272: // Gets the permitted algs for the signers of this entry.
This can probably be another `computeIfAbsent`.
-
PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
On Thu, 13 Jan 2022 13:56:14 GMT, Sean Mullan wrote: >> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java >> line 212: >> >>> 210: >>> 211: CodeSigner[] entrySigners = sigFileSigners.get(name); >>> 212: Map 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. >> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java >> line 239: >> >>> 237: >>> 238: // A non-disabled algorithm was used. >>> 239: 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! - PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
On Wed, 12 Jan 2022 21:57:22 GMT, Sean Mullan wrote: > 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. Marked as reviewed by coffeys (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
On Thu, 13 Jan 2022 12:33:53 GMT, Sean Coffey wrote: >> 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. > > src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line > 212: > >> 210: >> 211: CodeSigner[] entrySigners = sigFileSigners.get(name); >> 212: Map 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`? > src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line > 239: > >> 237: >> 238: // A non-disabled algorithm was used. >> 239: 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. - PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
On Wed, 12 Jan 2022 21:57:22 GMT, Sean Mullan wrote: > 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. src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line 212: > 210: > 211: CodeSigner[] entrySigners = sigFileSigners.get(name); > 212: Map permittedAlgs = maybe permittedAlgsChecker as variable name ? the Map contains both permitted and non-permitted algs. src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line 239: > 237: > 238: // A non-disabled algorithm was used. > 239: disabledAlgs = false; this usage doesn't seem right. I think it's always set to false no matter what algs are detected. - PR: https://git.openjdk.java.net/jdk/pull/7056
RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
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. - Commit messages: - Initial revision. Changes: https://git.openjdk.java.net/jdk/pull/7056/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7056=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278851 Stats: 263 lines in 3 files changed: 213 ins; 20 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/7056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7056/head:pull/7056 PR: https://git.openjdk.java.net/jdk/pull/7056
