Prevent replays of server data

[bemasc]

No description provided.

[fortuna]

@riobard, answering your comment #77 (comment) here to keep it in context.

Yeah, this new implementation has a separate filter for the server. I'm asking @bemasc to add a new error string to track that in the Prometheus metrics, so we'll be able to tell whether the attacker is actually using that.

He decided to use a different approach:

salt_prefix = 28 random bytes
encrypted, tag = cipher.seal(nil, 0, salt_prefix, "outline-salt-mark")
salt = salt_prefix || tag

Validation happens by repeating the operation and checking the tag.

This way we don't need to rewire the code to get the cipher secret.

[fortuna]

Isn't this always true if keyErr is nil? This if is confusing me.

[bemasc]

Yes, this is equivalent to if keyErr == nil. This code runs whether keyErr is nil or not. If it would be clearer, we could move this to line 234.

[riobard]

@fortuna I think the salt-size issue still applies here. Assuming you're using 32-byte salt and 28-byte random, that leaves 4 bytes for the tag. Not sure if it's sufficient.

[bemasc]

@riobard With a 4-byte tag, the false-positive rejection probability is one in 4 billion. That's much lower than the false positive rejection rate from our existing replay defense, so I think it's acceptable.

[riobard]

@bemasc Ah, right! I forgot about the Bloom filter false positive rate :P Anyway, does Outline enforce 32-byte salt?

[fortuna]

The Outline Server only uses chacha20-ietf-poly1305, which has a 32-byte salt.
outline-ss-server supports only the AEAD ciphers, but all of them, including aes-128-gcm, which has a 16-byte salt.

I recommend against aes-128-gcm, and if someone is worried about a 12-byte salt, they can always migrate to the longer ciphers.

[bemasc]

I recommend against aes-128-gcm, and if someone is worried about a 12-byte salt, they can always migrate to the longer ciphers.

In this version I added a check that disables the marking for this case, to be conservative.

[fortuna]

Nice! I like that you added the test. This looks really good. I only have a small suggestion for the case of a small salt, so we catch that early, output a warning, and keep the salt generator simpler.

[fortuna]

Could you also add a section to the README explaining how the replay protection works? It can be short, but I'd like to have something I can point to other than the code. Cover both the client and server replays. Thanks!

[fortuna]

@madeye You may be interested in this marked salt approach for shadowsocks-libev. It removes the need for saving server salts and can survive resets and old replays.

[bemasc]

Could you also add a section to the README explaining how the replay protection works?

Done. I put it in a separate file and linked it from the README.

[fortuna]

This looks great! Thanks for suggesting and implementing the crypto salts. And I really like the document you added too. I'm looking forward to releasing this!