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

Tue Jul 31 23:48:59 UTC 2018

I sent this email a bit prematurely. I still need to review TestTLS12.java.

All else is done.

Valerie

On 7/31/2018 4:26 PM, Valerie Peng wrote:
>
> 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/97768ff7/attachment.htm>