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/P11SecretKeyFactory.java line 138: > 136: if (key == null) { > 137: throw new InvalidKeyException("Key must not be null"); > 138: } Is there a particular reason for removing this code block? ------------- PR: https://git.openjdk.org/jdk/pull/12396
