Date: Wed, 20 Dec 2000 02:46:21 -0800 From: Don Lewis <Don.Lewis@tsc.tdk.com> To: Jesper Skriver <jesper@skriver.dk>, Kris Kennaway <kris@FreeBSD.ORG>, Poul-Henning Kamp <phk@critter.freebsd.dk> Cc: security-officer@FreeBSD.ORG, cvs-all@FreeBSD.ORG, freebsd-net@FreeBSD.ORG Subject: Re: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c tcp_subr.c tcp_var.h Message-ID: <200012201046.CAA19456@salsa.gv.tsc.tdk.com> In-Reply-To: <20001219222730.A29741@skriver.dk> References: <20001218182600.C1856@skriver.dk> <20001219222730.A29741@skriver.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 19, 10:27pm, Jesper Skriver wrote:
} Subject: Re: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c
}
} --17pEHd4RhPHOinZp
} Content-Type: text/plain; charset=us-ascii
} Content-Disposition: inline
}
} On Mon, Dec 18, 2000 at 06:26:00PM +0100, Jesper Skriver wrote:
} > 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
}
} I've got little response to my previous mail, so here is a new diff
} (relative to -current), which I suggest to commit, I think this solves
} all the issues people has brought up.
}
} It has the following functionality.
}
} - If the sysctl net.inet.tcp.icmp_admin_prohib_like_rst == 1 (default)
} it enables the below.
} - When a ICMP administrative prohibited is recieved, it check is the
} IP header attached to the ICMP packet has any options set, if it has
} it ignores it. The reason for this is, if any options is set the extra
} 8 bytes is no longer the first 8 bytes from the TCP header, source/
} destination ports and sequence number, which we need to find the
} right TCP session.
According to Stevens, we should get the first 8 bytes of the TCP header
even if there are options on the ICMP packet. We would have to be
careful to do sanity checking in this case, as well as guard against
unaligned accesses to the TCP header data.
} - Then it goes through the list of active TCP sessions, if it finds one
} with the same source/destination IP addresses and TCP port numbers, it
} checks if the sequence number it got from the ICMP packet is one of
} a unacknowledged packet from this sessions, if it's not, it ignores it.
} - If the sysctl net.inet.tcp.icmp_like_rst_syn_sent_only == 1 (default)
} it will only zap connections in SYN-SENT state, if it's == 0 it will
} zap the connection regardless of current state.
}
} In a addition to this, I've done some cleanup compared to the diff I
} posted on sunday.
}
} This is also submitted as PR kern/23655
}
} Someome please review this.
}
} /Jesper
}
} --
} Jesper Skriver, jesper(at)skriver(dot)dk - CCIE #5456
} Work: Network manager @ AS3292 (Tele Danmark DataNetworks)
} Private: Geek @ AS2109 (A much smaller network ;-)
}
} One Unix to rule them all, One Resolver to find them,
} One IP to bring them all and in the zone to bind them.
}
} --17pEHd4RhPHOinZp
} Content-Type: text/plain; charset=us-ascii
} Content-Description: tcp_drop_icmp_unreach2.diff
} Content-Disposition: attachment; filename="tcp_drop_icmp_unreach2.diff"
}
} diff -ru sys/netinet.old/in_pcb.c sys/netinet/in_pcb.c
} --- sys/netinet.old/in_pcb.c Sun Dec 17 18:57:24 2000
} +++ sys/netinet/in_pcb.c Tue Dec 19 21:18:58 2000
} @@ -667,13 +667,14 @@
} * any errors for each matching socket.
} */
} void
} -in_pcbnotify(head, dst, fport_arg, laddr, lport_arg, cmd, notify)
} +in_pcbnotify(head, dst, fport_arg, laddr, lport_arg, cmd, notify, tcp_sequence)
} struct inpcbhead *head;
} struct sockaddr *dst;
} u_int fport_arg, lport_arg;
} struct in_addr laddr;
} int cmd;
} void (*notify) __P((struct inpcb *, int));
} + u_int32_t tcp_sequence;
} {
} register struct inpcb *inp, *oinp;
} struct in_addr faddr;
} @@ -714,6 +715,15 @@
} (lport && inp->inp_lport != lport) ||
} (laddr.s_addr && inp->inp_laddr.s_addr != laddr.s_addr) ||
} (fport && inp->inp_fport != fport)) {
} + inp = inp->inp_list.le_next;
} + continue;
Wouldn't it be more cleaner (gets rid of the loop) and more efficient (if
we're getting blasted with ICMP messages) to use in_pcblookup_hash()?
} + }
} + /*
} + * If tcp_sequence is set, then skip sessions where
} + * the sequence number is not one of a unacknowledged
} + * packet.
} + */
} + if ((tcp_sequence) && (tcp_seq_vs_sess(inp, tcp_sequence) == 0)) {
} inp = inp->inp_list.le_next;
} continue;
We should pass in an extra flag to indicate if tcp_sequence is valid, since
it can legally be zero. We should also bail out if the sequence check fails,
since it isn't possible for there to be another connection with the same
src/srcport/dst/dstport, so there is no sense in continuing the search.
} }
} diff -ru sys/netinet.old/in_pcb.h sys/netinet/in_pcb.h
} --- sys/netinet.old/in_pcb.h Sun Dec 17 18:57:24 2000
} +++ sys/netinet/in_pcb.h Sun Dec 17 22:47:39 2000
} @@ -290,7 +290,7 @@
} struct in_addr, u_int, struct in_addr, u_int,
} int, struct ifnet *));
} void in_pcbnotify __P((struct inpcbhead *, struct sockaddr *,
} - u_int, struct in_addr, u_int, int, void (*)(struct inpcb *, int)));
} + u_int, struct in_addr, u_int, int, void (*)(struct inpcb *, int), u_int32_t));
} void in_pcbrehash __P((struct inpcb *));
} int in_setpeeraddr __P((struct socket *so, struct sockaddr **nam));
} int in_setsockaddr __P((struct socket *so, struct sockaddr **nam));
} diff -ru sys/netinet.old/tcp_subr.c sys/netinet/tcp_subr.c
} --- sys/netinet.old/tcp_subr.c Sun Dec 17 18:57:24 2000
} +++ sys/netinet/tcp_subr.c Tue Dec 19 21:18:00 2000
} @@ -139,9 +139,20 @@
} * as required by rfc1122 section 3.2.2.1
} */
}
} -static int icmp_admin_prohib_like_rst = 0;
} +static int icmp_admin_prohib_like_rst = 1;
} SYSCTL_INT(_net_inet_tcp, OID_AUTO, icmp_admin_prohib_like_rst, CTLFLAG_RW,
} - &icmp_admin_prohib_like_rst, 0, "Treat ICMP administratively prohibited messages like TCP RST, rfc1122 section 3.2.2.1");
} + &icmp_admin_prohib_like_rst, 0,
} + "Treat ICMP administratively prohibited messages like TCP RST, rfc1122 section 3.2.2.1");
} +
} +/*
} + * When icmp_admin_prohib_like_rst is enabled, only act on
} + * sessions in SYN-SENT state
} + */
} +
} +static int icmp_like_rst_syn_sent_only = 1;
} +SYSCTL_INT(_net_inet_tcp, OID_AUTO, icmp_like_rst_syn_sent_only, CTLFLAG_RW,
} + &icmp_like_rst_syn_sent_only, 0,
} + "When icmp_admin_prohib_like_rst is enabled, only act on sessions in SYN-SENT state");
}
} static void tcp_cleartaocache __P((void));
} static void tcp_notify __P((struct inpcb *, int));
} @@ -967,10 +978,19 @@
} register struct ip *ip = vip;
} register struct tcphdr *th;
} void (*notify) __P((struct inpcb *, int)) = tcp_notify;
} + tcp_seq tcp_sequence = 0;
}
} if (cmd == PRC_QUENCH)
} notify = tcp_quench;
} - else if ((icmp_admin_prohib_like_rst == 1) && (cmd == PRC_UNREACH_PORT) && (ip))
} + else if ((icmp_admin_prohib_like_rst == 1) && (cmd == PRC_UNREACH_PORT) &&
} + (ip) && ((IP_VHL_HL(ip->ip_vhl) << 2) == sizeof(struct ip)))
} + /*
} + * Only go here if the length of the IP header in the ICMP packet
} + * is 20 bytes, that is it doesn't have options, if it does have
} + * options, we will not have the first 8 bytes of the TCP header,
} + * and thus we cannot match against TCP source/destination port
} + * numbers and TCP sequence number.
} + */
} notify = tcp_drop_syn_sent;
} else if (cmd == PRC_MSGSIZE)
} notify = tcp_mtudisc;
} @@ -980,10 +1000,12 @@
} if (ip) {
} th = (struct tcphdr *)((caddr_t)ip
} + (IP_VHL_HL(ip->ip_vhl) << 2));
} + if (notify == tcp_drop_syn_sent)
} + tcp_sequence = ntohl(th->th_seq);
} in_pcbnotify(&tcb, sa, th->th_dport, ip->ip_src, th->th_sport,
} - cmd, notify);
} + cmd, notify, tcp_sequence);
} } else
} - in_pcbnotify(&tcb, sa, 0, zeroin_addr, 0, cmd, notify);
} + in_pcbnotify(&tcb, sa, 0, zeroin_addr, 0, cmd, notify, 0);
} }
}
} #ifdef INET6
} @@ -1070,6 +1092,30 @@
} #endif /* INET6 */
}
} /*
} + * Check if the supplied TCP sequence number is a sequence number
} + * for a sent but unacknowledged packet on the given TCP session.
} + */
} +int
} +tcp_seq_vs_sess(inp, tcp_sequence)
} + struct inpcb *inp;
} + tcp_seq tcp_sequence;
} +{
} + struct tcpcb *tp = intotcpcb(inp);
} + /*
} + * If the sequence number is less than that of the last
} + * unacknowledged packet, or greater than that of the
} + * last sent, the given sequence number is not that
} + * of a sent but unacknowledged packet for this session.
} + */
} + if (SEQ_LT(tcp_sequence, tp->snd_una) ||
} + SEQ_GT(tcp_sequence, tp->snd_max)) {
} + return(0);
} + } else {
} + return(1);
} + }
} +}
} +
} +/*
} * When a source quench is received, close congestion window
} * to one segment. We will gradually open it again as we proceed.
} */
} @@ -1086,7 +1132,9 @@
}
} /*
} * When a ICMP unreachable is recieved, drop the
} - * TCP connection, but only if in SYN_SENT
} + * TCP connection, depending on the sysctl
} + * icmp_like_rst_syn_sent_only, it only drops
} + * the session if it's in SYN-SENT state
} */
} void
} tcp_drop_syn_sent(inp, errno)
} @@ -1094,8 +1142,9 @@
} int errno;
} {
} struct tcpcb *tp = intotcpcb(inp);
} - if((tp) && (tp->t_state == TCPS_SYN_SENT))
} - tcp_drop(tp, errno);
} + if((tp) && ((icmp_like_rst_syn_sent_only == 0) ||
} + (tp->t_state == TCPS_SYN_SENT)))
} + tcp_drop(tp, errno);
} }
}
} /*
} diff -ru sys/netinet.old/tcp_var.h sys/netinet/tcp_var.h
} --- sys/netinet.old/tcp_var.h Sun Dec 17 18:57:24 2000
} +++ sys/netinet/tcp_var.h Tue Dec 19 20:16:54 2000
} @@ -392,6 +392,7 @@
} struct tcpcb *
} tcp_newtcpcb __P((struct inpcb *));
} int tcp_output __P((struct tcpcb *));
} +int tcp_seq_vs_sess __P((struct inpcb *, tcp_seq));
} void tcp_quench __P((struct inpcb *, int));
} void tcp_respond __P((struct tcpcb *, void *,
} struct tcphdr *, struct mbuf *, tcp_seq, tcp_seq, int));
} diff -ru sys/netinet.old/udp_usrreq.c sys/netinet/udp_usrreq.c
} --- sys/netinet.old/udp_usrreq.c Sun Dec 17 18:57:24 2000
} +++ sys/netinet/udp_usrreq.c Sun Dec 17 19:59:53 2000
} @@ -512,9 +512,9 @@
} if (ip) {
} uh = (struct udphdr *)((caddr_t)ip + (ip->ip_hl << 2));
} in_pcbnotify(&udb, sa, uh->uh_dport, ip->ip_src, uh->uh_sport,
} - cmd, udp_notify);
} + cmd, udp_notify, 0);
} } else
} - in_pcbnotify(&udb, sa, 0, zeroin_addr, 0, cmd, udp_notify);
} + in_pcbnotify(&udb, sa, 0, zeroin_addr, 0, cmd, udp_notify, 0);
} }
}
} static int
}
} --17pEHd4RhPHOinZp--
}
}
} To Unsubscribe: send mail to majordomo@FreeBSD.org
} with "unsubscribe freebsd-net" in the body of the message
}-- End of excerpt from Jesper Skriver
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200012201046.CAA19456>