On Wed, 15 Mar 2023 00:30:56 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> We would like to propose an implementation for the [JDK-8301553: Support >> Password-Based Cryptography in >> SunPKCS11](https://bugs.openjdk.org/browse/JDK-8301553) enhancement >> requirement. >> >> In addition to pursuing the requirement goals and guidelines of >> [JDK-8301553](https://bugs.openjdk.org/browse/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](https://datatracker.ietf.org/doc/html/rfc7292#appendix-B) >> 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](https://datatracker.ietf.org/doc/html/rfc8018#section-6.2) >> 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. I n 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 diff erent 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](https://bugs.openjdk.org/browse/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. > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java > line 108: > >> 106: } >> 107: >> 108: static boolean checkUse(KeyInfo ki, KeyInfo si) { > > 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. 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. ------------- PR: https://git.openjdk.org/jdk/pull/12396
