On Tue, 17 Mar 2026 22:32:38 GMT, Valerie Peng <[email protected]> wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/Argon2Impl.java line 64:
>>
>>> 62: MEMORY_MAX = 1 << 21; // 2 GiB?
>>> 63: } else {
>>> 64: MEMORY_MAX = memLimit >>> 12; // 1/4 of maximum jvm memory
>>
>> - Not sure about 2GiB. What if it's a small server/VM/Docker running on 2GiB
>> of total RAM? I think we should either have a configurable security property
>> or use the most conservative value of 64 MiB.
>> - I think we should document better this code block in comments: why we
>> compare to `Long.MAX_VALUE`? Why we shift by 21 and 12?
>> - IMO, the code would be more readable if we rename `MEMORY_MAX` to
>> `MAX_MEM_BLOCKS` and add a comment that 1 block = 1 KiB.
>> - What about the limit for (t * m) value?
>
> Note that this is the "max" not the default. Thus, you should not use a
> conservative value such as 64 MiB.
> Right shift 10 equals `/ 2^10`. I can add some comment for this and the
> `Long.MAX_VALUE`.
> As for the meaning of `MEMORY_MAX`, it is used to check the `memory` value.
> Note that the actual number of memory blocks can be different from `memory`
> when its value is not multiples of `4*p`. So, `MAX_MEM_BLOCKS` can be
> incorrect at times.
> Not too sure about `the limit for (t * m) value`, are you suggesting
> specifying a limit for this combination instead of `t` and `m` separately? Or
> Do you mean to use a limit for this combination instead of limiting `t`?
- Right, `max` is what I meant actually... but as I've mentioned, having this
value in a security property would be the flexible solution.
- `MAX_MEM_BLOCKS` would mean the max number of 1 KiB blocks that can be used,
not the actual number of blocks we are using.
- The limit for `t * m` would be in addition to existing limits, limiting just
`t` by itself doesn't make much sense.
Overall, I think we should have security properties for all those limits.
>> src/java.base/share/classes/com/sun/crypto/provider/Argon2Impl.java line 414:
>>
>>> 412: Runtime r = Runtime.getRuntime();
>>> 413: // memory limit = 1/4 of maximum amount of the jvm memory limit
>>> 414: MEMORY_MAX = r.maxMemory() >>> 12;
>>
>> Did you mean `>>> 2`? Also:
>>
>> - `r.maxMemory()` may return `java.lang.Long#MAX_VALUE` in some
>> environments, we should check for that and use something else in such case.
>> - Since we check for memory and parallelism, should we set some reasonable
>> limit for iterations?
>
> `r.maxMemory(`) is in bytes, so the extra 10 is to convert that to KiB. So
> combined this w/ 2 (for the 1/4) => 12. Will think about the `Long.MAX_VALUE`
> case.
> I thought about limiting iterations, but not sure what would be a reasonable
> limit, so I have not added it. Any suggestion?
Oh, I see, we should probably comment it so it's clear. About limiting
iterations: since iterations and memory are connected, I would limit the
product of them (t * m) to something like 32-64 GiB? Should we make it a
security property?
>> src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 463:
>>
>>> 461: ps("KDF", "HKDF-SHA512",
>>> 462:
>>> "com.sun.crypto.provider.HKDFKeyDerivation$HKDFSHA512");
>>> 463: ps("KDF", "Argon2id", "com.sun.crypto.provider.Argon2ID");
>>
>> This should be `Argon2`, right? And then we specify the exact variant with
>> `Argon2ParameterSpec` when calling `deriveData`?
>
> No, it's `Argon2id` as this is the only supported flavor in the Argon2 family
> algorithms per the feedback that I received.
I see, thanks!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2949976704
PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2933519727
PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2784108836
