From owner-freebsd-net@FreeBSD.ORG Mon Jan 10 08:50:14 2005 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 44E0A16A4CE for ; Mon, 10 Jan 2005 08:50:14 +0000 (GMT) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id C352443D3F for ; Mon, 10 Jan 2005 08:50:13 +0000 (GMT) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.1/8.13.1) with ESMTP id j0A8o6FY019623; Mon, 10 Jan 2005 00:50:10 -0800 (PST) (envelope-from truckman@FreeBSD.org) Message-Id: <200501100850.j0A8o6FY019623@gw.catspoiler.org> Date: Mon, 10 Jan 2005 00:50:06 -0800 (PST) From: Don Lewis To: silby@silby.com In-Reply-To: <20050109031431.H2669@odysseus.silby.com> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii cc: net@FreeBSD.org Subject: Re: Slipping in the window update X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Jan 2005 08:50:14 -0000 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.