[HN Gopher] Whisper: Wraps any Go io.ReadWriter in a secure tunn...
___________________________________________________________________
Whisper: Wraps any Go io.ReadWriter in a secure tunnel using
Ed25519/X25519
Author : bubblehack3r
Score : 105 points
Date : 2023-02-19 13:31 UTC (9 hours ago)
(HTM) web link (github.com)
(TXT) w3m dump (github.com)
| FiloSottile wrote:
| 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
|
| [1]
| https://securitycryptographywhatever.buzzsprout.com/1822302/...
|
| [2] https://noiseprotocol.org/noise.html
|
| [3] https://github.com/flynn/noise
|
| [4] https://words.filippo.io/dispatches/nacl-api/
|
| [5] https://libsodium.gitbook.io/doc/secret-
| key_cryptography/sec...
|
| [6] https://libsodium.gitbook.io/doc/key_exchange
| zxcvbn4038 wrote:
| Really great feedback, I hope it makes it to the module author.
| c7DJTLrn wrote:
| 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?
| d-z-m wrote:
| As far as language support goes, it doesn't get much easier
| than working with TLS, especially in Go.
|
| As far as alternatives go, Noise Pipes[0] never took off, but
| feel like they'd be well suited to this task.
|
| [0]: https://noiseprotocol.org/noise.html#noise-pipes
| mwcampbell wrote:
| IIUC the correct answer, according to the 2009 blog post that
| popularized the saying you're referring to, is to just use
| TLS anyway. In most cases it's better to accept some extra
| complexity than to get crypto wrong.
|
| Edit: Here's that classic: https://web.archive.org/web/200906
| 06064715/http://www.matasa...
| ThePhysicist wrote:
| I mean this looks like standard ECIES to me. I think it could
| even count as a Noise protocol, as in my understanding that
| protocol family subsumes most of the classic Diffie Hellman
| based key exchanges. Reading the source the author seems to use
| a long-lived ECDSA key pair to sign and exchange an ephemeral
| ECDH key pair, then derives a symmetric AES key from that and
| uses that for AES-GCM. Not sure how you would do message
| reordering here as GCM takes care of authenticating your data
| and keys are not reused. If you want replay protection it's
| probably sufficient to either remember used keys or add a
| timestamp. It's a well-done example of writing such a simple
| protocol, I don't think the author plans to replace TLS with it
| and he even mentions that in the README.
| d-z-m wrote:
| > If you want replay protection it's probably sufficient to
| either remember used keys or add a timestamp.
|
| The replay Filippo is talking about is in the context of a
| single key. The remote side simply reads the nonce || message
| off the wire and aead.Opens it, without regard to whether or
| not that nonce has been used before to decrypt a previous
| message.
| tptacek wrote:
| Can you express this with Noise framework tokens? I don't
| think you can. Noise is fiddlier than it looks! It's not just
| an ordering of DH exchanges; it's transcript hashes, cipher
| state tracking (and reinitializing), key derivation, the
| whole 9. The temptation (at least for me) is to just skip to
| the table of handshakes and skim, but the actual protocol
| framework is in Section 5, where they define precisely what
| each of those tokens really entails.
|
| (To say nothing of: this uses signatures, and Noise does
| not).
| [deleted]
| erincandescent wrote:
| Gotta say that I generally like the Noise framework (or
| rather the protocols that result), but it is one of the
| most impenetrable specifications I've ever read
|
| I don't remember what it is specifically about it, I just
| remember the document being a pain to read; a bit like the
| original Paxos paper in that regard.
| tptacek wrote:
| I don't know about that. As an implementor, it is
| probably one of the easiest-to-follow specs I've ever
| worked from. You can pretty much code it from the top of
| the spec to the bottom; when you get to the handshake
| patterns section and realize that each is just a
| different ordering of things in an array or whatnot, it's
| pretty slick.
| ThePhysicist wrote:
| Probably not, though Noise mentions that you could replace
| DH operations with signatures.
| tptacek wrote:
| Yeah, I don't mean to be coy. You can't end up with this
| protocol using Noise. :)
| ThePhysicist wrote:
| ECIES is quite fine though for most asynchronous
| applications, where having a signing key also makes sense
| as you often want to publish long-lived, signed data and
| build a trust chain (e.g. generate and sign session keys
| from a master key). I built several real-world systems
| based on that (e.g. [1]) and they all made it through the
| audits fine. I was exploring Noise-based protocols but I
| find it's best to rely on primitives that are supported
| by the Web Crypto API.
|
| 1: https://github.com/kiebitz-oss/
| tptacek wrote:
| ECIES is a hybrid encryption construction; Noise is a
| protocol. They're two different levels of abstraction.
| This thing we're commenting on has a protocol; it's just
| an accidental one, which is usually not what you want.
| WebCrypto doesn't provide a protocol framework, just a
| bunch of primitives.
| FiloSottile wrote:
| Each Write encrypts a separate "record" (in the parlance of
| TLS and Noise). Each of those records can be arbitrarily
| dropped, replayed, or reflected.
|
| Here's an example, if you do Write("Hello")
| Write(" the password is ") Write("password")
|
| then without a key I can make you or your peer read "Hello
| the password is Hello" (or "HelloHelloHello" or any
| composition of those messages).
|
| No Noise protocol would allow that.
| ThePhysicist wrote:
| Oh well, I did not realize he uses it like that.
| mlgoatherder wrote:
| Great opportunity to pitch my recent work on a similar solution
| for gRPC:
|
| https://github.com/sillystack/api/blob/main/transport/transp...
|
| I'm not going to make any security claims as of now (it's
| almost certainly broken) but it uses the Noise IK handshake.
|
| Long term I think NoiseSocket would be a good way to go
| https://noisesocket.org/post/1/ but perhaps in service-to-
| service use cases ciphersuite negotiation can be simplified.
| tptacek wrote:
| See also: https://github.com/flynn/noise
| mlgoatherder wrote:
| 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/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.
| nemo1618 wrote:
| 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 wrote:
| Yeah this code review isn't gonna go great.
| jerf wrote:
| 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
|
| 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 wrote:
| 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 -\\_(tsu)_/-
| jrockway wrote:
| 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.)
| jrockway wrote:
| 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
|
| 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.)
___________________________________________________________________
(page generated 2023-02-19 23:01 UTC)