Numerous design flaws

[tarcieri]

Designing a transport encryption protocol is among the most difficult undertakings in cryptography. It's something that I would leave in the hands of a professional cryptographer who is already well-versed in the attacks on TLS.

Your project more or less duplicates the functionality of spiped:

https://www.tarsnap.com/spiped.html

However, you have made a number of mistakes in your design:

• There is no reason to use RC4 in new protocols. RC4 has known biases which can be used for plaintext recovery. ChaCha20 is faster than RC4 and substantially more secure
• There is no reason to use AES-CFB in new protocols. Use AES-GCM.
• No cryptographic MAC is applied to the ciphertext, leaving you vulnerable to ciphertext malleability attacks. Your protocol is, in fact, less secure than SSLv3. Again, use AES-GCM, or ChaCha20+Poly1305
• MD5 is used as a KDF. That's gross. Use HKDF with a hash function considered secure today, like SHA-256 or SHA-512, or a keyed hash like Blake2
• AES-256 is used with a key derived from 128-bits entropy. That's pointless. If you have 128-bits entropy for your keying material, use AES-128
• The same key is used in both directions, increasing the chances of IV reuse. Ideally you use a separate key for each direction
• I can't even figure out the IV strategy here. I hope some high level API is picking your IVs randomly
• No defense against replay attacks

...and that's what I found after looking at it for about 20 minutes.

You should probably be using spiped or TLS in PSK mode

[rpicard]

This is not a secure way to zero memory as far as I know: https://github.com/getqujing/qtunnel/blob/master/src/tunnel/cipher.go#L31

See here (same guy from Tarsnap incidentally): http://www.daemonology.net/blog/2014-09-04-how-to-zero-a-buffer.html

[lunixbochs]

That line in secretToKey() isn't zeroing memory, it's using an MD5 hash as a KDF. h.Sum(nil) is defined here and simply returns the current hash as a []byte.

As far as securely-clearing memory in Go: I haven't found any definite documentation on it, but I'm pretty sure Go zeroes newly-allocated memory (and a couple of tests confirm it). It also forces array bounds checking, so there's not an obvious way in Go to accidentally reveal recycled memory contents to a user. This means you're probably okay regardless of whether you zeroed something unless you link in a vulnerable C library (like an SSL stack) and someone somehow uses it to read your memory.

[rpicard]

@lunixbochs Okay, yeah I thought it was weird that they were using the MD5 object to create nil stuff for the zeroing. That makes more sense.

[ghoulr]

Thanks for your professional advice @tarcieri, I'm trying to implement a new version of encryption protocol now:

1. For original AES-CFB encryption, use a new IV strategy, client and server will send their random IV to each other at the very start after the connection is established, then use it in the each direction of communication.
2. Add AES-GCM, client and server will send their random nonce to each other after connection is established, then use ad = nonce_client ^ nonce_server as additional data, and use the nonces on each directions like the IV.
3. Use SHA256 instead of MD5 in https://github.com/getqujing/qtunnel/blob/master/src/tunnel/cipher.go#L23, to calculate a key.
4. RC4 is only used in early development, and chacha20 will import 3rd party dependencies rather than go itself, which I'm not prefer, so let's just leave it there for now...

As I said in README, the reason I'm not using TLS based tools or spiped (this is a great tool, but I didn't know it before), is I'm avoiding the handshake before the real communication, this tool is used in the scenario that client and server may talk to each other oversea, and I don't want cost >500ms for handshaking. A simple but secure enough protocol would be good enough.

Any suggestions?

[josh-chan]

@ghoulr Go 版 shadowsocks 已经实现 AEAD (aes-gcm) 加密，能移植到 qtunnel 吗？