fix(dht)!: fixes MAC related key vuln for propagated cleartext msgs #3907
Conversation
sdbondi
force-pushed
the
dht-origin-mac
branch
2 times, most recently
from
97add0f to
f773132
Compare
sdbondi
force-pushed
the
dht-origin-mac
branch
2 times, most recently
from
792d5ec to
0a2b9d7
Compare
sdbondi
force-pushed
the
dht-origin-mac
branch
3 times, most recently
from
049cdff to
55008eb
Compare
| pub fn from_key<K: ByteArray>(key: &K) -> Self { | ||
| let bytes = key.as_bytes(); | ||
| let mut buf = [0u8; NodeId::byte_size()]; | ||
| VarBlake2b::new(NodeId::byte_size()) | ||
| Blake2bVar::new(NodeId::byte_size()) | ||
| .expect("NodeId::byte_size() is invalid") | ||
| .chain(bytes) | ||
| .finalize_variable(|hash| { | ||
| // Safety: output size and buf size are equal | ||
| buf.copy_from_slice(hash) | ||
| }); | ||
| // Safety: output size and buf size are equal | ||
| .finalize_variable(&mut buf).unwrap(); | ||
| NodeId(buf) | ||
| } | ||
|
|
There was a problem hiding this comment.
This hash invocation should use domain separation to avoid unexpected collisions elsewhere. This is supported directly using Blake2 personalization.
| mac_challenge.update(&protocol_version.to_bytes()); | ||
| mac_challenge.update(destination.as_inner_bytes()); | ||
| mac_challenge.update(&protocol_version.as_bytes()); | ||
| mac_challenge.update(destination.to_inner_bytes()); |
There was a problem hiding this comment.
Is this guaranteed to have a fixed-length byte representation?
There was a problem hiding this comment.
Looks like this has a fixed-length representation, and types are differentiated using a flag.
There was a problem hiding this comment.
It is possible, if desired, to use variable-length representations. In this case, the length (as a fixed-length representation!) should be prepended after the (fixed-length) flag.
There was a problem hiding this comment.
That's right, each variant of NodeDestination has a single byte representation prepended to the encoding. Gotcha, both the NodeId and CommsPublicKey have fixed-length representations so I don't think we'll have a need for variable-length encodings here.
| let signer_public_key = CommsPublicKey::from_secret_key(&signer_secret_key); | ||
| let challenge = construct_origin_mac_hash(&signer_public_key, &nonce_pk, message); | ||
| let signature = Signature::sign(signer_secret_key, nonce_s, &challenge) | ||
| .expect("challenge is [u8;32] but SchnorrSignature::sign failed"); |
There was a problem hiding this comment.
I suppose this is basically infallible.
There was a problem hiding this comment.
It's 100% infallible with a 32-byte challenge and Ristretto - we should probably make an infallible version of sign that takes a [u8;32]
Description
e = H(P||R||H(m))Motivation and Context
This only affects signed cleartext messages that are propagated through adversarial nodes.
Encrypted propagated messages are unaffected because only the recipient can decrypt the MAC and so cannot forge a signature.
Nonce malleability
Related key
All signed cleartext and encrypted DHT messages are incompatible with current network:
Namely,
How Has This Been Tested?
New unit tests that verify OriginMac is secure against these vulnerabilities
Tested base node manually, and normal cleartext block and tx propagation messages continue to work
Tested wallet, direct send transactions are still compatible