Date: Mon, 10 Jan 2005 00:50:06 -0800 (PST) From: Don Lewis <truckman@FreeBSD.org> To: silby@silby.com Cc: net@FreeBSD.org Subject: Re: Slipping in the window update Message-ID: <200501100850.j0A8o6FY019623@gw.catspoiler.org> In-Reply-To: <20050109031431.H2669@odysseus.silby.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 9 Jan, Mike Silbersack wrote:
>
> Ok, here's an updated patch for the SYN case. I've included the patch
> relative to 6.x, and some text from a tcpdump showing it in action.
>
> It responds to each SYN with an ACK like the latest tcpsecure document
> states, but it uses a global counter to rate limit the number of ACKs of
> this type that it will send to 200 per second.
>
> I was unable to incorporate the connect idle heuristic I wanted to because
> right now the incoming spoofed syns would reset the idle counter, which
> sounds like it could cause a problem somehow... best not to use it for
> now. Maybe a future change can clean up that along with the dropafterack
> case in tcp_input, but that would make this patch far too complex.
>
> Please take a look at the patch and the abbreviated tcpdump from my test
> and see if it looks correct.
> + if (thflags & TH_SYN) {
> + if (tp->t_state == TCPS_ESTABLISHED &&
> + tcp_insecure_syn == 0) {
Any good reason for the extra level of nesting?
Testing the SYN flag first is probably optimum, since in normal
operation the vast majority of segments won't have this flag set.
> + if (tp)
> + INP_UNLOCK(inp);
If we've successfully dereferenced tp->t_state, it should not be
necessary to protect INP_UNLOCK() with
if (tp)
> + if (headlocked)
> + INP_INFO_WUNLOCK(&tcbinfo);
I suspect that the headlocked flag is also known at this point in the
code.
Ordinary data segments will have the TH_SYN checked twice. The first
time in this new code, and the second time after the segment has been
trimmed to fit the window.
/*
* If a SYN is in the window, then this is an
* error and we send an RST and drop the connection.
*/
if (thflags & TH_SYN) {
tp = tcp_drop(tp, ECONNRESET);
rstreason = BANDLIM_UNLIMITED;
goto drop;
}
This could make a bit of a performance difference at high speeds, for
instance gigabit Ethernet in a compute cluster.
An alternate fix would be to modify the latter block of code as follows:
if (thflags & TH_SYN) {
+ if (tp->t_state == TCPS_ESTABLISHED &&
+ tcp_insecure_syn == 0)
+ goto dropafterack;
tp = tcp_drop(tp, ECONNRESET);
rstreason = BANDLIM_UNLIMITED;
goto drop;
}
and then after the dropafterack label add the code:
+ if (thflags & TH_SYN) {
+ if (tp->t_state == TCPS_ESTABLISHED &&
+ tcp_insecure_syn == 0) {
+ if (badport_bandlim(BANDLIM_SYN_ESTABLISHED) < 0)
+ goto drop;
+ tcp_respond(tp, mtod(m, void *), th, m, tp->rcv_nxt,
+ tp->snd_una, TH_ACK);
[snip]
I don't think this fix would be complete from the response rate limiting
point of view because this chunk of code in the block that trims to the
left window edge tosses the TH_SYN flag.
todrop = tp->rcv_nxt - th->th_seq;
if (todrop > 0) {
if (thflags & TH_SYN) {
thflags &= ~TH_SYN;
th->th_seq++;
if (th->th_urp > 1)
th->th_urp--;
else
thflags &= ~TH_URG;
todrop--;
}
and this block of code doesn't jump to dropafterack, even in the case
where the entire segment is to the left of the window. Something else
would have to be done to implement rate limiting for this half of the
sequence space.
Now that I've looked at the above case, it looks to me like your
suggested patch might affect the response to a legitimate duplicate SYN.
It will definitely follow a different code path.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200501100850.j0A8o6FY019623>