Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a minimal replay defense #43

Merged
merged 6 commits into from Jan 8, 2020
Merged

Add a minimal replay defense #43

merged 6 commits into from Jan 8, 2020

Conversation

@bemasc
Copy link

bemasc commented Jan 3, 2020

This defends against replays of the most recent 10,000
handshakes using a simple in-memory cache.

This defends against replays of the most recent 10,000
handshakes using a simple in-memory cache.

This change also simplifies the AEAD parsing logic for TCP.
@bemasc bemasc requested a review from fortuna Jan 3, 2020
shadowsocks/tcp.go Outdated Show resolved Hide resolved
`tcpService.ciphers` has to be an interface because the
value is modified elsewhere.

This commit also includes improved comments and other
small changes.
shadowsocks/replay.go Outdated Show resolved Hide resolved
shadowsocks/replay_test.go Outdated Show resolved Hide resolved
shadowsocks/tcp.go Outdated Show resolved Hide resolved
shadowsocks/tcp.go Show resolved Hide resolved
shadowsocks/tcp.go Outdated Show resolved Hide resolved
shadowsocks/replay.go Outdated Show resolved Hide resolved
@bemasc bemasc requested a review from fortuna Jan 7, 2020
shadowsocks/tcp.go Outdated Show resolved Hide resolved
shadowsocks/tcp.go Outdated Show resolved Hide resolved
shadowsocks/tcp.go Outdated Show resolved Hide resolved
shadowsocks/tcp.go Outdated Show resolved Hide resolved
ciphers: ciphers,
m: m,
readTimeout: timeout,
replayCache: NewReplayCache(replayHistory),

This comment has been minimized.

Copy link
@fortuna

fortuna Jan 7, 2020

This is creating a replayCache for each port.
I think this should be at SServer instead, and shared across the port.tcpServices, like the metrics, so servers with multiple ports don't get bloated. We have some long running servers, some may have dozens of ports.

Another thing in my mind: It should be perfectly fine for two different keys to use the same salt, since the encrypted content will be different anyway. Should we be isolating the different keys?

I'm also concerned about changing the behavior of existing servers. There should be at least a escape hatch. Let's have a flag with the value for the replayHistory, so server owners can disable if they need to. Or should it be a config option?

This can be useful for a gradual rollout. Let's discuss how we'll roll this out in a safe way.

This comment has been minimized.

Copy link
@bemasc

bemasc Jan 8, 2020

Author

This is creating a replayCache for each port.
I think this should be at SServer instead, and shared across the port.tcpServices, like the metrics

Done.

Another thing in my mind: It should be perfectly fine for two different keys to use the same salt, since the encrypted content will be different anyway. Should we be isolating the different keys?

This seems like a pathological client behavior, but I've addressed it by mixing the key ID into the map key, so that the same salt on different keys is unlikely to collide in the map.

I'm also concerned about changing the behavior of existing servers. There should be at least a escape hatch. Let's have a flag with the value for the replayHistory, so server owners can disable if they need to. Or should it be a config option?

OK, I added a flag.

This also optimizes map allocation and mixes
the key ID into the map key.
@bemasc bemasc force-pushed the bemasc-replay-min branch from 870a9c8 to 1eaf7b6 Jan 8, 2020
@bemasc bemasc requested a review from fortuna Jan 8, 2020
@fortuna
fortuna approved these changes Jan 8, 2020
@bemasc bemasc merged commit 3c2b0f1 into master Jan 8, 2020
1 check passed
1 check passed
cla/google All necessary CLAs are signed
@bemasc bemasc deleted the bemasc-replay-min branch Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.