Date: Fri, 11 Oct 1996 11:39:17 -0700 From: Paul Traina <pst@jnx.com> To: Bill Fenner <fenner@parc.xerox.com> Cc: bde@freebsd.org, dg@root.com, olah@freebsd.org, wollman@freebsd.org, current@freebsd.org Subject: Re: Partial review of SYN attack patch Message-ID: <199610111839.LAA00439@base.jnx.com> In-Reply-To: Your message of "Thu, 10 Oct 1996 17:01:13 PDT." <96Oct10.170123pdt.177476@crevenia.parc.xerox.com>
next in thread | previous in thread | raw e-mail | index | archive | help
The recent crashes were because of a typo in my code, sorry, at the last
second I optimized out something I shouldn't have. The idea was to call
tcp_drop() and then recreate a new socket, rather than reuse the old one.
When I cleaned up the code to go to dropablereq() in socket, I fat-fingered
the call to create a new connection. sodropablereq() has a 1/n chance of
returning null so that the "new" connection is a canidate for dropable requests
too.
This code was extensively tested under -current and a similar patch was tested
under 2.1.5, but at the last second, I restructured things, which is why this
happened.
Big regrets, fix on the way.
Paul
From: Bill Fenner <fenner@parc.xerox.com>
Subject: Partial review of SYN attack patch
Karl Denninger's crashes seem to be occurring because of a bug in the
SYN attack patch in rev 1.52 of tcp_input.c:
so2 = sonewconn(so, 0);
if (so2 == 0) {
tcpstat.tcps_listendrop++;
so2 = sodropablereq(so);
if (so2)
tcp_drop(sototcpcb(so2), ETIMEDOUT);
else
goto drop;
}
so = so2;
/*
* This is ugly, but ....
*
* Mark socket as temporary until we're
* committed to keeping it. The code at
* ``drop'' and ``dropwithreset'' check the
* flag dropsocket to see if the temporary
* socket created here should be discarded.
* We mark the socket as discardable until
* we're committed to it below in TCPS_LISTEN.
*/
dropsocket++;
inp = (struct inpcb *)so->so_pcb;
inp->inp_laddr = ti->ti_dst;
inp->inp_lport = ti->ti_dport;
in_pcbrehash(inp);
tcp_drop:
- Sets the socket errno
- Frees the pcb
- Frees the inpcb (which doesn't free the socket because of SS_NOFDREF)
But the code goes on to use the socket and quickly deferences the NULL
inpcb. This code should probably be either
if (so2) {
tcp_drop(sototcpcb(so2), ETIMEDOUT);
tcp_attach(so2);
} else
goto drop;
or
if (so2) {
so2->so_flags &= ~SS_NOFDREF;
tcp_drop(sototcpcb(so2), ETIMEDOUT);
so2 = sonewconn(so, 0);
if (so2 == 0) /* can't happen? */
goto drop;
} else
goto drop;
The first has the disadvantage that tcp_attach is static in a different
file, but has the advantage of being able to reuse the socket. The
second has the disadvantage of destroying and recreating the socket.
A third option might actually be
if (so2 == 0)
goto drop;
After all, we're just about to update inp->inp_laddr and inp->inp_lport,
and if in_pcbrehash(inp) can handle inp's laddr and lport changing (as
opposed to being new) then this is probably the best option. Of course,
there's probably more to it than that, perhaps flags got set in the
tcpcb by the previous SYN, or there might have been data included, so
it's probably not possible to reuse the tcpcb like this.
It looks like the original code was trying to reuse the socket so2,
without realizing that tcp_drop -> tcp_close calls in_pcbdetach?
Did anyone actually test this code under attack? It seems like if
sodropablereq() returns a socket, then this code has no chance of not
dereferencing NULL.
Bill
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199610111839.LAA00439>