Date: Tue, 2 Dec 1997 00:17:09 -0800 From: Don Lewis <Don.Lewis@tsc.tdk.com> To: freebsd-hackers@freebsd.org Cc: fenner@PARC.XEROX.COM, avalon@coombs.anu.edu.au, jas@flyingfox.com Subject: fixes for "LAND" and various other TCP bugs Message-ID: <199712020817.AAA10175@salsa.gv.tsc.tdk.com>
next in thread | raw e-mail | index | archive | help
While studying the "LAND" bug I stumbled across a couple of other
things that look like bugs in the FreeBSD TCP code.
The most serious is that the introduction of the segment trimming
fix from Stevens looks like it introduced a bug in RST processing
that allows reset segments with sequence numbers spanning half the
sequence number space to be accepted as valid and processed, which
is contrary to RFC 793. I think the original code didn't really get
this totally right, either.
The second is that reset segments terminate connections in the
TIME-WAIT state. While this is what RFC 793 says to do, it is
discouraged by RFC 1337.
The attached patch contains three partially redundant fixes (two
new ones plus the present one relocated) for the "LAND" DoS attack
and a related problem (ACK wars caused by sending spoofed SYNs to
two listening sockets) as well as fixes the above problems.
I did not include the following change suggested by RFC 1337 in
section 2.1, but the final part of my "LAND" fix is really a special
case of this:
H2. The new connection may be de-synchronized, with the two ends
in permanent disagreement on the state. Following the spec
of RFC-793, this desynchronization results in an infinite ACK
loop. (It might be reasonable to change this aspect of RFC-
793 and kill the connection instead.)
I'm wondering if it might be wise to actually make this change, since
it should provide at least partial protection against TCP splicing attacks.
Something else that is missing is a fix for the potential to connect
two listening sockets to each other by sending them each forged SYNs
with carefully chosen sequence numbers. This should be easy to fix
if RFC 1122 is followed:
4.2.2.11 Recovery from Old Duplicate SYN: RFC-793 Section 3.4,
page 33
Note that a TCP implementation MUST keep track of whether a
connection has reached SYN_RCVD state as the result of a
passive OPEN or an active OPEN.
If this is the case, we would only expect to see a SYN-ACK in the
SYN-RECEIVED state if the previous state was SYN-SENT. If the
previous state was not SYN-SENT, then an RST should be sent and
the connection dropped.
This patch changes the location of the currently implemented "LAND" bug
fix, which should allow legitimate self-connects to work. My testing
with my 2.1-stable machine indicates that it is not vulnerable to the
problem even without this protective measure.
I *think* this is the correct fix, but I'm interested in any comments
from the TCP experts, especially with regards to T/TCP.
--- tcp_input.c.2_2 Mon Dec 1 16:49:21 1997
+++ tcp_input.c Mon Dec 1 22:28:58 1997
@@ -318,19 +318,6 @@
#endif /* TUBA_INCLUDE */
/*
- * Reject attempted self-connects. XXX This actually masks
- * a bug elsewhere, since self-connect should work.
- * However, a urrently-active DoS attack in the Internet
- * sends a phony self-connect request which causes an infinite
- * loop.
- */
- if (ti->ti_src.s_addr == ti->ti_dst.s_addr
- && ti->ti_sport == ti->ti_dport) {
- tcpstat.tcps_badsyn++;
- goto drop;
- }
-
- /*
* Check that TCP offset makes sense,
* pull out TCP options and adjust length. XXX
*/
@@ -654,6 +641,24 @@
if (m->m_flags & (M_BCAST|M_MCAST) ||
IN_MULTICAST(ntohl(ti->ti_dst.s_addr)))
goto drop;
+
+ /*
+ * Reject attempted self-connects.
+ *
+ * Doing the test here should prevent the "LAND" DoS
+ * attack without affecting legitimate self-connects
+ * which will occur in the SYN-SENT state.
+ *
+ * In the dropafterack code below we'll also fix the real
+ * bug in the SYN-RECEIVED state that causes the infinite
+ * loop since it can also be used to generate ACK storms.
+ */
+ if (ti->ti_src.s_addr == ti->ti_dst.s_addr
+ && ti->ti_sport == ti->ti_dport) {
+ tcpstat.tcps_badsyn++;
+ goto drop;
+ }
+
am = m_get(M_DONTWAIT, MT_SONAME); /* XXX */
if (am == NULL)
goto drop;
@@ -962,17 +967,99 @@
/*
* States other than LISTEN or SYN_SENT.
- * First check timestamp, if present.
+ * First check the RST flag and sequence number since reset segments
+ * are exempt from the timestamp and connection count tests. This
+ * fixes a bug introduced by the Stevens, vol. 2, p. 960 bugfix
+ * below which allowed reset segments in half the sequence space
+ * to fall though and be processed (which gives forged reset
+ * segments with a random sequence number a 50 percent chance of
+ * killing a connection).
+ * Then check timestamp, if present.
* Then check the connection count, if present.
* Then check that at least some bytes of segment are within
* receive window. If segment begins before rcv_nxt,
* drop leading data (and SYN); if nothing left, just ack.
*
+ *
+ * If the RST bit is set, check the sequence number to see
+ * if this is a valid reset segment.
+ * RFC 793 page 37:
+ * In all states except SYN-SENT, all reset (RST) segments
+ * are validated by checking their SEQ-fields. A reset is
+ * valid if its sequence number is in the window.
+ * Note: this does not take into account delayed ACKs, so
+ * we should test against last_ack_sent instead of rcv_nxt.
+ * Also, it does not make sense to allow reset segments with
+ * sequence numbers greater than last_ack_sent to be processed
+ * since these sequence numbers are just the acknowledgement
+ * numbers in our outgoing packets being echoed back at us,
+ * and these acknowledgement numbers are monotonically
+ * increasing.
+ * If we have multiple segments in flight, the intial reset
+ * segment sequence numbers will be to the left of last_ack_sent,
+ * but they will eventually catch up.
+ * In any case, it never made sense to trim reset segments to
+ * fit the receive window since RFC 1122 says:
+ * 4.2.2.12 RST Segment: RFC-793 Section 3.4
+ *
+ * A TCP SHOULD allow a received RST segment to include data.
+ *
+ * DISCUSSION
+ * It has been suggested that a RST segment could contain
+ * ASCII text that encoded and explained the cause of the
+ * RST. No standard has yet been established for such
+ * data.
+ *
+ * If the reset segment passes the sequence number test examine
+ * the state:
+ * SYN_RECEIVED STATE:
+ * If passive open, return to LISTEN state.
+ * If active open, inform user that connection was refused.
+ * ESTABLISHED, FIN_WAIT_1, FIN_WAIT2, CLOSE_WAIT STATES:
+ * Inform user that connection was reset, and close tcb.
+ * CLOSING, LAST_ACK, TIME_WAIT STATES
+ * Close the tcb.
+ * TIME_WAIT state:
+ * Drop the segment - see Stevens, vol. 2, p. 964 and
+ * RFC 1337.
+ */
+ if (tiflags&TH_RST) {
+ if (tp->last_ack_sent == ti->ti_seq) {
+ switch (tp->t_state) {
+
+ case TCPS_SYN_RECEIVED:
+ so->so_error = ECONNREFUSED;
+ goto close;
+
+ case TCPS_ESTABLISHED:
+ case TCPS_FIN_WAIT_1:
+ case TCPS_FIN_WAIT_2:
+ case TCPS_CLOSE_WAIT:
+ so->so_error = ECONNRESET;
+ close:
+ tp->t_state = TCPS_CLOSED;
+ tcpstat.tcps_drops++;
+ tp = tcp_close(tp);
+ break;
+
+ case TCPS_CLOSING:
+ case TCPS_LAST_ACK:
+ tp = tcp_close(tp);
+ break;
+
+ case TCPS_TIME_WAIT:
+ break;
+ }
+ }
+ goto drop;
+ }
+
+ /*
* RFC 1323 PAWS: If we have a timestamp reply on this segment
* and it's less than ts_recent, drop it.
*/
- if ((to.to_flag & TOF_TS) != 0 && (tiflags & TH_RST) == 0 &&
- tp->ts_recent && TSTMP_LT(to.to_tsval, tp->ts_recent)) {
+ if ((to.to_flag & TOF_TS) != 0 && tp->ts_recent &&
+ TSTMP_LT(to.to_tsval, tp->ts_recent)) {
/* Check to see if ts_recent is over 24 days old. */
if ((int)(tcp_now - tp->ts_recent_age) > TCP_PAWS_IDLE) {
@@ -1003,10 +1090,19 @@
* RST segments do not have to comply with this.
*/
if ((tp->t_flags & (TF_REQ_CC|TF_RCVD_CC)) == (TF_REQ_CC|TF_RCVD_CC) &&
- ((to.to_flag & TOF_CC) == 0 || tp->cc_recv != to.to_cc) &&
- (tiflags & TH_RST) == 0)
+ ((to.to_flag & TOF_CC) == 0 || tp->cc_recv != to.to_cc))
goto dropafterack;
+ /*
+ * In the SYN-RECEIVED state, validate that the packet belongs to
+ * this connection before trimming the data to fit the receive
+ * window. Check the sequence number versus IRS since we know
+ * the sequence numbers haven't wrapped. This is a partial fix
+ * for the "LAND" DoS attack.
+ */
+ if (tp->t_state == TCPS_SYN_RECEIVED && SEQ_LT(ti->ti_seq, tp->irs))
+ goto dropwithreset;
+
todrop = tp->rcv_nxt - ti->ti_seq;
if (todrop > 0) {
if (tiflags & TH_SYN) {
@@ -1118,40 +1214,6 @@
}
/*
- * If the RST bit is set examine the state:
- * SYN_RECEIVED STATE:
- * If passive open, return to LISTEN state.
- * If active open, inform user that connection was refused.
- * ESTABLISHED, FIN_WAIT_1, FIN_WAIT2, CLOSE_WAIT STATES:
- * Inform user that connection was reset, and close tcb.
- * CLOSING, LAST_ACK, TIME_WAIT STATES
- * Close the tcb.
- */
- if (tiflags&TH_RST) switch (tp->t_state) {
-
- case TCPS_SYN_RECEIVED:
- so->so_error = ECONNREFUSED;
- goto close;
-
- case TCPS_ESTABLISHED:
- case TCPS_FIN_WAIT_1:
- case TCPS_FIN_WAIT_2:
- case TCPS_CLOSE_WAIT:
- so->so_error = ECONNRESET;
- close:
- tp->t_state = TCPS_CLOSED;
- tcpstat.tcps_drops++;
- tp = tcp_close(tp);
- goto drop;
-
- case TCPS_CLOSING:
- case TCPS_LAST_ACK:
- case TCPS_TIME_WAIT:
- tp = tcp_close(tp);
- goto drop;
- }
-
- /*
* If a SYN is in the window, then this is an
* error and we send an RST and drop the connection.
*/
@@ -1660,9 +1722,26 @@
/*
* Generate an ACK dropping incoming segment if it occupies
* sequence space, where the ACK reflects our state.
+ *
+ * We can now skip the test for the RST flag since all
+ * paths to this code happen after packets containing
+ * RST have been dropped.
+ *
+ * In the SYN-RECEIVED state, don't send an ACK unless the
+ * segment we received passes the SYN-RECEIVED ACK test.
+ * If it fails send a RST. This breaks the loop in the
+ * "LAND" DoS attack, and also prevents an ACK storm
+ * between two listening ports that have been sent forged
+ * SYN segments, each with the source address of the other.
*/
- if (tiflags & TH_RST)
- goto drop;
+ if (tp->t_state == TCPS_SYN_RECEIVED) {
+ if ((tiflags & TH_ACK) == 0)
+ goto drop;
+ else if (SEQ_GT(tp->snd_una, ti->ti_ack) ||
+ SEQ_GT(ti->ti_ack, tp->snd_max))
+ goto dropwithreset;
+ /* else fall through */
+ }
#ifdef TCPDEBUG
if (so->so_options & SO_DEBUG)
tcp_trace(TA_DROP, ostate, tp, &tcp_saveti, 0);
--- Truck
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199712020817.AAA10175>