On Tue, 3 Aug 2021 21:05:24 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java line > 386: > >> 384: (attributes[2].getBoolean() == false)) { >> 385: keySensitive = true; >> 386: } > > nit: this block and line 377 can simply be: > `boolean keySensitive = (attributes[0].getBoolean() || > attributes[1].getBoolean() || (!attributes[2].getBoolean()));` Sure > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java > line 123: > >> 121: >> 122: // signature length expected or 0 for unknown >> 123: private int signatureLength; > > nit: use a shorter name, e.g. sigLen, so to fit in one line. Ok > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java > line 817: > >> 815: } >> 816: >> 817: private byte[] asn1ToDSA(byte[] sig) throws SignatureException { > > Have you considered keeping this as a static method but add one more int > argument, i.e. signature length? It seems that the method asn1ToECDSA() can > be made static too. Ok > test/jdk/sun/security/pkcs11/Signature/LargeDSAKey.java line 52: > >> 50: "Known text known text known text"; >> 51: >> 52: public void main(Provider p) throws Exception { > > nit: add @Override Sure ------------- PR: https://git.openjdk.java.net/jdk/pull/4961
