Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8301553: Support Password-Based Cryptography in SunPKCS11 #12396

Closed
wants to merge 7 commits into from

Conversation

martinuy
Copy link
Contributor

@martinuy martinuy commented Feb 3, 2023

We would like to propose an implementation for the JDK-8301553: Support Password-Based Cryptography in SunPKCS11 enhancement requirement.

In addition to pursuing the requirement goals and guidelines of JDK-8301553, we want to share the following implementation notes (grouped per altered file):

  • src/java.base/share/classes/com/sun/crypto/provider/HmacPKCS12PBECore.java (modified)

    • This file contains the SunJCE implementation for the PKCS #12 General Method for Password Integrity algorithms. It has been modified with the intent of consolidating all parameter checks in a common file (src/java.base/share/classes/sun/security/util/PBEUtil.java), that can be used both by SunJCE and SunPKCS11. This change does not only serve the purpose of avoiding duplicated code but also ensuring alignment and compatibility between different implementations of the same algorithms. No changes have been made to parameter checks themselves.
    • The new PBEUtil::getPBAKeySpec method introduced for parameters checking takes both a Key and a AlgorithmParameterSpec instance (same as the HmacPKCS12PBECore::engineInit method), and returns a PBEKeySpec instance which consolidates all the data later required to proceed with the computation (password, salt and iteration count).
  • src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java (modified)

    • This file contains the SunJCE implementation for the PKCS #5 Password-Based Encryption Scheme algorithms, which use PBKD2 algorithms underneath for key derivation. In the same spirit than for the HmacPKCS12PBECore case, we decided to consolidate common code for parameters validation and default values in a single file (src/java.base/share/classes/sun/security/util/PBEUtil.java), that can serve both SunJCE and SunPKCS11 and ensure compatibility. However, instead of a single static method at the implementation level (see PBEUtil::getPBAKeySpec), we create an instance of an auxiliary class and invoke an instance method (PBEUtil.PBES2Params::getPBEKeySpec). The reason is to persist parameters data that has to be consistent between calls to PBES2Core::engineInit (in its multiple overloads) and PBES2Core::engineGetParameters, given a single PBES2Core instance. In particular, a call to any of these methods can potentially modify the state in an observable way by means of generating a random IV and a salt. Previous to the proposed patch, this data was persisted in the PBES2Core::ivSpec and PBES2Core::salt instance fields. For compatibility purposes, we decided to preserve SunJCE's current behavior.
  • src/java.base/share/classes/sun/security/util/PBEUtil.java (new file)

    • This utility file contains the PBE parameters checking routines and default values that are used by both SunJCE and SunPKCS11. These routines are invoked from HmacPKCS12PBECore (SunJCE), PBES2Core (SunJCE), P11PBECipher (SunPKCS11) and P11Mac (SunPKCS11). As previously noted, the goals are to avoid duplicate code and to improve compatibility between different security providers that implement PBE algorithms.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java (modified)

    • An utility function to determine if the token is NSS is now called. This function is in a common utility class (P11Util) and invoked from P11Key and P11SecretKeyFactory too.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java (modified)

    • A new type of P11 key is introduced: P11PBEKey. This new type represents a secret key that exists inside the token. Thus, this type inherits from P11SecretKey. At the same time, this type holds data used for key derivation. Thus, this type implements the javax.crypto.interfaces.PBEKey interface. In addition to the conceptual modeling, there are practical advantages of identifying a key by this new P11PBEKey type and holding the data used for derivation: 1) if the key is used in another token (different than the one where it was originally derived), a new derivation must take place; 2) if the key is passed to a non-SunPKCS11 security provider, its key translation method might use derivation data to derive again; and, 3) it's possible to return the PBEKeySpec for the key (see for example P11SecretKeyFactory::engineGetKeySpec).
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java (modified)

    • We decided to integrate PBE algorithms to the existing P11Mac service because the changes required have a low impact on the existing code. When the P11Mac instance is created, we use the algorithm to get PBE key information (if available). Only PBE algorithms have this type of information. In the P11Mac::engineInit method, we now need to handle the PBE service case. In such case, if the key is a P11Key, we check parameters and that the key implements javax.crypto.interfaces.PBEKey by calling PBEUtil::checkKeyParams. In other words, the key has to be a P11PBEKey and the parameters used for its derivation must match the ones passed in the invocation to P11Mac::engineInit. If the key is not a P11Key, a PBE derivation is needed. As for the SunJCE case, we go through parameters processing in PBEUtil::getPBAKeySpec.
    • There are two cases in which we need to call P11SecretKeyFactory::convertKey. One is when the service is not PBE, as we did before the proposed change. In the PBE case, we must call this function because it might be possible that, if the key token is not the same than the service's token, a new key derivation is required.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PBECipher.java (new file)

    • Contrary to the P11Mac case, we decided to separate PBE Cipher from non-PBE Cipher in a different class. There is some additional complexity or gap between the two that we prefer to keep simple. A PBE Cipher uses a non-PBE Cipher service underneath and forwards most of its operations, but adds wrapping code to potentially derive keys during initialization (see P11PBECipher::engineInit). The code associated to key derivation and parameters consistency checking is analogous to the one described for P11Mac.
    • P11PBECipher has a P11PBECipher::engineGetParameters method which calls PBEUtil.PBES2Params::getAlgorithmParameters and can potentially initialize an IV and a salt with a random value, as explained in the comments for PBES2Core.
    • A P11PBECipher service accepts PBE keys only.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java (modified)

    • The first significant change to this class that we want to discuss is the introduction of the KeyInfo class and the refactoring of the previous keyTypes map. Previous to the proposed change, the key information that we needed to retain for key creation at the PKCS #⁠11 level was simple: key algorithm -> PKCS #⁠11 native key type. With PBE, we must consider not only the algorithm name and key type but also (depending on the case) the mechanism that has to be used for derivation, the underlying derivation function and the key length. As an example, to derive a key for the PBEWithHmacSHA512AndAES_256 algorithm, we need to know that this algorithm maps to a PKCS #⁠11 derivation mechanism value of CKM_PKCS5_PBKD2, a derivation function value of CKP_PKCS5_PBKD2_HMAC_SHA512, a derived key type of CKK_AES —so it can be used in an AES Cipher service— and a key length of 256 bits. A new hierarchy of classes to represent these different entries on the mapping structure has been introduced (see KeyInfo, PBEKeyInfo, AESPBEKeyInfo, PBKDF2KeyInfo and P12MacPBEKeyInfo). The methods to add or find entries in the new map have been adjusted. The previous pseudo key types strategy (HMAC, SSLMAC), that allows any key type to be used in a HMAC service, has not been modified.
    • The second significant change to this class was in the P11SecretKeyFactory::convertKey method. When checking if a key can be used in a service —notice here that the service can be any of SecretKeyFactory, Cipher or Mac—, the following rules apply:
      • If the key algorithm matches the service algorithm, the use is allowed
      • If the key algorithm does not match the service algorithm and the service is not one of the pseudo types, further checks are needed. The KeyInfo structure for the key and service algorithms are obtained from the map and the KeyInfo::checkUse method is invoked. The following principles apply to make a decision: 1) PBE services require a javax.crypto.interfaces.PBEKey of the same algorithm —we cannot use an AES key, for example, in a PBEWithHmacSHA512AndAES_256 Cipher service—, 2) PBKD2 keys can be used on any service —there is no information about the key purpose to make a decision— and 3) keys can be used in a service if their underlying type match —as an example, a PBEWithHmacSHA512AndAES_256 PBE key has an underlying type of CKK_AES and can be used in an AES Cipher service—.
    • The third significant change to this class was the addition of the P11SecretKeyFactory::derivePBEKey function. This function does different checks and creates the PKCS #⁠11 structures to make a native call to C_GenerateKey to derive the key. They key returned, in case of success, is of P11PBEKey type. It is worth mentioning that this function is the only path to invoke a native key derivation. It's used not only by the PBE P11SecretKeyFactory service but also by PBE Cipher and PBE Mac services when they need to derive a key.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Util.java (modified)

    • Added some utility functions. One of them is P11Util::encodePassword which serves the purpose of encoding a password in a way that can go through native library truncation (OpenJDK) and reach the PKCS #⁠11 API in the expected encoding. Password encoding must be UTF-8 for PKCS #⁠5 v2.1 derivation and BMPString (UTF-16 big endian) for PKCS #⁠12 v1.1 General Method for Password Integrity derivation. By avoiding a modification to the existing native jCharArrayToCKUTF8CharArray function (p11_util.c), we reduce the risk of breaking existing code.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java (modified)

    • The new PBE algorithms are registered for SunPKCS11 services. It's worth noting that there is an additional (and optional) requiredMechs array to specify a list of native mechanisms that must be available in the token for the service to be enabled. To explain the need for this structure, we will focus on the HmacPBESHA1 Mac service example. On the one hand, we require the CKM_SHA_1_HMAC mechanism to be available in the token because this is, ultimately, a SHA-1 HMAC operation. However, the CKM_PBA_SHA1_WITH_SHA1_HMAC mechanism must be available as well for the preceding PBE key derivation. In the PBKDF2 case, we leverage on this structure to require a mechanism associated to the derivation function. The assumption is that, for example, if the CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC mechanisms are available, a PBKDF2 derivation using HMAC SHA-1 underneath will be available. In this case, the derivation function is represented by the CKP_PKCS5_PBKD2_HMAC_SHA1 constant.
    • As mentioned in JDK-8301553, for some algorithms there isn't a PKCS #⁠11 constant and we use the NSS vendor-specific one. This code can be easily be updated in the future if a constant is introduced to the standard. We don't know if that will ever happen as the newer PKCS #⁠5 derivation for Mac (PBMAC1) might be considered in the future as a PKCS #⁠12 integrity scheme replacement.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_ECDH1_DERIVE_PARAMS.java (modified)

    • Minor comment fix. This mistake was probably the result of using the CK_PKCS5_PBKD2_PARAMS file as a template and forgetting to update the comment.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_MECHANISM.java (modified)

    • New constructors for the CK_PBE_PARAMS, CK_PKCS5_PBKD2_PARAMS and CK_PKCS5_PBKD2_PARAMS2 structures that can be used along with a CK_MECHANISM.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_PBE_PARAMS.java (modified)

    • Some minor adjustments to comments and a constructor to make this class usable with the PKCS #⁠12 General Method for Password Integrity derivation.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_PKCS5_PBKD2_PARAMS.java (modified)

    • Same than for CK_PBE_PARAMS. It is worth noting that this structure is the one used in PKCS #⁠11 revisions previous to v2.40 Errata 01. Given that NSS has decided to keep using it —even when it's not compliant with the latest revisions of the v2.40 and v3.0 standards—, we make an exception for it.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_PKCS5_PBKD2_PARAMS2.java (new file)

    • The new structure for passing PBE parameters to the PKCS #⁠11 token in the PKCS #⁠5 v2.1 derivation scheme.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_X9_42_DH1_DERIVE_PARAMS.java (modified)

    • Same comment than for CK_ECDH1_DERIVE_PARAMS.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11.java (modified)

    • More visibility of major and minor versions of the PKCS #⁠11 standard implemented by a token is needed to decide between the CK_PKCS5_PBKD2_PARAMS and CK_PKCS5_PBKD2_PARAMS2 structures.
  • src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java (modified)

    • New constants added.
  • src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_convert.c (modified)

    • Adjustments made to work with the structures to pass parameters to the token for PBE derivation. It's worth noting that native PBKD2 parameter structures have a tag before the data so we can execute the correct logic to free up resources, once the operation is completed. This is how we differentiate a CK_PKCS5_PBKD2_PARAMS from a CK_PKCS5_PBKD2_PARAMS2 one.
  • src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c (modified)

    • Adjustments to work with the new PBE parameter structures.
    • A bug affecting non-null Java arrays whose length is 0 and need to be converted to native PKCS #⁠11 arrays has been fixed. For these arrays, it was possible that some platforms return NULL as a result of calling memory allocation functions when the size was 0 and an OutOfMemory exception was incorrectly thrown.
  • src/jdk.crypto.cryptoki/share/native/libj2pkcs11/pkcs11wrapper.h (modified)

    • Native constants and structures added.

Test files

  • test/jdk/sun/security/pkcs11/Cipher/PBECipher.java (new file)

    • Tests the PBE Cipher service in SunPKCS11, cross-comparing results against SunJCE (if available) and static data.
    • The PBE Cipher service is tested with different types of keys: derived from data in a PBEParameterSpec, derived with a SunPKCS11 SecretKeyFactory service, derived from data in AlgorithmParameters and derived from data contained in a javax.crypto.interfaces.PBEKey instance.
  • test/jdk/sun/security/pkcs11/KeyStore/ImportKeyToP12.java (new file)

    • Tests that, for several PBE algorithms, PKCS #⁠12 key stores (with Privacy and Integrity) written using SunPKCS11 underneath can be read using SunJCE underneath.
  • test/jdk/sun/security/pkcs11/Mac/MacSameTest.java (modified)

    • This test was not expecting PBE services to be available in the SunPKCS11 security provider, and needs to invoke a new function (PKCS11Test::generateKey) to generate a random key (password).
  • test/jdk/sun/security/pkcs11/Mac/PBAMac.java (new file)

    • Similar to PBECipher but these are the possible types of keys: derived from data in a PBEParameterSpec, derived with a SunPKCS11 SecretKeyFactory service and derived from data contained in a javax.crypto.interfaces.PBEKey instance.
  • test/jdk/sun/security/pkcs11/Mac/ReinitMac.java (modified)

    • Same issue fixed than for MacSameTest.
  • test/jdk/sun/security/pkcs11/PKCS11Test.java (modified)

    • Functions to generate random keys or passwords for PBE and non-PBE algorithms.
  • test/jdk/sun/security/pkcs11/SecretKeyFactory/TestPBKD.java (new file)

    • In addition to testing derived keys for different algorithms against SunJCE and static assertion data, this test asserts: 1) different types of valid and invalid key conversions, and 2) invalid or inconsistent parameters passed for key derivation. Keys are derived with data contained in a PBEKeySpec or in a javax.crypto.interfaces.PBEKey instance.
    • Both an empty and a unicode password, containing a non-ASCII character, are used during this test.

Testing

  • No regressions have been observed in the jdk/sun/security/pkcs11 category (SunPKCS11).

  • No regressions have been observed in the jdk/com/sun/crypto/provider category (SunJCE).

  • No regressions have been observed in the JDK Tier-1 category.

  • Anecdotally, a partial version of the proposed patch containing Cipher and Mac changes is shipped in Red Hat Enterprise Linux builds of OpenJDK 17 since November 2022, without any known issues at this moment.

This contribution is co-authored between @franferrax and @martinuy. We are both under the cover of the OCA agreement per our employer (Red Hat). We look forward to sharing this new feature for the benefit of the broad OpenJDK community and users.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change requires CSR request JDK-8308719 to be approved
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issues

  • JDK-8301553: Support Password-Based Cryptography in SunPKCS11 (Enhancement - "4")
  • JDK-8308719: Support Password-Based Cryptography in SunPKCS11 (CSR)

Reviewers

Contributors

  • Francisco Ferrari Bihurriet <fferrari@redhat.com>
  • Martin Balao <mbalao@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12396/head:pull/12396
$ git checkout pull/12396

Update a local copy of the PR:
$ git checkout pull/12396
$ git pull https://git.openjdk.org/jdk.git pull/12396/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12396

View PR using the GUI difftool:
$ git pr show -t 12396

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12396.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 3, 2023

👋 Welcome back mbalao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 3, 2023
@openjdk
Copy link

openjdk bot commented Feb 3, 2023

@martinuy The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security security-dev@openjdk.org label Feb 3, 2023
@mlbridge
Copy link

mlbridge bot commented Feb 3, 2023

@mlbridge
Copy link

mlbridge bot commented Feb 8, 2023

Mailing list message from Francisco Ferrari Bihurriet on security-dev:

Hi, here is a more plaintext-friendly version of the pull request
Markdown body.

NOTE: I did manual wrapping to get a reasonably good representation in
the archived version of this email [*], sorry if you are reading this in
a <72 characters screen. On the other hand, not wrapping at all would
lead to very long lines in wider screens.

[*]: https://mail.openjdk.org/pipermail/security-dev/2023-February/034438.html

------------------------------------------------------------------------

We would like to propose an implementation for the JDK-8301553: Support
Password-Based Cryptography in SunPKCS11 [1] enhancement requirement.

In addition to pursuing the requirement goals and guidelines of
JDK-8301553 [1], we want to share the following implementation notes
(grouped per altered file):

? src/java.base/share/classes/com/sun/crypto/provider/HmacPKCS12PBECore.java (modified)

? This file contains the SunJCE implementation for the PKCS #12
General Method for Password Integrity [2] algorithms. It has been
modified with the intent of consolidating all parameter checks in a
common file
(src/java.base/share/classes/sun/security/util/PBEUtil.java), that
can be used both by SunJCE and SunPKCS11. This change does not only
serve the purpose of avoiding duplicated code but also ensuring
alignment and compatibility between different implementations of
the same algorithms. No changes have been made to parameter checks
themselves.

? The new PBEUtil::getPBAKeySpec method introduced for parameters
checking takes both a Key and a AlgorithmParameterSpec instance
(same as the HmacPKCS12PBECore::engineInit method), and returns a
PBEKeySpec instance which consolidates all the data later required
to proceed with the computation (password, salt and iteration
count).

? src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java (modified)

? This file contains the SunJCE implementation for the PKCS #5
Password-Based Encryption Scheme [3] algorithms, which use PBKD2
algorithms underneath for key derivation. In the same spirit than
for the HmacPKCS12PBECore case, we decided to consolidate common
code for parameters validation and default values in a single file
(src/java.base/share/classes/sun/security/util/PBEUtil.java), that
can serve both SunJCE and SunPKCS11 and ensure compatibility.
However, instead of a single static method at the implementation
level (see PBEUtil::getPBAKeySpec), we create an instance of an
auxiliary class and invoke an instance method
(PBEUtil.PBES2Params::getPBEKeySpec). The reason is to persist
parameters data that has to be consistent between calls to
PBES2Core::engineInit (in its multiple overloads) and
PBES2Core::engineGetParameters, given a single PBES2Core instance.
In particular, a call to any of these methods can potentially
modify the state in an observable way by means of generating a
random IV and a salt. Previous to the proposed patch, this data was
persisted in the PBES2Core::ivSpec and PBES2Core::salt instance
fields. For compatibility purposes, we decided to preserve SunJCE's
current behavior.

? src/java.base/share/classes/sun/security/util/PBEUtil.java (new file)

? This utility file contains the PBE parameters checking routines and
default values that are used by both SunJCE and SunPKCS11. These
routines are invoked from HmacPKCS12PBECore (SunJCE), PBES2Core
(SunJCE), P11PBECipher (SunPKCS11) and P11Mac (SunPKCS11). As
previously noted, the goals are to avoid duplicate code and to
improve compatibility between different security providers that
implement PBE algorithms.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java (modified)

? An utility function to determine if the token is NSS is now called.
This function is in a common utility class (P11Util) and invoked
from P11Key and P11SecretKeyFactory too.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java (modified)

? A new type of P11 key is introduced: P11PBEKey. This new type
represents a secret key that exists inside the token. Thus, this
type inherits from P11SecretKey. At the same time, this type holds
data used for key derivation. Thus, this type implements the
javax.crypto.interfaces.PBEKey interface. In addition to the
conceptual modeling, there are practical advantages of identifying
a key by this new P11PBEKey type and holding the data used for
derivation: 1) if the key is used in another token (different than
the one where it was originally derived), a new derivation must
take place; 2) if the key is passed to a non-SunPKCS11 security
provider, its key translation method might use derivation data to
derive again; and, 3) it's possible to return the PBEKeySpec for
the key (see for example P11SecretKeyFactory::engineGetKeySpec).

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Mac.java (modified)

? We decided to integrate PBE algorithms to the existing P11Mac
service because the changes required have a low impact on the
existing code. When the P11Mac instance is created, we use the
algorithm to get PBE key information (if available). Only PBE
algorithms have this type of information. In the P11Mac::engineInit
method, we now need to handle the PBE service case. In such case,
if the key is a P11Key, we check parameters and that the key
implements javax.crypto.interfaces.PBEKey by calling
PBEUtil::checkKeyParams. In other words, the key has to be a
P11PBEKey and the parameters used for its derivation must match the
ones passed in the invocation to P11Mac::engineInit. If the key is
not a P11Key, a PBE derivation is needed. As for the SunJCE case,
we go through parameters processing in PBEUtil::getPBAKeySpec.

? There are two cases in which we need to call
P11SecretKeyFactory::convertKey. One is when the service is not
PBE, as we did before the proposed change. In the PBE case, we must
call this function because it might be possible that, if the key
token is not the same than the service's token, a new key
derivation is required.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PBECipher.java (new file)

? Contrary to the P11Mac case, we decided to separate PBE Cipher from
non-PBE Cipher in a different class. There is some additional
complexity or gap between the two that we prefer to keep simple. A
PBE Cipher uses a non-PBE Cipher service underneath and forwards
most of its operations, but adds wrapping code to potentially
derive keys during initialization (see P11PBECipher::engineInit).
The code associated to key derivation and parameters consistency
checking is analogous to the one described for P11Mac.

? P11PBECipher has a P11PBECipher::engineGetParameters method which
calls PBEUtil.PBES2Params::getAlgorithmParameters and can
potentially initialize an IV and a salt with a random value, as
explained in the comments for PBES2Core.

? A P11PBECipher service accepts PBE keys only.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java (modified)

? The first significant change to this class that we want to discuss
is the introduction of the KeyInfo class and the refactoring of the
previous keyTypes map. Previous to the proposed change, the key
information that we needed to retain for key creation at the PKCS
#?11 level was simple: key algorithm -> PKCS #?11 native key type.
With PBE, we must consider not only the algorithm name and key type
but also (depending on the case) the mechanism that has to be used
for derivation, the underlying derivation function and the key
length. As an example, to derive a key for the
PBEWithHmacSHA512AndAES_256 algorithm, we need to know that this
algorithm maps to a PKCS #?11 derivation mechanism value of
CKM_PKCS5_PBKD2, a derivation function value of
CKP_PKCS5_PBKD2_HMAC_SHA512, a derived key type of CKK_AES ?so it
can be used in an AES Cipher service? and a key length of 256 bits.
A new hierarchy of classes to represent these different entries on
the mapping structure has been introduced (see KeyInfo, PBEKeyInfo,
AESPBEKeyInfo, PBKDF2KeyInfo and P12MacPBEKeyInfo). The methods to
add or find entries in the new map have been adjusted. The previous
pseudo key types strategy (HMAC, SSLMAC), that allows any key type
to be used in a HMAC service, has not been modified.

? The second significant change to this class was in the
P11SecretKeyFactory::convertKey method. When checking if a key can
be used in a service ?notice here that the service can be any of
SecretKeyFactory, Cipher or Mac?, the following rules apply:

? If the key algorithm matches the service algorithm, the use is
allowed

? If the key algorithm does not match the service algorithm and the
service is not one of the pseudo types, further checks are
needed. The KeyInfo structure for the key and service algorithms
are obtained from the map and the KeyInfo::checkUse method is
invoked. The following principles apply to make a decision: 1)
PBE services require a javax.crypto.interfaces.PBEKey of the same
algorithm ?we cannot use an AES key, for example, in a
PBEWithHmacSHA512AndAES_256 Cipher service?, 2) PBKD2 keys can be
used on any service ?there is no information about the key
purpose to make a decision? and 3) keys can be used in a service
if their underlying type match ?as an example, a
PBEWithHmacSHA512AndAES_256 PBE key has an underlying type of
CKK_AES and can be used in an AES Cipher service?.

? The third significant change to this class was the addition of the
P11SecretKeyFactory::derivePBEKey function. This function does
different checks and creates the PKCS #?11 structures to make a
native call to C_GenerateKey to derive the key. They key returned,
in case of success, is of P11PBEKey type. It is worth mentioning
that this function is the only path to invoke a native key
derivation. It's used not only by the PBE P11SecretKeyFactory
service but also by PBE Cipher and PBE Mac services when they need
to derive a key.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Util.java (modified)

? Added some utility functions. One of them is
P11Util::encodePassword which serves the purpose of encoding a
password in a way that can go through native library truncation
(OpenJDK) and reach the PKCS #?11 API in the expected encoding.
Password encoding must be UTF-8 for PKCS #?5 v2.1 derivation and
BMPString (UTF-16 big endian) for PKCS #?12 v1.1 General Method for
Password Integrity derivation. By avoiding a modification to the
existing native jCharArrayToCKUTF8CharArray function (p11_util.c),
we reduce the risk of breaking existing code.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java (modified)

? The new PBE algorithms are registered for SunPKCS11 services. It's
worth noting that there is an additional (and optional)
requiredMechs array to specify a list of native mechanisms that
must be available in the token for the service to be enabled. To
explain the need for this structure, we will focus on the
HmacPBESHA1 Mac service example. On the one hand, we require the
CKM_SHA_1_HMAC mechanism to be available in the token because this
is, ultimately, a SHA-1 HMAC operation. However, the
CKM_PBA_SHA1_WITH_SHA1_HMAC mechanism must be available as well for
the preceding PBE key derivation. In the PBKDF2 case, we leverage
on this structure to require a mechanism associated to the
derivation function. The assumption is that, for example, if the
CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC mechanisms are available, a
PBKDF2 derivation using HMAC SHA-1 underneath will be available. In
this case, the derivation function is represented by the
CKP_PKCS5_PBKD2_HMAC_SHA1 constant.

? As mentioned in JDK-8301553 [1], for some algorithms there isn't a
PKCS #?11 constant and we use the NSS vendor-specific one. This
code can be easily be updated in the future if a constant is
introduced to the standard. We don't know if that will ever happen
as the newer PKCS #?5 derivation for Mac (PBMAC1) might be
considered in the future as a PKCS #?12 integrity scheme
replacement.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_ECDH1_DERIVE_PARAMS.java (modified)

? Minor comment fix. This mistake was probably the result of using
the CK_PKCS5_PBKD2_PARAMS file as a template and forgetting to
update the comment.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_MECHANISM.java (modified)

? New constructors for the CK_PBE_PARAMS, CK_PKCS5_PBKD2_PARAMS and
CK_PKCS5_PBKD2_PARAMS2 structures that can be used along with a
CK_MECHANISM.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_PBE_PARAMS.java (modified)

? Some minor adjustments to comments and a constructor to make this
class usable with the PKCS #?12 General Method for Password
Integrity derivation.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_PKCS5_PBKD2_PARAMS.java (modified)

? Same than for CK_PBE_PARAMS. It is worth noting that this structure
is the one used in PKCS #?11 revisions previous to v2.40 Errata 01.
Given that NSS has decided to keep using it ?even when it's not
compliant with the latest revisions of the v2.40 and v3.0
standards?, we make an exception for it.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_PKCS5_PBKD2_PARAMS2.java (new file)

? The new structure for passing PBE parameters to the PKCS #?11 token
in the PKCS #?5 v2.1 derivation scheme.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_X9_42_DH1_DERIVE_PARAMS.java (modified)

? Same comment than for CK_ECDH1_DERIVE_PARAMS.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11.java (modified)

? More visibility of major and minor versions of the PKCS #?11
standard implemented by a token is needed to decide between the
CK_PKCS5_PBKD2_PARAMS and CK_PKCS5_PBKD2_PARAMS2 structures.

? src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java (modified)

? New constants added.

? src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_convert.c (modified)

? Adjustments made to work with the structures to pass parameters to
the token for PBE derivation. It's worth noting that native PBKD2
parameter structures have a tag before the data so we can execute
the correct logic to free up resources, once the operation is
completed. This is how we differentiate a CK_PKCS5_PBKD2_PARAMS
from a CK_PKCS5_PBKD2_PARAMS2 one.

? src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c (modified)

? Adjustments to work with the new PBE parameter structures.

? A bug affecting non-null Java arrays whose length is 0 and need to
be converted to native PKCS #?11 arrays has been fixed. For these
arrays, it was possible that some platforms return NULL as a result
of calling memory allocation functions when the size was 0 and an
OutOfMemory exception was incorrectly thrown.

? src/jdk.crypto.cryptoki/share/native/libj2pkcs11/pkcs11wrapper.h (modified)

? Native constants and structures added.

Test files

? test/jdk/sun/security/pkcs11/Cipher/PBECipher.java (new file)

? Tests the PBE Cipher service in SunPKCS11, cross-comparing results
against SunJCE (if available) and static data.

? The PBE Cipher service is tested with different types of keys:
derived from data in a PBEParameterSpec, derived with a SunPKCS11
SecretKeyFactory service, derived from data in AlgorithmParameters
and derived from data contained in a javax.crypto.interfaces.PBEKey
instance.

? test/jdk/sun/security/pkcs11/KeyStore/ImportKeyToP12.java (new file)

? Tests that, for several PBE algorithms, PKCS #?12 key stores (with
Privacy and Integrity) written using SunPKCS11 underneath can be
read using SunJCE underneath.

? test/jdk/sun/security/pkcs11/Mac/MacSameTest.java (modified)

? This test was not expecting PBE services to be available in the
SunPKCS11 security provider, and needs to invoke a new function
(PKCS11Test::generateKey) to generate a random key (password).

? test/jdk/sun/security/pkcs11/Mac/PBAMac.java (new file)

? Similar to PBECipher but these are the possible types of keys:
derived from data in a PBEParameterSpec, derived with a SunPKCS11
SecretKeyFactory service and derived from data contained in a
javax.crypto.interfaces.PBEKey instance.

? test/jdk/sun/security/pkcs11/Mac/ReinitMac.java (modified)

? Same issue fixed than for MacSameTest.

? test/jdk/sun/security/pkcs11/PKCS11Test.java (modified)

? Functions to generate random keys or passwords for PBE and non-PBE
algorithms.

? test/jdk/sun/security/pkcs11/SecretKeyFactory/TestPBKD.java (new
file)

? In addition to testing derived keys for different algorithms
against SunJCE and static assertion data, this test asserts: 1)
different types of valid and invalid key conversions, and 2)
invalid or inconsistent parameters passed for key derivation. Keys
are derived with data contained in a PBEKeySpec or in a
javax.crypto.interfaces.PBEKey instance.

? Both an empty and a unicode password, containing a non-ASCII
character, are used during this test.

Testing

? No regressions have been observed in the jdk/sun/security/pkcs11
category (SunPKCS11).

? No regressions have been observed in the jdk/com/sun/crypto/provider
category (SunJCE).

? No regressions have been observed in the JDK Tier-1 category.

? Anecdotally, a partial version of the proposed patch containing
Cipher and Mac changes is shipped in Red Hat Enterprise Linux builds
of OpenJDK 17 since November 2022, without any known issues at this
moment.

This contribution is co-authored between fferrari at redhat.com and
mbalao at redhat.com. We are both under the cover of the OCA agreement per
our employer (Red Hat). We look forward to sharing this new feature for
the benefit of the broad OpenJDK community and users.

[1] https://bugs.openjdk.org/browse/JDK-8301553
[2] https://datatracker.ietf.org/doc/html/rfc7292#appendix-B
[3] https://datatracker.ietf.org/doc/html/rfc8018#section-6.2

------------------------------------------------------------------------

Regards,
--
Francisco Ferrari Bihurriet <fferrari at redhat.com>
Senior Software Engineer
Red Hat Argentina (https://www.redhat.com)

A00A 468A 5506 5D16 9851 E3A9 CD8B C3EF 772B BD6F

Comment on lines 163 to 165
getInfo();
this.version.major = pInfo.cryptokiVersion.major;
this.version.minor = pInfo.cryptokiVersion.minor;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you added getInfo(), then the C_GetInfo() could be made private, so that all callers can just leverage getInfo(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! We will make C_GetInfo private and delete the overload in SynchronizedPKCS11 for the next iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have analyzed this issue further and think that we need one more iteration. It's possible that the constructor of PKCS11 finishes with PKCS11::pInfo in null. In fact, it's very likely to happen and PKCS11::getInfo is not multi-thread safe. The second problem is that it's possible that a SyncrhonizedPKCS11 instance ends up calling a non-synchronized version of PKCS11::C_GetInfo, if we remove the override. The reason why we added PKCS11::getInfo in the first place was because PKCS11::getVersion is not currently available in 17u —where this feature was originally developed— and there were 2 calls to PKCS11::C_GetInfo. We can safely undo the PKCS11::getInfo change and call PKCS11::getVersion in P11SecretKeyFactory::derivePBEKey.

Comment on lines 1184 to 1192
// Additional PKCS #12 PBE key derivation algorithms defined in NSS v3.29
public static final long CKM_NSS_PKCS12_PBE_SHA224_HMAC_KEY_GEN
/* (CKM_NSS + 29) */ = 0xCE53436DL;
public static final long CKM_NSS_PKCS12_PBE_SHA256_HMAC_KEY_GEN
/* (CKM_NSS + 30) */ = 0xCE53436EL;
public static final long CKM_NSS_PKCS12_PBE_SHA384_HMAC_KEY_GEN
/* (CKM_NSS + 31) */ = 0xCE53436FL;
public static final long CKM_NSS_PKCS12_PBE_SHA512_HMAC_KEY_GEN
/* (CKM_NSS + 32) */ = 0xCE534370L;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For user-friendly sake, now that you added these mechanisms, you should add the string name mapping for these native NSS mechanisms into the sun.security.pkcs11.wrapper.Functions class through its addMech(long, String) method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. As part of the next iteration we will fix that and better align the order, grouping NSS mechanisms together in PKCS11Constants.java.

Copy link
Contributor Author

@martinuy martinuy Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We realized that strings for Pseudo-random function (CKP_) and Salt/Encoding parameter (CKZ_) constants were missing. We added it to Functions.java, and modified CK_PKCS5_PBKD2_PARAMS and CK_PKCS5_PBKD2_PARAMS2 classes to use them.

@@ -68,6 +68,7 @@
/* extra PKCS#11 constants not in the standard include files */

#define CKA_NETSCAPE_BASE (0x80000000 + 0x4E534350)
/* ^ now known as CKM_NSS (CKM_VENDOR_DEFINED | NSSCK_VENDOR_NSS) */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: " ^" part seems un-necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.

* CK_VOID_PTR pSaltSourceData;
* CK_ULONG ulSaltSourceDataLen;
* CK_ULONG iterations;
* CK_PKCS5_PBKD2_PSEUDO_RANDOM_FUNCTION_TYPE prf;
* CK_VOID_PTR pPrfData;
* CK_ULONG ulPrfDataLen;
* CK_UTF8CHAR_PTR pPassword;
* CK_ULONG_PTR ulPasswordLen;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not match the one in PKCS#11 spec, the 'ulPasswordLen' should be CK_ULONG type.
I see that you added another CK_PKCS5_PBKD2_PARAMS class matching the spec definition. Is this to work around some existing bug? It seems strange to put the inconsistent type in the original class and the correct one in the new class.

Copy link
Contributor

@franferrax franferrax Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @valeriepeng. There's been a typo in the CK_PKCS5_PBKD2_PARAMS structure for some time. Apparently this led to divergences in token implementations, where some considered ulPasswordLen as CK_ULONG (the intention) and others, including NSS, considered it as CK_ULONG_PTR (the typo, but trying to follow the standard verbatim).

This was fixed in PKCS#11 v2.40 errata 01 by introducing the new CK_PKCS5_PBKD2_PARAMS2 structure.

The PKCS#11 v3.0 Oasis published version of pkcs11t.h and also OpenJDK's pkcs11t.h define both structures:

/* CK_PKCS5_PBKD2_PARAMS is a structure that provides the
* parameters to the CKM_PKCS5_PBKD2 mechanism.
*/
typedef struct CK_PKCS5_PBKD2_PARAMS {
CK_PKCS5_PBKDF2_SALT_SOURCE_TYPE saltSource;
CK_VOID_PTR pSaltSourceData;
CK_ULONG ulSaltSourceDataLen;
CK_ULONG iterations;
CK_PKCS5_PBKD2_PSEUDO_RANDOM_FUNCTION_TYPE prf;
CK_VOID_PTR pPrfData;
CK_ULONG ulPrfDataLen;
CK_UTF8CHAR_PTR pPassword;
CK_ULONG_PTR ulPasswordLen;
} CK_PKCS5_PBKD2_PARAMS;
typedef CK_PKCS5_PBKD2_PARAMS CK_PTR CK_PKCS5_PBKD2_PARAMS_PTR;
/* CK_PKCS5_PBKD2_PARAMS2 is a corrected version of the CK_PKCS5_PBKD2_PARAMS
* structure that provides the parameters to the CKM_PKCS5_PBKD2 mechanism
* noting that the ulPasswordLen field is a CK_ULONG and not a CK_ULONG_PTR.
*/
typedef struct CK_PKCS5_PBKD2_PARAMS2 {
CK_PKCS5_PBKDF2_SALT_SOURCE_TYPE saltSource;
CK_VOID_PTR pSaltSourceData;
CK_ULONG ulSaltSourceDataLen;
CK_ULONG iterations;
CK_PKCS5_PBKD2_PSEUDO_RANDOM_FUNCTION_TYPE prf;
CK_VOID_PTR pPrfData;
CK_ULONG ulPrfDataLen;
CK_UTF8CHAR_PTR pPassword;
CK_ULONG ulPasswordLen;
} CK_PKCS5_PBKD2_PARAMS2;

Also, nowadays NSS still uses the deprecated CK_PKCS5_PBKD2_PARAMS instead of the new and recommended CK_PKCS5_PBKD2_PARAMS2, thus we defined both CK_PKCS5_PBKD2_PARAMS.java and CK_PKCS5_PBKD2_PARAMS2.java.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks much for the explanation~

return ivSpec;
}

public AlgorithmParameters getAlgorithmParameters(int blkSize,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see most if not all of the methods in this class are lifted from other classes as the result of code refactoring. Would be nice to document its callers, e.g. where it's used.

Copy link
Contributor Author

@martinuy martinuy Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added references, that include the security provider and class, to each method and inner class in PBEUtil. The instance methods in PBEUtil$PBES2Params use the reference for their containing class. Let me know if you still want to repeat the reference for each one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We finally decided to include a general comment in the PBEUtil$PBES2Params class and also comments in each method. We also added comments to other PBEUtil methods.

Comment on lines 487 to 493
private static void dA(String type, String algorithm, String className,
int[] m, int[] requiredMechs) {
register(new Descriptor(type, algorithm, className,
getAliases(algorithm), m, requiredMechs));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this to after the other dA(String, String, String, int[]) below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

* | | | |
* PKCS #11 call (bytes) => [ 0x00, 0x61, 0x00, 0x00 ]
*/
byte[] passwordBytes = new String(password).getBytes(cs);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to not storing the password into a String object. Perhaps use a CharBuffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

Comment on lines 1142 to 1152
// PBKDF2 support, used in P11Util
public static final long CKZ_SALT_SPECIFIED = 0x00000001L;
public static final long CKP_PKCS5_PBKD2_HMAC_SHA1 = 0x00000001L;
public static final long CKP_PKCS5_PBKD2_HMAC_GOSTR3411 = 0x00000002L;
public static final long CKP_PKCS5_PBKD2_HMAC_SHA224 = 0x00000003L;
public static final long CKP_PKCS5_PBKD2_HMAC_SHA256 = 0x00000004L;
public static final long CKP_PKCS5_PBKD2_HMAC_SHA384 = 0x00000005L;
public static final long CKP_PKCS5_PBKD2_HMAC_SHA512 = 0x00000006L;
public static final long CKP_PKCS5_PBKD2_HMAC_SHA512_224 = 0x00000007L;
public static final long CKP_PKCS5_PBKD2_HMAC_SHA512_256 = 0x00000008L;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of moving these PBKDF2 constants out of the commented section, how about preserving the ordering and just break the commented section into two sections? IIRC, this is easier to check/compare to pkcs11t.h by following the same order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

case CKM_NSS_PKCS12_PBE_SHA224_HMAC_KEY_GEN:
case CKM_NSS_PKCS12_PBE_SHA256_HMAC_KEY_GEN:
case CKM_NSS_PKCS12_PBE_SHA384_HMAC_KEY_GEN:
case CKM_NSS_PKCS12_PBE_SHA512_HMAC_KEY_GEN:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about CKM_PBE_SHA1_DES3_EDE_CBC and CKM_PBE_SHA1_DES2_EDE_CBC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. While CKM_PBE_SHA1_DES3_EDE_CBC and CKM_PBE_SHA1_DES2_EDE_CBC mechanisms were never supported, there was dead code in jMechParamToCKMechParamPtrSlow (p11_convert.c) for them before this enhancement proposal. In fact, CK_PBE_PARAMS (Java and C classes) existed without any uses, probably added for the sake of comprehensiveness. This code had issues that we had to fix when actually using it. It's not our intention to include legacy derivation mechanisms, such as PBEWithSHA1AndDESede, at this time in the scope. I'll propose to remove all references to CKM_PBE_SHA1_DES3_EDE_CBC and CKM_PBE_SHA1_DES2_EDE_CBC in jMechParamToCKMechParamPtrSlow so it don't lead to confusion.

&& (*env)->IsInstanceOf(env, jParam, jPkcs5Pbkd2ParamsClass)) {
paramVersion = PARAMS2;
} else {
return NULL;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning NULL, should some exception be thrown to indicate that there is a problem with the specified parameter (not the expected type)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, so the native call with a null CK_MECHANISM pParameter is avoided. This path should never happen, though.

this.keyType = keyType;
}

static boolean checkUse(KeyInfo ki, KeyInfo si) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comment on ki and si and how this method is used is needed as it's not obvious when looking at the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add an upper comment explaining the purpose of the checkUse method and a comment explaining the ki.keyType == si.keyType path in particular.

// so any service is allowed in principle.
return true;
}
return ki.keyType == si.keyType;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, for non-PBE key info, algos do not have to match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For services and keys cases in which algorithms identity-match —irrespective if they are PBE or non-PBE—, KeyInfo::checkUse is not called and execution moves forward as if the check passed (see here). The same is true for services that accept any key type, such as those whose pseudo types are PCKK_HMAC or PCKK_SSLMAC.

The ki.keyType == si.keyType success value affects cases in which algorithms are different but it's still possible to use the key in the service. One example that would hit this path is a PBE key derived for AES that it's used in an AES Cipher service. For non-PBE keys and services cases, one example is algorithms "RC4" and "ARCFOUR" that have both the underlying CKK_RC4 key type. Notice that this latter case is not new: previous to this enhancement proposal, key types were compared as well (see here).

For non-PBE keys and services cases, what is new with this enhancement is to accept them if their algorithms are identity-equal. This condition necessarily means that key types are equal —the opposite is obviously not true—. One minor detail, when we refer to the algorithms equality trivial pass condition, it's an object identity comparison for performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented here, I'll add a comment to the code explaining this execution path.

static long getPKCS11KeyType(String algorithm) {
long kt = getKeyType(algorithm);
if (kt == -1 || kt > PCKK_ANY) {
// Replace pseudo key type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it seems clearer to have the "No pseudo key types" comment as method comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, moved as a method comment and replaced by the suggested wording.

public final class PBEUtil {

// Used by SunJCE and SunPKCS11
public final static class PBES2Params {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class confuses me. An instance is constructed without any parameter. The method getPBEKeySpec is able to mutate it and return something, the method getAlgorithmParameters is able to read the content and possibly mutate again and return something, and finally another method getIvSpec is able to return the content.

Please either precisely document how these methods will be called and enforce this rule with internal checks, or refactor it to something immutable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class mutates because we have to keep state that may change an undefined number of times. This is not new, in the previous code state was kept as well. The only difference is that instead of keeping the state spread across different fields —which would now require mirroring fields between SunJCE and SunPKCS11 sides—, we have it aggregated in a single class. We are also proposing that the methods that change this state —which are used equally from SunJCE and SunPKCS11— are also part of the same class. The intention is to help with consistency and code reuse.

I'll give a brief reminder of how state changes work, even before the PBE enhancement. When engineInit is called, parameter values (passed, defaults or randomly generated) will change the state. All following calls to engineGetParameters must return the same state. When engineInit is called again —with the same CipherSpi instance—, state is re-initialized and the previous sequence repeats. The only special case is when engineGetParameters is called before the first call to engineInit. In such case, engineGetParameters generates the initial state and this state will be overwritten by the first call to engineInit later.

I'll add comments to the class to help explaining this logic.

Comment on lines -136 to -138
if (key == null) {
throw new InvalidKeyException("Key must not be null");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for removing this code block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key has to be an instance of SecretKey (see here), so a null value would lead to an InvalidKeyException anyways. We proposed to remove it in the interest of simplifying/shortening the code.

Comment on lines 230 to 233
putKeyInfo(new P12MacPBEKeyInfo("HmacPBESHA512/224",
CKM_NSS_PKCS12_PBE_SHA512_HMAC_KEY_GEN, 512));
putKeyInfo(new P12MacPBEKeyInfo("HmacPBESHA512/256",
CKM_NSS_PKCS12_PBE_SHA512_HMAC_KEY_GEN, 512));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these key lengths really 512? Or should they match the output size as in other key infos?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Looks like a bug. The values should probably be 224 and 256 respectively (output sizes). @franferrax what do you think? We can trace the CKA_VALUE_LEN for these mechanisms in the NSS Software Token to verify it. Also, we should explore if it's possible to add a test for HmacPBESHA512/224 and HmacPBESHA512/256 to TestPBKD. I'll make the change but leave this comment open until we further explore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that the underlying native mechanism is the same so I'm not even sure that the NSS Software Token will truncate the output as we expect. This reinforces the need for further exploration and testing. We may need to remove support for these algorithms.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've given it another look, and realized that in fact this was left there by mistake. I initially thought that we could implement those algorithms by just truncating the CKM_NSS_PKCS12_PBE_SHA512_HMAC_KEY_GEN derivation output from 512 to 224 or 256 bits, but never moved ahead with that implementation.

This chosen keyLen divided by 8 ends up passed as the CKA_VALUE_LEN attribute, which the PKCS #11 token can ignore and just use 64 (512 / 8), given the mechanism itself indicates the key length. NSS does this, see pkcs11c.c:4655-4659 and pkcs11c.c:4421. So changing 512 to 224 or 256 doesn't have any effect.

On the other hand, re-checking this, even if I had developed truncation of the derived key, it wouldn't have worked, because is the output of the pseudo-random hash function what should be truncated instead, in every iteration and step of the derivation.

For this reason, we decided to remove the HmacPBESHA512/224 and HmacPBESHA512/256 algorithms from the proposal.

@@ -65,6 +67,9 @@ final class P11Mac extends MacSpi {
// algorithm name
private final String algorithm;

// PBEKeyInfo if algorithm is PBE, otherwise null

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: PBE-related, not just PBE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

p11Key = P11SecretKeyFactory.convertKey(token, key, algorithm);
if (svcPbeKi != null) {
if (key instanceof P11Key) {
params = PBEUtil.checkKeyParams(key, params, algorithm);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems strange that you check the key to be instanceof P11Key but then inside PBEUtil.checkKeyParams, it errors out if the key instanceof PBEKey. Maybe you meant to check if the key is an instanceof P11PBEKey? Could the key be a PBEKey but not P11Key and contains more than just password? I don't quite follow the logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can only pass PBEKey keys to PBE Mac services.

If the key is a P11Key (PBEKey + P11Key == P11PBEKey), derivation is not needed. We just check that the P11Key came from a PBE derivation inside PBEUtil.checkKeyParams and that's it. This is for consistency enforcement only: the token does not care the P11Key origin as long as it has the right attributes to calculate the HMAC.

If the key is not a P11Key, we have to derive it. We just check that the key implements the PBEKey interface, so there is data for derivation. The derived key will be a P11Key (in particular, a P11PBEKey).

Thus, the if here should be read as: are we receiving an already derived key (true) or do we have to derive it (false)? In any case, apply the corresponding checks and move forward.

I'll add a comment to the code to make this logic more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the new comments, we finally implemented a couple of minor changes for clarity and to enforce a check that was previously omitted:

  • PBEUtil.checkKeyParams was renamed to PBEUtil.checkKeyAndParams and does not longer return the underlying service params (PBEParameterSpec.paramSpec). It's only checking that the key implements the PBEKey interface and that params, if an instance of PBEParameterSpec, are consistent with the key's derivation data. We added a comment in PBEUtil.java too, where PBEUtil.checkKeyParams is defined.
  • For all PBE cases in which params is an instance of PBEParameterSpec, the underlying service params value (PBEParameterSpec.paramSpec) is obtained and assigned to params. Notice that this was not done before in derivation cases.
  • Now, we enforce the check that the underlying service params is null even in those cases in which we derived.

We modified P11PBECipher to be aligned to these changes, and added equivalent comments there.

Comment on lines 217 to 238
if (svcPbeKi == null || key instanceof P11Key) {
if (params != null) {
throw new InvalidAlgorithmParameterException
("Parameters not supported");
}
p11Key = P11SecretKeyFactory.convertKey(token, key, algorithm);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to handle Non-PBE-related case? The logic here seems conflicting with the above code block where the params must be null. Also if key is instanceof P11Key already, why are you calling P11SecretKeyFactory.convertKey(...) again? Strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quoted block handles non-PBE cases (svcPbeKi == null) and PBE cases in which the derivation was not required (svcPbeKi != null && key instanceof P11Key). The only cases left out of the block are PBE ones in which the key had to be derived because no conversion is needed.

If we enter the block in the non-PBE case, params must be null like before to this enhancement (see here).

If we enter the block in the PBE case (derivation not required), params is what PBEUtil::checkKeyParams returned. This function returns the PBEParameterSpec::paramSpec field of the passed PBEParameterSpec instance, which is always null except for the PBE Cipher cases in which the IV can be specified. Given that this is a Mac service, a non-null value there is not expected. We check this for consistency.

We are calling P11SecretKeyFactory::convertKey if the key is a P11Key because it might be a P11Key from a token different than the P11Mac service one (the condition here could be false). If that's the case, a new derivation in the P11Mac service token will be done. Notice that this is a little caveat to my previous comment here: a P11Key might still require a derivation.

I'll add comments to the code to make this more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We made a subtle modification to this code that does not change execution paths —except for checking params against null in derivation cases, as commented here— but hopefully helps with clarity. In particular:

  • The condition to try conversion is now p11Key == null. Conversion is tried in non-PBE cases and PBE cases in which derivation was not done, as commented here.
  • The check of params against null is now previous to the conversion block, and applies to all cases (including those in which we derived).

@martinuy
Copy link
Contributor Author

In addition to all inline comments, our next iteration will contain minor improvements validation of static assertion data in TestPBKD.java, PBAMac.java and PBECipher.java tests.

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 19, 2023

@martinuy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@martinuy
Copy link
Contributor Author

Please keep this open.

Co-authored-by: Francisco Ferrari <fferrari@redhat.com>
Co-authored-by: Martin Balao <mbalao@redhat.com>
// It's guaranteed that when engineInit succeeds, the key length
// for the underlying cipher is equal to the PBE service key length.
// Otherwise, initialization fails.
return svcPbeKi.keyLen;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method "Returns the key size of the given key object in bits. "
How do you ensure that this key is the one used in the initialization? This method may also throw InvalidKeyException though, With this impl, even if passing a null key, this would return an int value and not detecting the key is invalid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valeriepeng: the rationale behind this decision is based on the only usage of engineGetKeySize(), which corresponds to a crypto.policy=limited Cryptographic Jurisdiction Policy. Here, engineGetKeySize() is employed to check the key during the Cipher initialization (see Cipher.java:1463, Cipher.java:1110, Cipher.java:1139). So, what we can assert is that if the key size isn't going to be svcPbeKi.keyLen, then that very same Cipher initialization will later fail anyway.

There also is a previous precedent set by SunJCE:

protected int engineGetKeySize(Key key) throws InvalidKeyException {
return keyLength;
}

protected int engineGetKeySize(Key key) throws InvalidKeyException {
return 168;
}

@Override
protected int engineGetKeySize(Key key) throws InvalidKeyException {
return 112; // effective key size
}

protected int engineGetKeySize(Key key) throws InvalidKeyException {
// Always returns 56 since the encryption key
// is a DES key.
return 56;
}

NOTE: by passing -Djava.security.properties=<(echo crypto.policy=limited) as a JVM argument to test/jdk/sun/security/pkcs11/Cipher/PBECipher.java, I could reproduce a failure of the older implementation using the underlying cipher: if we pass a com.sun.crypto.provider.PBEKey of the PBEPBEWithMD5AndDES algorithm, the underlying AES cipher's P11SecretKeyFactory.convertKey() call fails because this algorithm is not registered in the P11SecretKeyFactory.keyInfo map, so ki is null here, resulting in InvalidKeyException: Cannot use a PBEWithMD5AndDES key for a AES service.

I've also found a failure for the current implementation when using crypto.policy=limited, I will investigate this further tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that the failures I was seeing were caused by the 128 bits key size limit here:

permission javax.crypto.CryptoPermission *, 128;

If I set that to 512, test/jdk/sun/security/pkcs11/Cipher/PBECipher.java passes with crypto.policy=limited.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @franferrax. This internal API was a bit confusing to me because a key is passed to obtain a key size but the key itself can be converted/derived right before use in the service, so the service can affect the real key size. This goes beyond the SunPKCS11 implementation. The rationale that we applied was to return the key size that a successful initialization of the service would have in all cases. There is only one caller, which will pass the same key to engineGetKeySize and to the service initialization.

@franferrax
Copy link
Contributor

@seanjmullan @valeriepeng , can you please take a look at the proposed JDK-8308719 CSR?

Under the "Specification" section, the Cipher ones list out the required mech(s). But the rest, e.g. SecretKeyFactory and Mac, have X and Y. Based on the code in SunPKCS11.java, it seems Y is the required one and X is the one which is used.

@valeriepeng: I think there are a couple of things worth clarifying. Let's take one example of each service type.

Cipher.PBEWithHmacSHA1AndAES_128

d(CIP, "PBEWithHmacSHA1AndAES_128", P11PBECipher,
m(CKM_AES_CBC_PAD, CKM_AES_CBC),
m(CKM_PKCS5_PBKD2, CKM_SHA_1_HMAC));

Here, one of CKM_AES_CBC_PAD or CKM_AES_CBC must be present for the encryption/decryption operation (performed by the underlying Cipher). Additionally, we require CKM_PKCS5_PBKD2 for the derivation operation, and CKM_SHA_1_HMAC as way to anticipate the CKP_PKCS5_PBKD2_HMAC_SHA1 availability:

putKeyInfo(new AESPBEKeyInfo("PBEWithHmacSHA1AndAES_128",
CKP_PKCS5_PBKD2_HMAC_SHA1, 128));

A totally explicit entry would be:

Java Algorithm PKCS#11 Mechanisms
Cipher.PBEWithHmacSHA1AndAES_128 (CKM_AES_CBC_PAD or CKM_AES_CBC) and CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC

But we decided to follow the comma ⟹ "or" convention, previously established in the table, so we added the additionally required mechanisms as a parenthesised note:

Java Algorithm PKCS#11 Mechanisms
Cipher.PBEWithHmacSHA1AndAES_128 CKM_AES_CBC_PAD, CKM_AES_CBC (CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC required in any case)

Mac.HmacPBESHA1

d(MAC, "HmacPBESHA1", P11Mac, m(CKM_SHA_1_HMAC),
m(CKM_PBA_SHA1_WITH_SHA1_HMAC));

Here, CKM_SHA_1_HMAC must be present for the MAC generation/verification operation (performed by letting the derived key continue to the Mac implementation). Additionally, we require CKM_PBA_SHA1_WITH_SHA1_HMAC for the derivation operation:

putKeyInfo(new P12MacPBEKeyInfo("HmacPBESHA1",
CKM_PBA_SHA1_WITH_SHA1_HMAC, 160));

A totally explicit entry would be:

Java Algorithm PKCS#11 Mechanisms
Mac.HmacPBESHA1 CKM_SHA_1_HMAC and CKM_PBA_SHA1_WITH_SHA1_HMAC

This is what we wrote (in the inverse order).

SecretKeyFactory.PBEWithHmacSHA1AndAES_128

d(SKF, "PBEWithHmacSHA1AndAES_128",
P11SecretKeyFactory, m(CKM_PKCS5_PBKD2), m(CKM_SHA_1_HMAC));

Since the SecretKeyFactory service just derives, we require CKM_PKCS5_PBKD2 for the derivation operation, and CKM_SHA_1_HMAC as way to anticipate the CKP_PKCS5_PBKD2_HMAC_SHA1 availability.

A totally explicit entry would be:

Java Algorithm PKCS#11 Mechanisms
SecretKeyFactory.PBEWithHmacSHA1AndAES_128 CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC

This is what we wrote.


Proposal

We should either follow the convention as in Cipher or explain what "X and Y" means. Rest looks good to me.

We are more or less following the same convention as in Cipher, using comma instead of "and" for Mac.HmacPBESHA1 and SecretKeyFactory.PBEWithHmacSHA1AndAES_128 would wrongly imply that any of the mechanisms is enough.

With the above explanations, do you still think that this table needs more clarification? Would it improve if we add a trailing "required in any case" at the end, just like the parenthesised notes? Something like:

Java Algorithm PKCS#11 Mechanisms
[…] […]
Cipher.PBEWithHmacSHA1AndAES_128 CKM_AES_CBC_PAD, CKM_AES_CBC (CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC required in any case)
[…] […]
Mac.HmacPBESHA1 CKM_SHA_1_HMAC and CKM_PBA_SHA1_WITH_SHA1_HMAC required in any case
[…] […]
SecretKeyFactory.PBEWithHmacSHA1AndAES_128 CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC required in any case

@valeriepeng
Copy link

As someone who is familiar with the Cipher convention, it's clearer to apply the Cipher convention across the board, i.e. for Mac and SecretKeyFactory too.
For example: For SecretKeyFactory.PBEWithHmacSHA1AndAES_128, use
CKM_PKCS5_PBKD2 (required CKM_SHA_1_HMAC) instead of CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC.

The listed mechanism is the one the impl maps to or based on. The required mechanism is for auxiliary functionalities. Putting the auxiliary one inside the required brackets seems clearer than combining them with the "and" word.

@franferrax
Copy link
Contributor

For example: For SecretKeyFactory.PBEWithHmacSHA1AndAES_128, use CKM_PKCS5_PBKD2 (required CKM_SHA_1_HMAC) instead of CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC.

Ah, ok, now I see. Unfortunately I don't yet have a JBS user, but I'll send the updated text to @martinuy so he can quickly update JDK-8308719 when he is back on Monday.

@franferrax
Copy link
Contributor

@valeriepeng: if the following format looks good to you, I'll pass the update to @martinuy:

Java Algorithm PKCS#11 Mechanisms
Cipher.PBEWithHmacSHA1AndAES_128 CKM_AES_CBC_PAD, CKM_AES_CBC (requires CKM_PKCS5_PBKD2 and CKM_SHA_1_HMAC)
[…] […]
Mac.HmacPBESHA1 CKM_SHA_1_HMAC (requires CKM_PBA_SHA1_WITH_SHA1_HMAC)
[…] […]
SecretKeyFactory.HmacPBESHA1 CKM_PBA_SHA1_WITH_SHA1_HMAC
[…] […]
SecretKeyFactory.PBKDF2WithHmacSHA224 CKM_PKCS5_PBKD2 (requires CKM_SHA224_HMAC)

@valeriepeng
Copy link

valeriepeng commented Jun 2, 2023

@franferrax Yes, sounds good to me.
@martinuy I have updated the CSR based on the agreement and added myself as reviewer.


public final class PBAMac extends PKCS11Test {
private static final char[] password = "123456".toCharArray();
private static final byte[] salt = "abcdefgh".getBytes();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.