Support custom hash functions in ECDH API

[Kagami]

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

[peterdettman]

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.

[gmaxwell]

@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. :)

[peterdettman]

@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:

• Change terminology from "hash function" to "KDF"
• Make provision for "shared info" input to the KDF via the callback - and if present, use it in the default implementation.
• I would still like to see an x-only option, since that is a common case (e.g. TLS) with potentially better performance.

[gmaxwell]

I agree on these constructive suggestions. I'm pondering how to best handle x-only via this. It could query the callback and ask "do you need y"? and then it could internally use an x-only implementation if not.

[Kagami]

I'm pondering how to best handle x-only via this

Just add secp256k1_ecdh_xonly function/one more argument to secp256k1_ecdh?

[gmaxwell]

I would not like to expose an xonly ecdh ultimately, thats an internal implementation detail, largely.

[peterdettman]

Since we need to know (x-only output)? right at the start (to avoid slow compressed input -> x/y output case), I would expect the output format to just be specified in an argument to the ecdh method itself.

[sipa]

If you look at the resulting solution here, it looks pretty silly: you're passing in a function that process the output of ECDH, and then directly return that. I do agree that it follows the "safe to use by default" principle, but it does look a bit overly complex.

[apoelstra]

@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.

[fanatid]

Any progress with this issue?
I agree with @apoelstra (secp256k1_unsafe_ecdh_inner + secp256k1_ecdh) and ready for creating PR if you support ecdh function with raw output.

[sipa]

Sorry for the delay, I was working on other things. Will look soon.

[axic]

To summarise some of the current ECDH uses:

• Bitcoin (sha256(v+Px))
• Bitmessage (sha512(Px))
• Ethereum (nisp_sp800_56a_kdf(Px+Py) since it uses uncompressed keys only)
• TLS

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?

[apoelstra]

@axic I really don't think we should explicitly support common schemes. This is not scalable, increases our maintenance burden, and would also require we cryptanalyze those schemes to ensure we aren't explicitly endorsing anything we don't believe to be safe.

[fanatid]

I wrote ecdhUnsafe for JavaScript secp256k1 bindings. Are you merge PR with such changes?

[chfast]

I accidentally implemented b) in #446 with name secp256k1_ecdh_raw.

And I think Ethereum takes only Px.

[sipa]

@fanatid I'd be fine with that.

[apoelstra]

Was this supposed to be closed by #354?

[jonasnick]

That's what it looks like.