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

Thu Jul 26 16:12:02 UTC 2018

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/~mbalao/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>
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/20180726/0ac48e2b/attachment.htm>