Validate XDH native secret derivation

[taoliult]

Add return-code checks for the native XDH secret derivation path, including ICC_EVP_PKEY_derive_init, ICC_EVP_PKEY_derive_set_peer, and the ICC_EVP_PKEY_derive call used to query the secret length.

Also validate the derived secret length against the allocated Java byte array length before returning the result.

[jasonkatonica]

Is there a way to check if the derived key is larger then the buffer before we derive the key? Seems like if we get this exception then its too late and an overlay has already occurred since we used too small of a buffer.

[taoliult]

For the path where secretBufferSize <= 0, we first query the required secret size by calling:

ICC_EVP_PKEY_derive(ockCtx, gen_ctx, NULL, &secret_key_len); /* Get secret key size */

Then we allocate the Java byte array based on the ICC returned size. In that case, the buffer should already match the required derived secret size before the actual derive call.

The possible risk is the secretBufferSize > 0 path, where the caller provides the buffer size directly and we trust that value. If the provided size is smaller than the actual derived secret size, then checking secret_key_len > allocated_len after the derive call could be too late.

To avoid that, I think we can always query the required secret size first, even when secretBufferSize is provided, and validate that the provided buffer size is large enough before doing the actual derive.

Does this approach sound good?

[johnpeck-us-ibm]

Have you tried this to see if ICC has an internal protection against the over write?

[jasonkatonica]

Agreed bruce that perhaps its safer to always request the secret size from ICC instead of allocating our own. This seems safer although this might be at the cost of performance in that we need to make an additional call more often to ICC to calculate such a key size.

I also agree with John that if ICC is already doing this for us to ensure there is not over write then we should be ok. You could also test this bruce by setting the buffer to a small size and seeing what ICC does when we derive the key in too small of a buffer.

[taoliult]

I did a test as shown below:

In XDHKeyAgreement.java, I manually changed SECRET_BUFFER_SIZE_X25519 from 32 to 30 to reduce the secret buffer size for X25519.

I also updated ECKey.c by commenting out the secret buffer size check that I added earlier and adding the debug output shown below:

    fprintf(stderr, "DEBUG: before size check, secretBufferSize = %d, required_len = %zu\n",
        secretBufferSize, required_len);
    fflush(stderr);

    // if ((secretBufferSize > 0) && ((size_t)secretBufferSize < required_len)) {
    //     throwOCKException(env, 0, "Provided secret buffer size is smaller than required size");
    //     goto cleanup;
    // }

I ran TestXDH and got the following output:

DEBUG: before size check, secretBufferSize = 30, required_len = 32

************************** Starting runNonCanonicalTest ************************
Test curve = X25519
Encoded private: 302e020100300506032b656e0422042077076d0a7318a57d3c16c17251b26645df4c2f87ebc0992ab177fba51db92c2a
Encoded public: 302a300506032b656e032100de9edb7d7b7dc1b4d35b61c2ece435373f8343c85b78674dadfc7e146f882b0f
[INFO] 
[INFO] Standard error
[INFO] [*] testXDH_runKAT
[INFO] 
[INFO] Stack trace
[INFO] java.lang.RuntimeException: fail: 
expected=4a5d9d5ba4ce2de1728e3bf480350f25e07e21c947d19e3376f09b3c1e161742, actual=4a5d9d5ba4ce2de1728e3bf480350f25e07e21c947d19e3376f09b3c1e16
	at openjceplus/ibm.jceplus.junit.base.BaseTestXDH.runDiffieHellmanTest(BaseTestXDH.java:561)
	at openjceplus/ibm.jceplus.junit.base.BaseTestXDH.runKAT(BaseTestXDH.java:433)
	at openjceplus/ibm.jceplus.junit.base.BaseTestXDH.testXDH_runKAT(BaseTestXDH.java:80)
	at java.base/java.lang.reflect.Method.invoke(Method.java:575)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Also, changed SECRET_BUFFER_SIZE_X25519 from 32 to 34 to increase the secret buffer size for X25519.

DEBUG: before size check, secretBufferSize = 34, required_len = 32

************************** Starting runNonCanonicalTest ************************
Test curve = X25519
Encoded private: 302e020100300506032b656e0422042077076d0a7318a57d3c16c17251b26645df4c2f87ebc0992ab177fba51db92c2a
Encoded public: 302a300506032b656e032100de9edb7d7b7dc1b4d35b61c2ece435373f8343c85b78674dadfc7e146f882b0f
[INFO] 
[INFO] Standard error
[INFO] [*] testXDH_runKAT
[INFO] 
[INFO] Stack trace
[INFO] java.lang.RuntimeException: fail: 
expected=4a5d9d5ba4ce2de1728e3bf480350f25e07e21c947d19e3376f09b3c1e161742, actual=4a5d9d5ba4ce2de1728e3bf480350f25e07e21c947d19e3376f09b3c1e1617420000
	at openjceplus/ibm.jceplus.junit.base.BaseTestXDH.runDiffieHellmanTest(BaseTestXDH.java:561)
	at openjceplus/ibm.jceplus.junit.base.BaseTestXDH.runKAT(BaseTestXDH.java:433)
	at openjceplus/ibm.jceplus.junit.base.BaseTestXDH.testXDH_runKAT(BaseTestXDH.java:80)
	at java.base/java.lang.reflect.Method.invoke(Method.java:575)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

When ICC derives the key using:

rc = ICC_EVP_PKEY_derive(ockCtx, gen_ctx, secretBytesNative, &secret_key_len);

The output size is based on the secret_key_len value we provide. ICC does not appear to check whether secret_key_len or the size of secretBytesNative is larger or smaller than the required size.

If calling the native code to get the secret key size every time affects performance, then we can keep the previous design: only retrieve the secret key size when secretBufferSize is not provided, and trust the secretBufferSize value from the caller, since the caller is also the OpenJCEPlus code.

[jasonkatonica]

I would be ok trying the additional safer call to get the size in this case. Can we capture some performance data before and after this change to see if this would even be noticeable?

[taoliult]

I triggered two performance runs using ibm.jceplus.jmh.XDHKeyExchangeBenchmark.
No performance difference was detected between the main branch, which does not include this change, and the branch with this "always call ICC get size" code changes.

Then, I will update the code to remove secretBufferSize and always use required_len.

[KostasTsiounis]

If we are calculating the size and creating the byte array ourselves, why not eliminate the secretBufferSize parameter altogether?

It seems like we calculate it in our code anyway here.

[taoliult]

You mean, since the native code already knows the correct required size by calling "rc = ICC_EVP_PKEY_derive(ockCtx, gen_ctx, NULL, &required_len);", why should the caller pass a secretBufferSize at all? Can we remove secretBufferSize and always use required_len?

[KostasTsiounis]

Exactly. But that also depends on the conversation above on whether we want to always call ICC one more time to get the length instead of passing it down. @jasonkatonica what do you think?

[taoliult]

@KostasTsiounis @jasonkatonica

After double-checking the code, I think we don’t need this extra check:

if ((secretBufferSize > 0) && ((size_t)secretBufferSize < required_len)) {
    throwOCKException(env, 0,
            "Provided secret buffer size is smaller than required size");
    goto cleanup;
}

We keep the previous design: only retrieve the secret key size when secretBufferSize is not provided, and trust the secretBufferSize value from the caller, since the caller is our OpenJCEPlus code.

[jasonkatonica]

Exactly. But that also depends on the conversation above on whether we want to always call ICC one more time to get the length instead of passing it down. @jasonkatonica what do you think?

I think it would be safer to always call ICC and have them derive the size for us. If we could measure this to determine the performance of such a call then that would even be better since I would doubt it will cost too much more to do this extra API call.

[taoliult]

I triggered two performance runs using ibm.jceplus.jmh.XDHKeyExchangeBenchmark.
No performance difference was detected between the main branch, which does not include this change, and the branch with this "always call ICC get size" code changes.

Then, I will update the code to remove secretBufferSize and always use required_len.