magic-wormhole / magic-wormhole-protocols

The documentation of the protocols powering Magic Wormhole
MIT License
26 stars 12 forks source link

Replace Transit cryptography with Noise #25

Closed piegamesde closed 2 years ago

piegamesde commented 2 years ago

At the moment, the Transit channel uses Salsa20 and Poly1305 via the NaCl "Secretbox" abstraction. Dilation, however, contains a Transit clone based on the Noise protocol framework. I've been told its cryptography is superior in some way (can't judge for myself due to lack of knowledge) so we would probably want this for "normal" Transit too.

I think adapting it should not be too difficult. Add a noise-crypto-v1 feature flag, and if both sides support it then replace the initial handshake and later the message encoding with the noise-based implementation.

piegamesde commented 2 years ago

One question that came up is how to deal with the maximum message size. While a maximum of 64 MiB is recommended, the theoretical limit is 4 GiB. Contrasting to this, the noise protocol has a hard-coded maximum message size of mere 64 kiB.

I do not think it is practical to reduce the maximum transit message size to a value this low, especially since it wouldn't provide a drop-in replacement for the current secretbox cryptography anymore. Instead, I suggest that a message longer than 64 kiB is encoded by concatenating multiple noise protocol messages to each other. In detail:

vu3rdd commented 2 years ago

What do the dilation implementation in Python do currently in this regard?

piegamesde commented 2 years ago

@vu3rdd AFAICT it just forwards bytes to the noise implementation, without any further processing: https://github.com/magic-wormhole/magic-wormhole/blob/master/src/wormhole/_dilation/connection.py#L458

So it will probably just throw up or panic on large messages.

piegamesde commented 2 years ago

Btw, I'm thinking about using authenticated encryption for the length fields. The noise protocol already supports it, so implementing it is just a matter of feeding the length into the encryption and decryption process.

For longer messages as per my proposal above, only the first noise message would have the length of the entire transit message as authenticated data.

vu3rdd commented 2 years ago

Perhaps we should not add anything more around the existing transit protocol and concentrate on dilation as it is much superior on all counts. Existing transit would just be a fallback mechanism once dilation is enabled everywhere.

meejah commented 2 years ago

I agree that it doesn't make any sense to develop a second Transit protocol.

There aren't any problems with the existing protocol and the only justification presented here is that "Noise is good". Unless there are pressing security problems with Secretbox or the ciphers it uses, the current Transit protocol shall remain. Introducing a second nearly-identical protocol just makes more work for all implementations.

Any issues Dilation may have should be filed as separate issues or taken up in appropriate existing tickets.