holepunchto / hypercore-crypto

The crypto primitives used in hypercore, extracted into a separate module
MIT License
35 stars 14 forks source link

crypto.discoveryKey seems to have the arguments in the wrong order #4

Closed gmaclennan closed 4 years ago

gmaclennan commented 4 years ago

sodium.crypto_generichash expects an input buffer of arbitrary length and an optional key. According to sodium docs the input is hashed, and the optional key "can be used to make sure that different applications generate different fingerprints even if they process the same data." and different keys "are very likely to produce distinct fingerprints". This "very likely" seems to suggest that this is not as reliable as a hash.

The sodium-native docs say the input is the second arg, and key is the third arg, however discoveryKey seems to call the arguments in the wrong order:

exports.discoveryKey = function (tree) {
  var digest = bufferAlloc(32)
  sodium.crypto_generichash(digest, HYPERCORE, tree)
  return digest
}

As far as I understand, HYPERCORE should be the key. I understand from #3 that tree is actually the publicKey. The consequence of this (I think, although I don't know the internals of sodium) is that the hash is not as guaranteed against collisions. It may be that this doesn't matter and changing the key is being hashed with the same algorithm as the input, but it doesn't seem like this was the intended way for this to work.

mafintosh commented 4 years ago

I think the sodium documentation might just be a bit confusing here with the term "very likely".

Having implemented blake2b myself in wasm, the only thing the key argument does in practice is set a flag in the hash state and then call hash.update with the key payload

Ie. hash(key, buffer) ~== hash(concat(key, buffer) + A FLAG) See https://github.com/mafintosh/blake2b-wasm/blob/master/index.js#L62

We set the public key as the key argument as that's the argument with the most entropi meaning an attacker has to mine more to try and break it.

gmaclennan commented 4 years ago

ok got it, so since the hash is just pretty much concat(key, buffer) it doesn't really make a difference which is which, apart from key being limited to crypto_generichash_KEYBYTES_MAX whilst buffer can be arbitrary length.

mafintosh commented 4 years ago

Yep, I just asked the sodium maintainer on Twitter about the docs phrasing. Will update here if/when he replies.

mafintosh commented 4 years ago

Frank confirmed https://twitter.com/mafintosh/status/1194621010999332865