On Fri, 3 Feb 2023 01:41:41 GMT, Martin Balao <mba...@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. 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 diffe rent 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/P11Mac.java line 204: > 202: if (svcPbeKi != null) { > 203: if (key instanceof P11Key) { > 204: params = PBEUtil.checkKeyParams(key, params, algorithm); 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. ------------- PR: https://git.openjdk.org/jdk/pull/12396
