libp2p / specs

Technical specifications for the libp2p networking stack
https://libp2p.io
1.56k stars 273 forks source link

noise-libp2p: introduce "handshake seal"; more. #234

Closed raulk closed 4 years ago

raulk commented 4 years ago

This PR supersedes #210, and implements the outcome of that discussion.

The current construction is vulnerable to replay attacks by a MITM agent that records previous handshakes, and transplants old user data from those handshakes into new ones, as long as the static key hasn't changed.

We fix this vulnerability by leveraging Noise channel binding, in order to "seal" the handshake and assert that the exchanged data was witnessed equally by both parties.

This commit also simplifies protobuf field naming.

Finally, we formalise in which Noise messages of IK and XX the message payload is to be shared, to guarantee secrecy, integrity and authentication.

ping: @shahankhatch @Mikerah @djrtwo @dryajov @mpetrunic @morrigan @araskachoi -- please review too (can't assign you as a reviewer as you're not part of the libp2p org yet).

raulk commented 4 years ago

@yusefnapora awesome! Let's wait for a few more approvals. @burges -- would appreciate thoughts, if any.

raulk commented 4 years ago

🚃🚃🚃🚃🚃 Merge train leaving tomorrow morning. Please get your remarks in today!

romanb commented 4 years ago

Could someone elaborate (e.g. with concrete examples) on

The current construction is vulnerable to replay attacks by a MITM agent that records previous handshakes, and transplants old user data from those handshakes into new ones, as long as the static key hasn't changed.

as well as

[..] In the above scheme, the data field is not guarded against tampering. Consequently, a MITM agent could alter the data in transit, or even record the handshake to replay it at a later time with mutated or transplanted data.

Given that all "early data" is sent as part of encrypted handshake payloads using an AEAD scheme and the current handshake hash h as associated data, how is such transplantation or mutation performed exactly?

I'm probably missing the obvious, but I currently don't see what the signing of the handshake hash is supposed to accomplish on top of signing the static DH public key as far as authentication is concerned. I mean, I can understand why one would rather want to sign the handshake hash instead of the static DH public key (e.g. to prevent signature replays when a static DH private key is compromised, allowing impersonating the owner of the private identity key) but I fail to see why you would want to do both. If someone could enlighten me with more details on the attack vectors addressed here and why both, signing the static DH key as well as the handshake hash is necessary, I would be very grateful.

raulk commented 4 years ago

@romanb I think you're right.

I missed the fact that EncryptAndHash and DecryptAndHash also call MixHash with the ciphertexts, and that WriteMessage(payload) and ReadMessage(into_payload_buffer) call EncryptAndHash and DecryptAndHash respectively.

For some reason, I thought only the keys were mixed in, but I was wrong.

Consequently, the AD (authentication data) already includes the payload ciphertexts we've witnessed, and by introducing this handshake seal we are not proposing any extra guarantee that is not already awarded by the AEAD scheme that encompasses the payloads.

If an intermediary constructs a transplant or tamper attack, I'm confident it'll be caught by an authentication tag mismatch.

I'm going to retract this RFC, and open a new PR with the other changes.