On Thu, 25 May 2023 23:44:44 GMT, Martin Balao <mba...@openjdk.org> wrote:
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PBECipher.java
>> line 214:
>>
>>> 212: protected int engineGetKeySize(Key key)
>>> 213: throws InvalidKeyException {
>>> 214: return cipher.engineGetKeySize(key);
>>
>> I am not too sure that this would work for all types of key... Based on the
>> current impl, cipher is an AES cipher, but the specified key here can be
>> P11Key or a PBEKey object. The key algorithms for both would be PBE-related,
>> but then you are using a AES cipher to retrieve the key size?
>
> If the key is a P11PBEKey, the underlying Cipher will call
> P11SecretKeyFactory::convertKey with a non-PBE service algorithm and,
> assuming that it's in the same token and that the key is suitable for the
> service, will return the same key. This case should return the key length
> used during derivation. If the key is a PBEKey but not a P11PBEKey, there
> will be a call to P11SecretKeyFactory::convertKey with a non-PBE service
> algorithm and the key algorithm is PBE. Looks to me that, assuming that the
> key algorithm is suitable for the service, there will be a key derivation to
> finally get the length from a derived P11PBEKey. This case should not be
> different than using a PBEKey in a non-PBE service. @franferrax what do you
> think?
We discussed this further with @franferrax and changed the approach.
While the previous approach was working for cases such as P11PBEKey and other
javax.crypto.interfaces.PBEKey keys whose information was the same as used for
derivation in engineInit later, we found that some cases were not working
because engineGetKeySize does not have access to parameters. In particular,
com.sun.crypto.provider.PBEKey keys need parameter information for a proper
derivation and there could also be javax.crypto.interfaces.PBEKey keys whose
information is different than passed later by parameters.
In addition to the aforementioned problems, deriving twice (in many cases) was
not the best for efficiency.
There is a strong guarantee that when PBE Cipher service initialization is
successful, the key size is the same as the service key size (obtained from the
P11SecretKeyFactory::keyInfo map, passing the service algorithm for the
lookup). This is because all keys that can successfully land on P11 PBE Cipher
service are either 1) derived keys of the same algorithm, or 2) keys to be
derived in which the derived key length is taken from the service key length.
As part of this change, you will see in our next commit a code fix to hold the
invariant "derived PBE Cipher keys that reach a PBE Cipher service must have
the same algorithm". We missed this check before because such cases are not
calling P11SecretKeyFactory::convertKey —which implements the checks—, and were
forwarding the key to the underlying cipher service. Notice that the underlying
cipher service does not apply the same enforcement and that the underlying
cipher is AES, so it will accept both 128-bit and 256-bit AES keys. Thus, the
following key would have mistakenly succeeded: a PBEWithHmacSHA512AndAES_256
key passed to a PBEWithHmacSHA512AndAES_128 PBE Cipher service. We don't want
to call P11SecretKeyFactory::convertKey for the case described in P11 PBE
Cipher for efficiency, we just need to check the algorithms.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1207418757
