From owner-freebsd-net Tue Dec 19 6:26: 4 2000 From owner-freebsd-net@FreeBSD.ORG Tue Dec 19 06:25:57 2000 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from gatekeeper.tsc.tdk.com (gatekeeper.tsc.tdk.com [207.113.159.21]) by hub.freebsd.org (Postfix) with ESMTP id 6CC3B37B404; Tue, 19 Dec 2000 06:25:57 -0800 (PST) Received: from imap.gv.tsc.tdk.com (imap.gv.tsc.tdk.com [192.168.241.198]) by gatekeeper.tsc.tdk.com (8.8.8/8.8.8) with ESMTP id GAA05841; Tue, 19 Dec 2000 06:25:48 -0800 (PST) (envelope-from gdonl@tsc.tdk.com) Received: from salsa.gv.tsc.tdk.com (salsa.gv.tsc.tdk.com [192.168.241.194]) by imap.gv.tsc.tdk.com (8.9.3/8.9.3) with ESMTP id GAA87896; Tue, 19 Dec 2000 06:25:47 -0800 (PST) (envelope-from Don.Lewis@tsc.tdk.com) Received: (from gdonl@localhost) by salsa.gv.tsc.tdk.com (8.8.5/8.8.5) id GAA14731; Tue, 19 Dec 2000 06:25:46 -0800 (PST) From: Don Lewis Message-Id: <200012191425.GAA14731@salsa.gv.tsc.tdk.com> Date: Tue, 19 Dec 2000 06:25:46 -0800 In-Reply-To: <20001218182600.C1856@skriver.dk> References: <20001218182600.C1856@skriver.dk> X-Mailer: Mail User's Shell (7.2.6 beta(5) 10/07/98) To: Jesper Skriver , Kris Kennaway , Poul-Henning Kamp Subject: Re: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c tcp_subr.c tcp_var.h Cc: security-officer@FreeBSD.ORG, cvs-all@FreeBSD.ORG, freebsd-net@FreeBSD.ORG Sender: gdonl@tsc.tdk.com Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Dec 18, 6:26pm, Jesper Skriver wrote: } Subject: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c tcp_ } Hi, } } I'm trying to find out what to to now regarding this. } } To summarize. } } PHK committed my original patch, this patch have the following } functionality } - When a ICMP administrative prohibited is recieved, it zap's } all TCP sessions in SYN-SENT state matching the source and destination } IP addresses and TCP port numbers in the IP header + 8 bytes from the } ICMP packet. } - It does not match against TCP sequence number } - disabled by default } } Yesterday I summitted a new diff, with the following changes to the } above. } } - Matches against the TCP sequence number in the IP header + 8 bytes } from the ICMP packet, against the last unacknowledged packet in the } TCP session matching the source and destination IP addresses and } TCP port numbers, these must be equal, thus it only matches if the } ICMP unreachable is for the last sent packet. } This is very secure, but in reality only has effect when setting up } the session, as it doesn't work with multiple outstanding packets, } it does work when setting up sessions, as the window will be zero } here. } this could be fixed by something like (*) } - Check for SYN-SENT state removed } - enabled by default } } What I will suggest at this point, is to do one of 2 things: } } 1) Extend the original diff PHK committed to check for sequence number, } and enable it by default, trivial as it's part of the second diff. } 2) Fix the second diff with the below code. } } For both I'll also add a extra check if the IP header in the ICMP packet } has options set, and if it has, don't act on it, this applies to both, } the reason for this is, if it has options set, we'll miss some (or all) } of the 8 bytes from the TCP header, and thus, we'll not know port and } sequence numbers. } } What do you prefer ? When I know this, I'll post a new diff for review. } } (*) replace } } if (tp->snd_una != tcp_sequence) { } } with } } /* } * First check: if sequence numbers have wrapped, don't act on this. } * Second -"- : if the sequence number from the ICMP packet is for a } * "old" packet, it's probably spoofed, dont't act on this. } * Third -"- : if the sequence number from the ICMP packet is for a } * packet from the future, it's spoofed, don't act on this. } */ } if ((tp->snd_max < tp->snd_una) || (tcp_sequence < tp->snd_una) || \ } (tp->snd_max < tcp_sequence)) { The sequence number comparisons should use the SEQ_xx() macros, which handle sequence number wrapping. The sanity checks should probably be the same as incoming RST validation, see the code in tcp_input() for the code and matching comments. In the SYN-SENT state, this would translate to: if (SEQ_LEQ(tcp_sequence, tp->iss) || SEQ_GT(tcp_sequence, tp->snd_max)) { /* ignore the icmp */ In the other states, RFC 793 says that the RST sanity checking is done by comparing the sequence number of the of the incoming RST packet against the transmit window (our outgoing acknowledgement numbers). The host sending the RST is supposed to copy the acknowledgement number from an incoming packet to the sequence number of the outgoing RST packet. This presents a bit of a problem if we try to do the same thing with ICMP, since it appears that the acknowledgement number is trimmed off the the data that is returned in the ICMP packet. It's been too long a day for me to figure out the security implications of nuking non-SYN-SENT connections based on the sequence number (which would still be better than nuking these connections without any additional checking). If we want to do this, the test should probably be: if (SEQ_LEQ(tcp_sequence, tp->snd_una) || SEQ_GT(tcp_sequence, tp->snd_max)) { though someone needs to check this for fencepost errors. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message