RFR: 8350661: PKCS11 HKDF throws ProviderException when requesting a 31-byte AES key [v2]

[Valerie Peng]

On Mon, 14 Apr 2025 19:01:45 GMT, Francisco Ferrari Bihurriet <fferrari at openjdk.org> wrote:

>> As far as I understand it, `HmacSHA256` is blocked, but not `PBEWithHmacSHA224AndAES_256`.
>> 
>> ### `HmacSHA256`
>> 
>> * Has an `HMACKeyInfo` entry with the following non-static fields:
>>     * `KeyInfo.algo` = `"HmacSHA256"`
>>     * `KeyInfo.keyType` = `CKK_GENERIC_SECRET`
>>     * `KeyInfo.keyGenMech` = `CK_UNAVAILABLE_INFORMATION`
>>     * `HMACKeyInfo.mech` = `CKM_SHA256_HMAC`
>>     * `HMACKeyInfo.keyLen` = `256`
>> 
>> Given `ki.keyType` is `CKK_GENERIC_SECRET` and `alg` is `HmacSHA256`, in `P11HKDF::getDerivedKeyType` it will enter the first `case` but not the `if`. So it will finally throw the expected exception:
>> 
>> 
>> InvalidAlgorithmParameterException("A key of algorithm 'HmacSHA256' is not valid for derivation.")
>> 
>> 
>> ### `PBEWithHmacSHA224AndAES_256`
>> 
>> * Has an `AESPBEKeyInfo` entry with the following non-static fields:
>>     * `KeyInfo.algo` = `"PBEWithHmacSHA224AndAES_256"`
>>     * `KeyInfo.keyType` = `CKK_AES`
>>     * `KeyInfo.keyGenMech` = `CK_UNAVAILABLE_INFORMATION`
>>     * `PBEKeyInfo.kdfMech` = `CKM_PKCS5_PBKD2`
>>     * `PBEKeyInfo.prfMech` = `CKP_PKCS5_PBKD2_HMAC_SHA224`
>>     * `PBEKeyInfo.keyLen` = `256`
>>     * `PBEKeyInfo.extraAttrs` = `new CK_ATTRIBUTE[] { CK_ATTRIBUTE.ENCRYPT_TRUE }`
>> 
>> Given `ki.keyType` is `CKK_AES`, in `P11HKDF::getDerivedKeyType` it will enter the first `case` and also the `if`, returning `CKK_AES`. Later, in `P11KeyGenerator::checkKeySize(..., Token token)`, `P11KeyGenerator::getSupportedRange` will return `null`, because `ki.keyGenMech` is `CK_UNAVAILABLE_INFORMATION`. This will make `P11KeyGenerator::checkKeySize(..., CK_MECHANISM_INFO range)` enter the `default` case, and finally return the unmodified `keySize`. No exception is thrown, unless I'm missing something.
>
> Also, we could save one of the `if` conditions by creating a separate `case` for `CKK_GENERIC_SECRET`:
> 
> 
>         switch ((int) ki.keyType) {
>             case (int) CKK_DES, (int) CKK_DES3, (int) CKK_AES, (int) CKK_RC4,
>                     (int) CKK_BLOWFISH, (int) CKK_CHACHA20 -> {
>                 return ki.keyType;
>             }
>             case (int) CKK_GENERIC_SECRET -> {
>                 if (alg.equalsIgnoreCase("Generic")) {
>                     return ki.keyType;
>                 }
>             }
>             // [...]
>         }
> 
> 
> In my view, this is quicker to understand, what do you think?

Yes, I much prefer this idea of separate case for `CKK_GENERIC_SECRET`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24526#discussion_r2047543388