On Mon, 14 Apr 2025 19:01:00 GMT, Francisco Ferrari Bihurriet 
<fferr...@openjdk.org> wrote:

>> For the TlsXXX issue I check the pseudo-mechanism. That works if all 
>> algorithms are known to the map. I'll check how many we have and see what 
>> are the pros/cons of having them in the map. I prefer symmetric key 
>> algorithms to be in the map.
>> 
>> The reason for the check you referred is to block deriving keys such as 
>> HmacSHA256, PBEWithHmacSHA224AndAES_256, etc. which are not the result of 
>> HKDF derivations, but of Mac and PBE derivation.
>
> 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?

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

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

Reply via email to