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

[Ferenc Rakoczi]

On Thu, 4 May 2023 20:00:18 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   adding key translation, finally block, removing 24-byte LMOTS parameters
>
> src/java.base/share/classes/sun/security/provider/HSS.java line 94:
> 
>> 92:             result &= lmsVerify(lmsPubKey, sig.siglist[sig.Nspk], messageStream.toByteArray());
>> 93:             return result;
>> 94:         } catch (Exception e) {
> 
> If all exceptions thrown are already `SignatureException`, we can let them thrown out instead of returning false. According to the `engineVerify` spec, any problem inside the signature should throw a `SignatureException`. False is returned when the public key cannot verify the exception.

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 295:
> 
>> 293:             int inLen = sigArray.length;
>> 294:             if (inLen < 4)
>> 295:                 throw new InvalidParameterException("Invalid LMS signature");
> 
> This is LM-OTS signature. Also, give some explanation. Same on line 301.

Explained.

> src/java.base/share/classes/sun/security/provider/HSS.java line 459:
> 
>> 457:         final byte[] sigArr;
>> 458: 
>> 459:         public LMSignature(byte[] sigArray, int offset, boolean checkExactLen) throws InvalidParameterException {
> 
> How about we throw a `SignatureException` here. `new HSSSignature` and `new LMOTSignature` all throw it.

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 467:
> 
>> 465:             qArr = Arrays.copyOfRange(sigArray, offset, offset + 4);
>> 466:             sigOtsType = LMSUtils.fourBytesToInt(sigArray, offset + 4);
>> 467:             LMOTSParams lmotsParams = LMOTSParams.of(sigOtsType);
> 
> Catch `IllegalArgumentException` here and rethrow a `SignatureException`. Same on line 482.

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 659:
> 
>> 657:         }
>> 658:         public byte[] lmotsPubKeyCandidate(LMSignature lmSig, byte[] message, LMSPublicKey pKey)
>> 659:                 throws NoSuchAlgorithmException, DigestException {
> 
> `DigestException` should not happen because we allocate all the buffer ourselves. `NoSuchAlgorithmException` should not happen because we should have all the algorithms. If they do happen, that's not user problem at all. I think the usual way is to wrap them into a `ProviderException`. This is also true in `lmsVerify()`.

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 662:
> 
>> 660:             LMOTSignature lmOtSig = lmSig.lmotSig;
>> 661:             if (lmOtSig.otSigType != pKey.otsType) {
>> 662:                 throw new IllegalArgumentException("OTS public key type and OTS signature type do not match");
> 
> I'd rather throw `SignatureException`. This method is actually verifying an LM-OTS signature, right? Avoid using unchecked exceptions instead of `ProviderException` unless you can be sure they are caught nicely.

Changed.

> src/java.base/share/classes/sun/security/provider/HSS.java line 736:
> 
>> 734:                 }
>> 735:             }
>> 736:             throw new InvalidKeySpecException();
> 
> Give an error message. Try look at what other factories are doing. Same for lines 751, 768, 727, 44, 49, and 57.

Added explanations.

> src/java.base/share/classes/sun/security/provider/HSS.java line 745:
> 
>> 743: 
>> 744:         @Override
>> 745:         protected <T extends KeySpec> T engineGetKeySpec(Key key, Class<T> keySpec) throws InvalidKeySpecException {
> 
> Usually when `key` is null an `InvalidKeySpecException` is thrown. Here it's an NPE.

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 809:
> 
>> 807:         HSSSignature(byte[] sigArr, int pubKeyL, String pubKeyHashAlg) throws SignatureException {
>> 808:             if (sigArr.length < 4) {
>> 809:                 throw new SignatureException("Bad HSS signature");
> 
> Maybe you can say why it is bad. This also applies to "Invalid HSS public key", "Invalid LMS signature" and "Invalid LMS public key" messages.

Done.

> src/java.base/share/classes/sun/security/provider/HSS.java line 823:
> 
>> 821:                     index += siglist[i].sigArrayLength();
>> 822:                     pubList[i] = new LMSPublicKey(sigArr, index, false);
>> 823:                     if (!pubList[i].getDigestAlgorithm().equals(pubKeyHashAlg)) {
> 
> Comparing hash algorithm is not enough. Length (`m`) should also be compared.

Compared.

> src/java.base/share/classes/sun/security/provider/HSS.java line 829:
> 
>> 827:                 }
>> 828:                 siglist[Nspk] = new LMSignature(sigArr, index, true);
>> 829:             } catch (Exception E) {
> 
> The `SignatureException` thrown on line 824 does not need to be wrapped into another `SignatureException`. Maybe just catch `InvalidKeyException` (if you throw `SignatureException` in `new LMSSignature`).

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187452891
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453204
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453062
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453398
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187452980
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453253
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453292
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187452998
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453109
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187453175
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1187452934