Improve handling of syncookies

[tuexen]

Summary

Syncoockies can be used in combination with the syncache. If the cache overflows, syncookies are used.
This patch restricts the usage of syncookies in this case: accept syncookies only if there was an overflow of the syncache recently.

This mitigates a problem reported in PR217637.

Test Plan

Get the following packetdrill tests passed:

• pr217637-cache.pkt2 KBDownload
• pr217637-cookie.pkt2 KBDownload
• pr217637-cache-cookie.pkt2 KBDownload

[glebius]

Looks like there is a tiny window for improvement. See inline comment.

sys/netinet/tcp_syncache.c

999 ↗

(On Diff #27015)

So here is an idea on to reduce the check to:

if (!V_tcp_syncookiesonly && sch->sch_last_overflow < time_uptime - SYNCOOKIE_LIFETIME) {

if you initialize sch->sch_last_overflow to maximum value in syncache_init(). So, if syncache has never overflowed, the value would be all-ones, instead of all-zeroes. time_uptime is time_t, which is signed, so even at first 15 seconds of uptime we are just fine.

[tuexen]

Simplify the check based on Gleb's suggestion.

[gnn]

Note that this has failed to build according to the build bot (see Harbormaster). Thoughts on that?

[tuexen]

I have no idea why it doesn't build. I tested the patch locally on amd64 (compiling and running the test scripts). Is there any way to get the error message from the console it figure out what went wrong. Please note that I have the same problem in https://reviews.freebsd.org/D9894.

https://reviews.freebsd.org/harbormaster/build/8812/?l=0 only show an HTTP header, but no console output... Any hints appreciated.

[bz]

Question: do we add time dependent bits somewhere so we could simply check if the syn-cookie itself is outside a possible replay-window? I know this does not entirely close the race, but I don't see how this would differ from the proposed "global" solution but would be working independent on whether a syn-cache is also used or not. Am I completely missing a point here? I think 4987 mentioned something along these lines but it's been ages since I last looked at SYN problems.

[tuexen]

A cookie lifetime is realised by changing the secret. The point here is not to process valid cookies since there was recently no overflow. The cookies lifetime has not expired. It is valid. We just should not processed it, since we established the connection in the past (using the syncache) and dropped the connection.

[bz]

@tuexen right, assume you run with syn-cookies only, how do you mitigate the replay; I thought that is the actual problem we are looking at (whether or not there is a syn-cache in use).

[jason_eggnet.com]

Here's an idea from gnn that I agree with:

The fundamental issue here is that the syncookie is good enough to pass validation, but there is sufficient information in the syncache entry to reject it. The patch proposed here is a workaround for deleting the syncache entry.

I think the original problem is that we deleted the syncache entry. Why not set a flag or change the state of the syncache entry to prevent future packets from being accepted for that connection, and let the syncache expiry mechanism deal with deleting the syncache entry eventually.

[sepherosa_gmail.com]

In D10272#213213, @jason_eggnet.com wrote:

Here's an idea from gnn that I agree with:

The fundamental issue here is that the syncookie is good enough to pass validation, but there is sufficient information in the syncache entry to reject it. The patch proposed here is a workaround for deleting the syncache entry.

I think the original problem is that we deleted the syncache entry. Why not set a flag or change the state of the syncache entry to prevent future packets from being accepted for that connection, and let the syncache expiry mechanism deal with deleting the syncache entry eventually.

Everyone fall back to syncookie under heavy load, if you reap syncache using callout.

[jason_eggnet.com]

In D10272#213260, @sepherosa_gmail.com wrote:

Everyone fall back to syncookie under heavy load, if you reap syncache using callout.

That's ok, there's no fix for that condition relative to this issue.

[tuexen]

@bz: No, the proposed patch does not address the problem that syn cookies can be replayed. It only addresses the problem in the case where you use syn cookies in case of syncache overflow and there was no overflow recently.

[gnn]

The Transport group discussed this during a conference call and decided that this is good to go in.

[glebius]

Thanks for implementing my idea despite I made a mistake in its description :)