The secp256k1_ellswift_xdh API has a data arg that is optional for one
of the hash functions (unused) but required for the other.
This adds a simple check to provide a clear error instead of crashing
due to an out-of-bounds memory access when the user mistakenly provides
data=NULL and selects the xdh_hash_function_prefix() function.
Note: Open to improving the arg description in the API as well. I’m just adding this check because a better error would have saved me a few minutes.
The secp256k1_ellswift_xdh API has a `data` arg that is optional
for one of the hash functions (unused) but required for the other.
This adds a simple check to provide a clear error instead of crashing
due to an out-of-bounds memory access when the user mistakenly provides
data=NULL and selects the `xdh_hash_function_prefix()` function.
when the user mistakenly provides data=NULL
Note that VERIFY_CHECKs are only active in the tests.
when the user mistakenly provides data=NULL
Note that VERIFY_CHECKs are only active in the tests.
We can’t use ARG_CHECK because ellswift_xdh_hash_function_prefix() doesn’t receive the context (for now). #1777 will solve that and let us add the proper arg check.
Update: We probably could leave this open until the other PR gets merged.
We can’t use
ARG_CHECKbecauseellswift_xdh_hash_function_prefix()doesn’t receive the context (for now). #1777 will solve that and let us add the proper arg check.
Unfortunately, not exactly. ellswift_xdh_hash_function_prefix() now gets the static context, hardcoded. But we need the real context to call the right illegal_callback.
I think this is an oversight in the API design.
Whenever there’s an argument where the user can provide a hash function (nonce derivation functions and hashing the raw secret in ECDH/Ellswift), we expose a pointer to our built-in functions in the API. These functions don’t get a context object, which makes sense: the corresponding function type doesn’t allow for a context object because user-provided functions aren’t meant to deal with context objects.
But what perhaps doesn’t make sense is that we expose these function pointers at all. The user isn’t really supposed to call these functions directly. If anything, the user is supposed to pass these function pointers back to our API functions. So they should have been rather something like “handles” or enum values representing the built-in functions.
In fact, we always have NULL as a value representing the default built-in, but at least in the ellswift module, there are two built-in functions, one of them being the default. And one of them even makes use of the data pointer, so we can’t even abuse the data pointer to pass the context object…
All of this is a bit unsatisfactory because it means that the runtime SHA256 won’t work for nonce derivation and ECDH hashing.
For this PR, using ARG_CHECK will still be okay, I think. Then we call the default illegal callback, which is still better than dereferencing a NULL pointer.