vulnerable against reflection, replay, reordering, and nonce-reuse

[t0b0]

Each packet in the stream is encrypted with its own, randomly generated nonce. This can lead to nonce re-use (when using AES GCM) if encrypting more than 2^32 packets. That would mean that keys need to be rotated that often. I doubt a user of the library would do that. Also, because there is no sense of direction, an attacker could reflect a packet sent by a client back to the client and the client would mistake it as a response from the server. There is also no authenticated packet counter, meaning that a man in the middle (MITM) attacker could replay and re-order packets without detection by the recipient.

Overall, I think it'd be fair to say that this library achieves confidentiality in presence of an eavesdropper, but no security in presence of a MITM.

[yilunzhang]

Thanks for your comments!

Each packet in the stream is encrypted with its own, randomly generated nonce. This can lead to nonce re-use (when using AES GCM) if encrypting more than 2^32 packets. That would mean that keys need to be rotated that often. I doubt a user of the library would do that.

Default nonce size for GCM is 16 bytes, and can be increased using NewGCMWithNonceSize if needed. Nonce size for XSalsa20-Poly1305 is 24 bytes. For both case nonce re-use should not be a concern.

There is also no authenticated packet counter, meaning that a man in the middle (MITM) attacker could replay and re-order packets without detection by the recipient.

Currently this package is used with ncp, which contains packet ID and handles attacks such as replay, reorder, etc. But I agree that adding it into this package to make the package secure in the presence of MITM is better.

[yilunzhang]

@t0b0 What do you think of this solution that is compatible with current data format: Instead of using independent nonce for each packet, each client generate a random nonce initially, and use previous nonce + 1 for next packet sent. Also we require that the initial nonce from the both side should be different to prevent reflection attack of the first packet.

[t0b0]

@yilunzhang It looks like the default nonce size for GCM is 12 bytes, not 16 bytes, according to the link you provided. Then nonce-reuse becomes a problem. Even with 16 bytes, it is only secure up to 2^64 messages, which isn't too difficult to reach if this was to be deployed on a high-throughput system. With XSalsa and 24 bytes, I agree that nonce-reuse isn't an issue.

When starting to think about adding additional protections, I am really hesitant to "roll my own crypto". I've seen record protocols security fail in interesting ways before. Maybe it'd be good to have a look at an established protocol like SSH, TLS 1.3 and see how they are doing it? There is currently no concept of doing a handshake in encrypted-stream in order to establish a session key. Without that handshake, it's impossible to defend against replay attacks. The scheme you described of starting with a random nonce and then incrementing it for each packet would make it harder for an attacker to replay any packet or to re-order or drop packets in the middle, but it wouldn't prevent him from dropping packets at the beginning of the stream or replaying an entire stream. We could maybe somehow designate the first message as special and thereby protect against the attacker dropping messages at the start of a stream, but it still wouldn't protect against replays of an entire stream.

If we wanted to implement a handshake protocol, I'd start with the noise protocol framework (https://noiseprotocol.org/noise.html). libhydrogen is an example implementation.

[yilunzhang]

@t0b0 My bad, it's 12 not 16 byte. But 12 bytes is 2^96 messages, which should be enough for practical use cases.

There is currently no concept of doing a handshake in encrypted-stream in order to establish a session key.

That is intentional actually. In our case, we do have handshake protocol before using encrypted-stream to encrypt the stream to negotiate the session key and can prevent the whole session to be replayed, but we didn't include the handshake part into encrypted-stream, otherwise encrypted-stream will be a lot more complicated and coupled with tons of handshake configs (see noise config, for example).

I can think of two ways to make it better: either integrate the handshake part into encrypted-stream to make it a complete protocol that doesn't require handshake to be done beforehand. or state it clear that one must do handshake beforehand and just use encrypted-stream to wrap the conn.

[t0b0]

12 byte nonces are enough for practical use, if you chose nonces incrementally and have less than 2^96 messages. But if you chose nonces randomly, the birthday paradox applies and the probability of a collision becomes 2^-64 after 2^32 messages. See also e.g. https://pkg.go.dev/crypto/cipher@go1.17#example-NewGCM-Decrypt: "// Never use more than 2^32 random nonces with a given key because of the risk of a repeat."

I think saying that this package doesn't handle handshakes and that the user must supply a new key for each session would be fair. But that would make this package more difficult to use since you couldn't simply wrap a net.Conn anymore, right?

The question of how to do counters remains: Starting at a random counter would allow the attacker to drop packets at the beginning of the stream. If we start the counter at 0, we could be introducing vulnerabilities in projects which rely on this package, if they use a static key.

[yilunzhang]

I think saying that this package doesn't handle handshakes and that the user must supply a new key for each session would be fair. But that would make this package more difficult to use since you couldn't simply wrap a net.Conn anymore, right?

Even for now people still need to obtain or exchange public key, then derive the shared key before wrapping net.Conn. Maybe the most user friendly way (for future improvement) is to add support for noise framework, so people can simply pass a net.Conn and a noise config to get everything work.

The question of how to do counters remains: Starting at a random counter would allow the attacker to drop packets at the beginning of the stream. If we start the counter at 0, we could be introducing vulnerabilities in projects which rely on this package, if they use a static key.

One way without changing data frame is to introduce direction in config. Then session initializer will start with nonce 0 and increments from there; session responder will start with nonce max_nonce/2 and increments from there.

[yilunzhang]

@t0b0 These issues should be fixed by #8. In the future we can add native support for noise framework for easier usage.

[yilunzhang]

Closed due to inactivity. Feel free to reopen it if needed