On Wed, 11 Sep 2024 22:49:12 GMT, Kevin Driver <kdri...@openjdk.org> wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
>> line 143:
>>
>>> 141: throws InvalidAlgorithmParameterException {
>>> 142: List<SecretKey> ikms, salts;
>>> 143: byte[] inputKeyMaterial, salt, pseudoRandomKey, info;
>>
>> It's always nice to clean up these intermediate bytes related to secret keys.
>
> @wangweij: Addressed in
> https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
We usually clean up the array in a `finally` block if an exception could be
thrown in between.
>> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
>> line 274:
>>
>>> 272: }
>>> 273: } else if(keys != null) {
>>> 274: return null;
>>
>> Isn't an empty array better here? The null return is unexpected and has to
>> be handled specially.
>
> @wangweij: Resolved in
> https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
Thanks. And you don't need to check `if (salt == null` and `if
(inputKeyMaterial == null)` in `hkdfExtract` now. Also, change the javadoc for
`hkdfExtract` about nulls.
>> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
>> line 299:
>>
>>> 297: throws InvalidKeyException, NoSuchAlgorithmException {
>>> 298:
>>> 299: if (salt == null) {
>>
>> Also cover the empty `salt` case here. The `SecretKeySpec` creation below
>> would fail.
>>
>> Hint: when people call `addSalt(k)`, `k` can be empty. It doesn't have to be
>> a `SecretKeySpec`. This is worth a test.
>
> @wangweij: Addressed in
> https://github.com/openjdk/jdk/pull/20301/commits/82791ac01fb7f597a7e814403261c7a50e8a08df.
I understand `addSalt(new byte[0])` is not a problem, but what about
addSalt(new SecretKey() {
public byte[] getEncoded() { return new byte[0]; }
....
};
Anyway, with the new length check, this will be detected.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1756008449
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1756012745
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1756011616
