RFR: 8298127: HSS/LMS Signature Verification [v8]

[Ferenc Rakoczi]

On Tue, 9 May 2023 14:37:36 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   agreeing with the newest review comments
>
> src/java.base/share/classes/sun/security/provider/HSS.java line 47:
> 
>> 45: 
>> 46:     @Deprecated
>> 47:     protected void engineSetParameter(String param, Object value) {
> 
> Better to add `@Override` as much as you can.

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 97:
> 
>> 95: 
>> 96:         result &= LMSUtils.lmsVerify(lmsPubKey, sig.siglist[sig.Nspk], messageStream.toByteArray());
>> 97:         messageStream.reset();
> 
> We still need the `messageStream.reset()` call in a `finally` block even if there is no `catch` block. It must be called even if an exception is thrown.

Reintroduced try/finally.

> src/java.base/share/classes/sun/security/provider/HSS.java line 114:
> 
>> 112:         }
>> 113: 
>> 114:         public LMSPublicKey(byte[] keyArray, int offset, boolean checkExactLength) throws InvalidKeyException {
> 
> Two methods are still public.

One removed, the other changed.

> src/java.base/share/classes/sun/security/provider/HSS.java line 255:
> 
>> 253:         final int otSigType;
>> 254:         final LMOTSParams lmotsParams;
>> 255:         final int n;
> 
> `n` and `p` can be private.

Changed.

> src/java.base/share/classes/sun/security/provider/HSS.java line 376:
> 
>> 374:                 qArr = Arrays.copyOfRange(sigArray, offset, offset + 4);
>> 375:                 sigOtsType = LMSUtils.fourBytesToInt(sigArray, offset + 4);
>> 376:                 lmotsParams = LMOTSParams.of(sigOtsType);
> 
> Only this single line needs to be put in the `try` block.

OK.

> src/java.base/share/classes/sun/security/provider/HSS.java line 474:
> 
>> 472:                 sha256Fix = new SHA2.SHA256();
>> 473:             } else {
>> 474:                 sha256Fix = null;
> 
> Or, shall we throw a `ProviderException` or even an `AssertionError` here?

Doesn't really matter as now it can never be null. I left in the if() in preparation of adding the SHAKE version, but I removed it now.

> src/java.base/share/classes/sun/security/provider/HSS.java line 540:
> 
>> 538:                 throw new ProviderException("Digest implementation not found", e);
>> 539:             }
>> 540:             byte[] result = new byte[md.getDigestLength()];
> 
> No need to allocate an array here.

True. Removed allocation.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190064400
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190060922
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190061753
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190061961
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190062147
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190062330
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190062543