Date: Sat, 23 Feb 2002 13:50:33 +0200 From: Ruslan Ermilov <ru@FreeBSD.org> To: "Crist J. Clark" <cjc@FreeBSD.org> Cc: net@FreeBSD.org Subject: Re: TCP Connections to a Broadcast Address Message-ID: <20020223115033.GB47437@sunbay.com> In-Reply-To: <20020222022626.A83807@blossom.cjclark.org> References: <20020222022626.A83807@blossom.cjclark.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Feb 22, 2002 at 02:26:26AM -0800, Crist J. Clark wrote:
> BSD-based TCP/IP code have a bug with respect to creating TCP
> connections to a broadcast address. This bug can potentially be a
> security vulnerability when firewall administrators assume that the
> TCP implementation works correctly and does not block broadcast
> addresses.
>
>
> The Standard:
>
> TCP connections are only valid when the destination address is a
> unicast address. That is, the destination must not be a multicast or
> broadcast address. One place this is clearly specified in the
> Standards is RFC 1122 (everyone's very most favorite RFC),
>
> 4.2.3.10 Remote Address Validation
>
> ...
>
> A TCP implementation MUST silently discard an incoming SYN
> segment that is addressed to a broadcast or multicast
> address.
>
>
> The Bug:
>
> Uncorrected BSD-based TCP implementations do not actually check if the
> destination IP address is a broadcast address. Rather, the packet's
> link layer address is checked. Here is the code from FreeBSD's
> tcp_input.c,
>
> * RFC1122 4.2.3.10, p. 104: discard bcast/mcast SYN
> * in_broadcast() should never return true on a received
> * packet with M_BCAST not set.
> *
> * Packets with a multicast source address should also
> * be discarded.
> */
> if (m->m_flags & (M_BCAST|M_MCAST))
> goto drop;
> #ifdef INET6
> if (isipv6) {
> if (IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst) ||
> IN6_IS_ADDR_MULTICAST(&ip6->ip6_src))
> goto drop;
> } else
> #endif
> if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) ||
> IN_MULTICAST(ntohl(ip->ip_src.s_addr)) ||
> ip->ip_src.s_addr == htonl(INADDR_BROADCAST))
> goto drop;
>
> The comment in the code reveals the reason for the mistake. The
> authors assume that no one would accidentally or maliciously break the
> rules. One can easily send packets with a unicast link-layer address,
> but containing an IP broadcast address. No check is made in the above
> code for such a pathological situation.
>
Nice catch!
> Adding an in_broadcast() check trivially fixes the problem. Here is a
> patch (which fixes the comment too),
>
> Index: src/sys/netinet/tcp_input.c
> ===================================================================
> RCS file: /export/ncvs/src/sys/netinet/tcp_input.c,v
> retrieving revision 1.146
> diff -u -r1.146 tcp_input.c
> --- src/sys/netinet/tcp_input.c 4 Jan 2002 17:21:27 -0000 1.146
> +++ src/sys/netinet/tcp_input.c 17 Feb 2002 12:54:39 -0000
> @@ -798,11 +798,10 @@
> }
> /*
> * RFC1122 4.2.3.10, p. 104: discard bcast/mcast SYN
> - * in_broadcast() should never return true on a received
> - * packet with M_BCAST not set.
> - *
> - * Packets with a multicast source address should also
> - * be discarded.
> + *
> + * It is possible for a malicious (or misconfigured)
> + * attacker to send unicast link-layer packets with a
> + * broadcast IP address. Use in_broadcast() to find them.
> */
> if (m->m_flags & (M_BCAST|M_MCAST))
> goto drop;
> @@ -815,7 +814,8 @@
> #endif
> if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) ||
> IN_MULTICAST(ntohl(ip->ip_src.s_addr)) ||
> - ip->ip_src.s_addr == htonl(INADDR_BROADCAST))
> + ip->ip_src.s_addr == htonl(INADDR_BROADCAST) ||
> + in_broadcast(ip->ip_dst, m->m_pkthdr.rcvif))
> goto drop;
> /*
> * SYN appears to be valid; create compressed TCP state
>
The patch is incomplete (see dropwithreset below). Here's the tcp_input.c
part of the original delta that introduced this bug:
: Script started on Sat Feb 23 13:37:18 2002
: $ sccs prs -r7.35 tcp_input.c
: D 7.35 93/04/07 19:28:08 sklower 159 158 00007/00003/01623
: MRs:
: COMMENTS:
: Mostly changes recommended by jch for variable subnets & multiple
: IP addresses per physical interface. May require further work.
:
: $ sccs sccsdiff -up -r7.34 -r7.35 tcp_input.c
: SCCS/s.tcp_input.c: 7.34 vs. 7.35
: --- /tmp/get.19874.7.34 Sat Feb 23 13:37:41 2002
: +++ /tmp/get.19874.7.35 Sat Feb 23 13:37:41 2002
: @@ -518,9 +518,13 @@ findpcb:
: goto dropwithreset;
: if ((tiflags & TH_SYN) == 0)
: goto drop;
: - /* RFC1122 4.2.3.10, p. 104: discard bcast/mcast SYN */
: + /*
: + * RFC1122 4.2.3.10, p. 104: discard bcast/mcast SYN
: + * in_broadcast() should never return true on a received
: + * packet with M_BCAST not set.
: + */
: if (m->m_flags & (M_BCAST|M_MCAST) ||
: - in_broadcast(ti->ti_dst) || IN_MULTICAST(ti->ti_dst.s_addr))
: + IN_MULTICAST(ti->ti_dst.s_addr))
: goto drop;
: am = m_get(M_DONTWAIT, MT_SONAME); /* XXX */
: if (am == NULL)
: @@ -1271,7 +1275,7 @@ dropwithreset:
: * Don't bother to respond if destination was broadcast/multicast.
: */
: if ((tiflags & TH_RST) || m->m_flags & (M_BCAST|M_MCAST) ||
: - in_broadcast(ti->ti_dst) || IN_MULTICAST(ti->ti_dst.s_addr))
: + IN_MULTICAST(ti->ti_dst.s_addr))
: goto drop;
: if (tiflags & TH_ACK)
: tcp_respond(tp, ti, m, (tcp_seq)0, ti->ti_ack, TH_RST);
: $
: Script done on Sat Feb 23 13:37:43 2002
I think you should just back the CSRG revision 7.35 out of tcp_input.c,
mentioning what was wrong with removing in_broadcast() check.
route add -net 192.168.4 192.168.1.1
ping 192.168.4.255
on a directly attached 192.168.1 network isn't a "malicious use".
Cheers,
--
Ruslan Ermilov Sysadmin and DBA,
ru@sunbay.com Sunbay Software AG,
ru@FreeBSD.org FreeBSD committer,
+380.652.512.251 Simferopol, Ukraine
http://www.FreeBSD.org The Power To Serve
http://www.oracle.com Enabling The Information Age
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-net" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020223115033.GB47437>