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

Weijun Wang weijun at openjdk.org
Thu May 4 21:52:28 UTC 2023


On Tue, 2 May 2023 21:43:19 GMT, Ferenc Rakoczi <duke at openjdk.org> wrote:

>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC 8554. LMS is an approved software signing algorithm for CNSA 2.0, with SHA-256/192 parameters recommended.
>
> 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.

src/java.base/share/classes/sun/security/provider/HSS.java line 181:

> 179:             try {
> 180:                 lmParams = new LMParams(type);
> 181:                 lmotsParams = LMOTSParams.of(otsType);

Try to code `LMParams` and `LMOTSParams` the same style by choosing from using a constructor or static `of` method. Should `LMParams` be renamed to `LMSParams`?

src/java.base/share/classes/sun/security/provider/HSS.java line 294:

> 292:         LMOTSignature(byte[] sigArray, LMOTSParams lmotsParams) throws InvalidParameterException {
> 293:             int inLen = sigArray.length;
> 294:             if (inLen < 4)

Add braces around the one-line block. Same as in lines 300, 173, 461, 472, 487, 569, 571, and 783.

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.

src/java.base/share/classes/sun/security/provider/HSS.java line 357:

> 355:                     break;
> 356: 
> 357: /*

Remove commented-out lines if they cannot be supported on time.

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.

src/java.base/share/classes/sun/security/provider/HSS.java line 528:

> 526:         // update()-digest() sequence) which is parametrized so that the digest output is copied back into this buffer.
> 527:         // This way, we avoid memory allocations and some computations that would have to be done otherwise.
> 528:         final byte[] hashBuf;

I'm a little worried about the mutability of `hashBuf` and whether it's suitable to be put inside `LMOTSParams`.  By using `of` to return an `LMOTSParams` object we have the chance to return cached objects in the future. There should always be one `hashBuf` for each LM-OTS verification, and this is not clear from the current code.

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()`.

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?

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.

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.

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.

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.

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`).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185459409
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185523625
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185518085
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185540342
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185521512
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185512129
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185533184
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185503139
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185544334
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185546621
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185508073
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185535770
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185536879
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1185497769


More information about the security-dev mailing list