Current ECDH implementation always hashes both Px and Py of point multiplication result with SHA256. It makes impossible to use secp256k1 with existing protocols like Bitmessage which uses SHA512(Px) as shared secret. As mentioned here, possible solution for this is to accept custom hash function in API similar to custom nonce function.
cc @apoelstra
It seems to me that a major reason for handling the hash internally was to avoid “revealing” the raw point coordinate(s). Supporting a (callback-based) custom hash function lets them “escape” again (even if the callback is isolated somehow, it could still calculate a no-op of the input).
So it seems to me that internal hashing with a customisable hash function is a needless complication. Of course, if you’re restricted to SHA256, you can’t use this even for what should be an obvious target - TLS key exchange.
I propose we abandon the internal hashing approach altogether.
@peterdettman We are not trying to avoid revealing anything, but rather not provide interfaces that by that we know will primarily be used in less safe ways because using it that way is the simplest thing possible.
[Because of things like http://lists.linuxfoundation.org/pipermail/bitcoin-dev/2014-March/004720.html (see also later posts where I gave some more attacks) I wish we didn’t have a reason to expose ECDH at all, in favor of safe higher level constructs– e.g. ECIES instead of “here is ecdh then you can makeup your own crypto scheme”, because we should assume that the caller is not a subject matter expert in cryptography whos work is being reviewed by other subject matter experts– but thats not always the best trade-off.. and sometimes we have to expose more to get functionality. But the interface can still encourage more prudent behavior.]
An example of this is how we handle DSA nonces. By default the library uses RFC6979 internally. But if you need to do something special, you can send in a callback that implements whatever you need– upto and including “emits a constant nonce”. :) So the caller can do whatever they like, but if they do something dangerous or inadvisable at least it wasn’t because it was the easiest thing to do (or worse, the only obvious thing to do).
for this PR, I think it’s a good approach; we’ll probably want to change the api; I’ve just been too slammed this week to give it a proper review yet. :)
@gmaxwell OK, that’s reasonable, although I think the argument is a bit weaker than for the ECDSA/RFC6979 case.
I will observe that I think the “no-op escape hatch” will be the method of choice for a TLS implementation. There are several different key exchange methods each producing a premaster secret after their own fashion, then a distinct master secret generation via the TLS PRF. I would expect most implementations to have a module boundary between those two things - even if an implementation only supports ECDH(E), but uses other libraries for other curves.
Switching to constructive suggestions mode:
I’m pondering how to best handle x-only via this
Just add secp256k1_ecdh_xonly function/one more argument to secp256k1_ecdh?
@sipa I wonder if we should have a function secp256k1_unsafe_ecdh_inner function that returns the raw output, and have the normal ECDH function be a thin wrapper around that that does the hash?
That is, replace the function pointer with a scary name.
secp256k1_unsafe_ecdh_inner + secp256k1_ecdh) and ready for creating PR if you support ecdh function with raw output.
To summarise some of the current ECDH uses:
sha256(v+Px))sha512(Px))nisp_sp800_56a_kdf(Px+Py) since it uses uncompressed keys only)There are perhaps other schemes.
The two proposed solutions are:
a) secp256k1_edch to accept a hash callback function
b) to have the non-hashing inner function exported as secp256k1_unsafe_ecdh_inner
The problem with a) is that the callback could just return the unmodified vanilla input. Though secp256k1_ecdh could check and reject if the input equals the output, that wouldn’t stop the case when a slightly modified, but still unsafe data is returned.
Another option, c) was so far not mentioned: to include the hashing for the common schemes (the four stated above). This has the drawback that it will take effort and time to implement all the new schemes which are deemed useful, increases code size and possibly has bigger maintenance burden. The positive aspect is that raw keys are not leaked. I am not convinced this single positive aspect justifies this option. @sipa did you had a chance to look at the options and perhaps @fanatid’s PR #354?
