On Tue, 2 May 2023 21:43:19 GMT, Ferenc Rakoczi <d...@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
