From owner-freebsd-current  Fri Oct 11 11:40:05 1996
Return-Path: owner-current
Received: (from root@localhost)
          by freefall.freebsd.org (8.7.5/8.7.3) id LAA17096
          for current-outgoing; Fri, 11 Oct 1996 11:40:05 -0700 (PDT)
Received: from red.jnx.com (red.jnx.com [208.197.169.254])
          by freefall.freebsd.org (8.7.5/8.7.3) with ESMTP id LAA17018;
          Fri, 11 Oct 1996 11:39:59 -0700 (PDT)
Received: from base.jnx.com (base.jnx.com [208.197.169.238]) by red.jnx.com (8.7.6/8.7.3) with ESMTP id LAA03876; Fri, 11 Oct 1996 11:39:18 -0700 (PDT)
Received: from base.jnx.com (localhost.jnx.com [127.0.0.1]) by base.jnx.com (8.7.6/8.7.3) with ESMTP id LAA00439; Fri, 11 Oct 1996 11:39:17 -0700 (PDT)
Message-Id: <199610111839.LAA00439@base.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 
In-reply-to: Your message of "Thu, 10 Oct 1996 17:01:13 PDT."
             <96Oct10.170123pdt.177476@crevenia.parc.xerox.com> 
Date: Fri, 11 Oct 1996 11:39:17 -0700
From: Paul Traina <pst@jnx.com>
Sender: owner-current@freebsd.org
X-Loop: FreeBSD.org
Precedence: bulk

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