RFR JDK-8029661: JDK-Support TLS v1.2 algorithm in SunPKCS11 provider

Tue Jul 31 23:26:04 UTC 2018

Hi Martin,

Here are the review comments for the remaining files:

<p11_convert.c>

- Always check non-null result for JNIEnv FindClass(...) calls, e.g. 
line 530 and 802

- Sometimes the check is there but is after the return value has been 
used. These checks should be moved up, i.e. right after the 
FindClass(..) calls, e.g. line 555, 834.

- line 1262, should be CK_TLS12_MASTER_KEY_DERIVE_PARAMS instead of 
CK_TLS12_MASTER_KEY_DERIVE_PARAMS_PTR

Thanks,
Valerie

On 7/26/2018 9:12 AM, Martin Balao wrote:
> Hi Valerie,
>
> Thanks for your feedback!
>
> CKM_TLS12_MAC looks like it's not in use. Authentication codes are 
> calculated through CKM_TLS12_KEY_AND_MAC_DERIVE mechanism. Do you know 
> of a library supporting CKM_TLS12_MAC but not 
> CKM_TLS12_KEY_AND_MAC_DERIVE? I've been testing this with NSS software 
> token which does not support CKM_TLS12_MAC.
>
> module-info.java changes were from when we used SSL/TLS constants, but 
> then I replaced these constants with hardcoded numbers -after 
> discussing with Xuelei-. I forgot to revert changes on this file. Thanks.
>
> Config.java changes were from a wrong version I uploaded. This has 
> been already fixed.
>
> SunPKCS11.java: I agree with you that registration must be done 
> separately. SunTls12MasterSecret and SunTls12KeyMaterial are now 
> registered separately. I've added CKM_TLS12_MASTER_KEY_DERIVE_DH 
> mechanism to SunTls12MasterSecret algorithm. It's still assumed that 
> if CKM_TLS12_MASTER_KEY_DERIVE is supported, then 
> CKM_TLS12_MASTER_KEY_DERIVE_DH it's and vice-versa -same as with 
> previous key derivation mechanisms-. Not a big deal though I think: 
> it's unlikely that a library implements only one of these. Using 
> protocol version to decide which mechanism to use should be valid in 
> this case because if an instance of P11TlsMasterSecretGenerator is 
> used and algorithm is SunTls12MasterSecret, then protocol must be TLS 
> 1.2 and we can assume that there is support of these mechanisms in the 
> underlying library.
> L527: I've realized that a better place for this map is 
> Functions.java. I've moved it there.
>
> P11TlsPrfGenerator.java: This code is for TLS 1.2 only. That's because 
> CKM_TLS_MAC mechanism is registered for SunTls12Prf algorithm. 
> Previous TLS versions use CKM_TLS_PRF and CKM_NSS_TLS_PRF_GENERAL.
>
> P11TlsRsaPremasterSecretGenerator.java: Yes, we can remove it and call 
> all keys "TlsRsaPremasterSecret", disregarding the mechanism used to 
> generate it (CKM_SSL3_PRE_MASTER_KEY_GEN or 
> CKM_TLS_PRE_MASTER_KEY_GEN). At the end of the day, this is just an 
> array of 48 bytes. I've removed the field too.
>
> Functions.java: New entries (CKM_TLS12_MASTER_KEY_DERIVE, 
> CKM_TLS12_MASTER_KEY_DERIVE_DH and CKM_TLS_MAC) moved after line 721. 
> CKM_TLS12_KEY_AND_MAC_DERIVE has been added for completeness.
>
> CK_ATTRIBUTE.java: SENSITIVE_TRUE removed.
>
> Removed a dependency in TestTLS12.java to HexDumper (internal).
>
> Webrev 06 with all these changes here:
>
>  * 
> http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.06/ 
> <http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.06/>
>  * 
> http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.06.zip 
> <http://cr.openjdk.java.net/%7Embalao/webrevs/8029661/8029661.webrev.06.zip>
>
> Regression testing on jdk/sun/security/pkcs11 passes with samae pass rate.
>
> Kind regards,
> Martin.-
>
> On Tue, Jul 24, 2018 at 11:00 PM, Valerie Peng 
> <valerie.peng at oracle.com <mailto:valerie.peng at oracle.com>> wrote:
>
>     Hi Martin,
>
>     Sorry for the delay, I am able to resume on reviewing this now.
>
>     Here are some initial comments.
>
>     What about CKM_TLS12_MAC? I see that it's defined under TLS 1.2
>     mechanisms, but not much other details for it.
>
>     <module-info.java>
>     - Is this change still necessary? I didn't notice any new import
>     statement with sun.security.ssl package in the rest of changes.
>
>     <sun/security/pkcs11/Config.java>
>     - Why removing the SENSITIVE_FALSE attribute on line 829 and 854?
>
>     <sun/security/pkcs11/P11TlsKeyMaterialGenerator.java>
>     - As my comments for the SunPKCS11.java (see below), I think we
>     should not assume the support for CKM_TLS12_KEY_AND_MAC_DERIVE.
>     The existing impl and how it's registered in SunPKCS11 class is
>     already somewhat problematic when CKM_TLS_KEY_AND_MAC_DERIVE is
>     not supported. We should avoid assuming
>     CKM_TLS12_KEY_AND_MAC_DERIVE is supported which may create even
>     more problems. Overriding this.mechanism based on the TLS version
>     specified in the parameters from the engineInit(...) call will
>     lead to failure when the mechanism is not supported by underlying
>     PKCS11 library
>
>     <sun/security/pkcs11/P11TlsMasterSecretGenerator.java>
>     - Please see above comments for P11TlsKeyMaterialGenerator.java
>
>     <sun/security/pkcs11/P11TlsPrfGenerator.java>
>     - Retrieve label outside of the new code block for TLS 1.2, i.e.
>     line 133- 167, as it's used by all mechanisms.
>
>     <sun/security/pkcs11/P11TlsRsaPremasterSecretGenerator.java>
>     - Line 131, Is it really necessary to use
>     SunTls12RsaPremasterSecret? SunJCE provider uses
>     SunTlsRsaPremasterSecret for all cases. If different algorithm is
>     not needed, then no need to save tlsVersion as a field
>
>     <sun/security/pkcs11/SunPKCS11.java>
>     - Hmm, for TLS 12 specific mechanisms, some PKCS11 libraries may
>     not support them. Instead of registering
>     SunTls12MasterSecret/SunTls12KeyMaterial as aliases, they should
>     be registered separately so that if the native TLS 12 mechanisms
>     were not supported, the new entry will be skipped. The
>     corresponding implementation classes such as
>     P11TlsMasterSecretGenerator and P11TlsKeyMaterialGenerator will
>     have to check the specified parameter spec in their engineInit(..)
>     calls to make sure things line up, e.g. error out if the TLS
>     version in the spec does not match the native mechanism.
>     - Lines 528 - 532, the mapping is stored without checking for
>     support. May become an issue when the underlying PKCS11 library
>     does not support all of these hash mechanisms. Probably not a big
>     deal as these are fairly common hash algorithms, but I think it'd
>     be good to add a comment on line 527 with something like "//
>     assuming all hash mechanisms below are supported".
>
>     <sun/security/pkcs11/wrapper/Functions.java>
>     - Miss the mapping for CKM_TLS12_KEY_AND_MAC_DERIVE? Move these
>     new entries to be after the existing SSL3/TLS entries (from line
>     711-721)
>
>     I still have a few files left and will send you comments on them
>     later.
>     Thanks,
>     Valerie
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180731/fd3acf98/attachment.htm>