On Thu, 4 May 2023 20:00:18 GMT, Weijun Wang <wei...@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
