Page MenuHomeFreeBSD

Improve handling of syncookies
ClosedPublic

Authored by tuexen on Apr 4 2017, 9:11 AM.

Details

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:

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tuexen created this revision.Apr 4 2017, 9:11 AM
Herald added a subscriber: imp. · View Herald Transcript
glebius requested changes to this revision.Apr 4 2017, 3:01 PM

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.

This revision now requires changes to proceed.Apr 4 2017, 3:01 PM
tuexen updated this revision to Diff 27052.Apr 4 2017, 7:25 PM
tuexen edited edge metadata.

Simplify the check based on Gleb's suggestion.

gnn edited edge metadata.Apr 4 2017, 9:56 PM

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

tuexen added a comment.Apr 5 2017, 5:46 AM

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.

jch added a subscriber: jch.Apr 6 2017, 4:26 PM
bz added a subscriber: bz.Apr 6 2017, 9:38 PM

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.

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 added a comment.EditedApr 6 2017, 10:06 PM

@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).

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.

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.

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 added a comment.Apr 7 2017, 7:21 AM

@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.

emaste added a subscriber: emaste.Apr 13 2017, 5:45 PM
gnn accepted this revision.Apr 20 2017, 4:09 PM

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

This revision is now accepted and ready to land.Apr 20 2017, 4:09 PM
glebius accepted this revision.Apr 20 2017, 4:25 PM

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

This revision was automatically updated to reflect the committed changes.