10 ms·

Whisper: Wraps any Go io.ReadWriter in a secure tunnel using Ed25519/X25519

125 points by bubblehack3r 4y ago

tptacek 4y ago
See also: https://github.com/flynn/noise https://github.com/flynn/noise
nemo1618 4y ago
Interesting, though AFAIK a secure tunnel is only useful for a net.Conn, not an io.ReadWriter. What's the usecase for a "secure tunnel" over a bytes.Buffer? btw, I noticed that the decrypt function reads a 32-bit message length and immediately allocates a slice of that size. That means an attacker can send 0xFFFFFFFF and cause you to allocate 4GiB.
- ikiris 4y ago
  Yeah this code review isn’t gonna go great.
- jerf 4y ago
  You're kinda looking at the interface issue backwards. Being an io.ReadWriter is a promise that the tunnel code won't do anything other than read or write. The tunnel code shouldn't be getting local and remote addresses or setting deadlines of its own, or most of the rest of what net.Conn can do. https://pkg.go.dev/net#Conn https://pkg.go.dev/net#Conn It is good that it doesn't take more than it needs. That bytes.Buffer happens to implement io.ReadWriter is not of consequence; for any given task you want a io.Reader or io.Writer for there may be any number of specific implementations that don't make sense to use with it, but that doesn't mean you should require more methods of the interface. I'm speaking solely about the interface here; the many other concerns are valid.
  - nemo1618 4y ago
    Sure, in general you want to use the narrowest possible interface. But in this particular case, I think `net.Conn` communicates intent better than `io.ReadWriter` -- the former implies that two distinct parties are involved, whereas `io.ReadWriter` is usually for (single-user) buffers or files. Sometimes it's sensible to use a larger interface than necessary; e.g. if you have a helper function that only calls `SetDeadline` and `Read`, the argument should be a `net.Conn`, not a custom interface with just those methods. tbh though, it's a very minor quibble and not really worth worrying about ‾\_(ツ)_/‾
    - jrockway 4y ago
      I disagree with this. If the only surface area of the underlying transport that the implementation uses is Read() and Write(), then it should be an io.Reader and an io.Writer. If it wants to mess around with read/write deadlines, then you'd have to bring in net.Conn, but it doesn't, so don't. If you're worried about someone using bytes.Buffer as their transport layer, net.Conn doesn't fix that; there is net.Pipe. (It's not buffered, though.)
FiloSottile 4y ago
There is no description of the protocol or of its security goals, so I am making some guesses based on a cursory look at the source and what I imagine this might be for. A single symmetric key is derived for both directions, and there is no checking of nonces, so as far as I can tell any message can be dropped, reordered, or replayed in both directions. (Including replaying message from A to B as if they were from B to A.) This is a bit like using ECB and likely to lead to fun application-specific attacks like [0]. This is very much rolling your own crypto, in a dangerous way. I am on the record as being "against" the "don't roll your own crypto" refrain [1], but mostly because it doesn't work: it should discourage people from publishing hand-rolled protocols such as this, but instead people think it means "don't roll your own primitives" and accept any use of "Ed25519/X25519" as probably secure. Please read about the Noise framework [2] to get an idea of how much nuance there is to this, and consider using a Go implementation of it [3] instead. P.S. This kind of issue is also why I maintain that NaCl is not a high-level scheme [4]: this could have used NaCl and have the exact same issues. libsodium has a couple slightly higher-level APIs that could have helped, secretstream [5] and kx [6], but again please use Noise. [0] https://cryptopals.com/sets/2/challenges/13 https://cryptopals.com/sets/2/challenges/13 [1] https://securitycryptographywhatever.buzzsprout.com/1822302/8953842-the-great-roll-your-own-crypto-debate-with-filippo-valsorda https://securitycryptographywhatever.buzzsprout.com/1822302/... [2] https://noiseprotocol.org/noise.html https://noiseprotocol.org/noise.html [3] https://github.com/flynn/noise https://github.com/flynn/noise [4] https://words.filippo.io/dispatches/nacl-api/ https://words.filippo.io/dispatches/nacl-api/ [5] https://libsodium.gitbook.io/doc/secret-key_cryptography/secretstream https://libsodium.gitbook.io/doc/secret-key_cryptography/sec... [6] https://libsodium.gitbook.io/doc/key_exchange https://libsodium.gitbook.io/doc/key_exchange
- c7DJTLrn 4y ago
  I once heard something along the lines of "if you're writing the words RSA, you're doing it wrong." I guess that goes for ED25519 also. If you want to secure a stream with asymmetric cryptography and don't need all the bells and whistles of TLS, what's the correct way to do it? Noise? Is there nothing simpler?
  - 4y ago
jrockway 4y ago
There has been a lot of discussion about how the crypto doesn't work. I have some quibbles about the Go. Don't put a mutex in your io.Reader. It is assumed that, unless mentioned in the documentation, it is not safe to use anything in Go from multiple goroutines. If someone wants to make the reader synchronous for use among multiple goroutines, they can do so themselves. But it's so rare that it's unlikely anyone would want this. read/write mutexes typically perform worse than a plain mutex. You pay a performance cost to prevent readers and writers from starving each other; if you don't care (and this code doesn't), use a Mutex. https://zephyrtronium.github.io/articles/rwmutex.html https://zephyrtronium.github.io/articles/rwmutex.html Finally, it's fine to embed the mutex value directly in your struct if your methods take pointer receivers. &MyThing{Mutex: new(sync.Mutex)} is pretty weird to see. If the mutex were included as its value, then the zero value of MyThing is ready to use; &MyThing{} has a new mutex in it. (You can't copy the value of &MyThing either way; a correct copy requires holding the mutex. The reason to embed a *sync.Mutex instead of a sync.Mutex is so that you can copy the containing type, but that is actually unsafe. You grab a pointer to the wrong Mutex in your copy, and you also copy data protected by the mutex without holding it. So your methods MUST have a pointer receiver either way, and thus the indirection to *sync.Mutex is unnecessary.)
mlgoatherder 4y ago
I recently implemented a very similar service to service gRPC authentication mechanism for Go using EC25519 and the noise protocol framework. https://github.com/sillystack/api/blob/main/transport/transport.go https://github.com/sillystack/api/blob/main/transport/transp... The code is very fresh and hasn't yet gone through a audit so please don't use it for anything where security actually matters.

125 points by bubblehack3r 4y ago

See also: https://github.com/flynn/noise https://github.com/flynn/noise

Interesting, though AFAIK a secure tunnel is only useful for a net.Conn, not an io.ReadWriter. What's the usecase for a "secure tunnel" over a bytes.Buffer? btw, I noticed that the decrypt function reads a 32-bit message length and immediately allocates a slice of that size. That means an attacker can send 0xFFFFFFFF and cause you to allocate 4GiB.

ikiris 4y ago
Yeah this code review isn’t gonna go great.
jerf 4y ago
You're kinda looking at the interface issue backwards. Being an io.ReadWriter is a promise that the tunnel code won't do anything other than read or write. The tunnel code shouldn't be getting local and remote addresses or setting deadlines of its own, or most of the rest of what net.Conn can do. https://pkg.go.dev/net#Conn https://pkg.go.dev/net#Conn It is good that it doesn't take more than it needs. That bytes.Buffer happens to implement io.ReadWriter is not of consequence; for any given task you want a io.Reader or io.Writer for there may be any number of specific implementations that don't make sense to use with it, but that doesn't mean you should require more methods of the interface. I'm speaking solely about the interface here; the many other concerns are valid.
- nemo1618 4y ago
  Sure, in general you want to use the narrowest possible interface. But in this particular case, I think `net.Conn` communicates intent better than `io.ReadWriter` -- the former implies that two distinct parties are involved, whereas `io.ReadWriter` is usually for (single-user) buffers or files. Sometimes it's sensible to use a larger interface than necessary; e.g. if you have a helper function that only calls `SetDeadline` and `Read`, the argument should be a `net.Conn`, not a custom interface with just those methods. tbh though, it's a very minor quibble and not really worth worrying about ‾\_(ツ)_/‾
  - jrockway 4y ago
    I disagree with this. If the only surface area of the underlying transport that the implementation uses is Read() and Write(), then it should be an io.Reader and an io.Writer. If it wants to mess around with read/write deadlines, then you'd have to bring in net.Conn, but it doesn't, so don't. If you're worried about someone using bytes.Buffer as their transport layer, net.Conn doesn't fix that; there is net.Pipe. (It's not buffered, though.)

FiloSottile 4y ago

There is no description of the protocol or of its security goals, so I am making some guesses based on a cursory look at the source and what I imagine this might be for. A single symmetric key is derived for both directions, and there is no checking of nonces, so as far as I can tell any message can be dropped, reordered, or replayed in both directions. (Including replaying message from A to B as if they were from B to A.) This is a bit like using ECB and likely to lead to fun application-specific attacks like [0]. This is very much rolling your own crypto, in a dangerous way. I am on the record as being "against" the "don't roll your own crypto" refrain [1], but mostly because it doesn't work: it should discourage people from publishing hand-rolled protocols such as this, but instead people think it means "don't roll your own primitives" and accept any use of "Ed25519/X25519" as probably secure. Please read about the Noise framework [2] to get an idea of how much nuance there is to this, and consider using a Go implementation of it [3] instead. P.S. This kind of issue is also why I maintain that NaCl is not a high-level scheme [4]: this could have used NaCl and have the exact same issues. libsodium has a couple slightly higher-level APIs that could have helped, secretstream [5] and kx [6], but again please use Noise. [0] https://cryptopals.com/sets/2/challenges/13 https://cryptopals.com/sets/2/challenges/13 [1] https://securitycryptographywhatever.buzzsprout.com/1822302/8953842-the-great-roll-your-own-crypto-debate-with-filippo-valsorda https://securitycryptographywhatever.buzzsprout.com/1822302/... [2] https://noiseprotocol.org/noise.html https://noiseprotocol.org/noise.html [3] https://github.com/flynn/noise https://github.com/flynn/noise [4] https://words.filippo.io/dispatches/nacl-api/ https://words.filippo.io/dispatches/nacl-api/ [5] https://libsodium.gitbook.io/doc/secret-key_cryptography/secretstream https://libsodium.gitbook.io/doc/secret-key_cryptography/sec... [6] https://libsodium.gitbook.io/doc/key_exchange https://libsodium.gitbook.io/doc/key_exchange

c7DJTLrn 4y ago
I once heard something along the lines of "if you're writing the words RSA, you're doing it wrong." I guess that goes for ED25519 also. If you want to secure a stream with asymmetric cryptography and don't need all the bells and whistles of TLS, what's the correct way to do it? Noise? Is there nothing simpler?
- 4y ago

jrockway 4y ago

There has been a lot of discussion about how the crypto doesn't work. I have some quibbles about the Go. Don't put a mutex in your io.Reader. It is assumed that, unless mentioned in the documentation, it is not safe to use anything in Go from multiple goroutines. If someone wants to make the reader synchronous for use among multiple goroutines, they can do so themselves. But it's so rare that it's unlikely anyone would want this. read/write mutexes typically perform worse than a plain mutex. You pay a performance cost to prevent readers and writers from starving each other; if you don't care (and this code doesn't), use a Mutex. https://zephyrtronium.github.io/articles/rwmutex.html https://zephyrtronium.github.io/articles/rwmutex.html Finally, it's fine to embed the mutex value directly in your struct if your methods take pointer receivers. &MyThing{Mutex: new(sync.Mutex)} is pretty weird to see. If the mutex were included as its value, then the zero value of MyThing is ready to use; &MyThing{} has a new mutex in it. (You can't copy the value of &MyThing either way; a correct copy requires holding the mutex. The reason to embed a *sync.Mutex instead of a sync.Mutex is so that you can copy the containing type, but that is actually unsafe. You grab a pointer to the wrong Mutex in your copy, and you also copy data protected by the mutex without holding it. So your methods MUST have a pointer receiver either way, and thus the indirection to *sync.Mutex is unnecessary.)

mlgoatherder 4y ago

I recently implemented a very similar service to service gRPC authentication mechanism for Go using EC25519 and the noise protocol framework. https://github.com/sillystack/api/blob/main/transport/transport.go https://github.com/sillystack/api/blob/main/transport/transp... The code is very fresh and hasn't yet gone through a audit so please don't use it for anything where security actually matters.