Date: Sat, 19 Aug 2000 15:56:22 +1000 (Australia/NSW) From: Darren Reed <avalon@coombs.anu.edu.au> To: luigi@info.iet.unipi.it (Luigi Rizzo) Cc: freebsd-security@freefall.freebsd.org Subject: Re: [avalon@COOMBS.ANU.EDU.AU: Ip packet filtering with bridging on Message-ID: <200008190556.PAA16336@cairo.anu.edu.au> In-Reply-To: <200008182048.WAA09758@info.iet.unipi.it> from "Luigi Rizzo" at Aug 18, 2000 10:48:05 PM
next in thread | previous in thread | raw e-mail | index | archive | help
In some mail from Luigi Rizzo, sie said:
>
> I was informed by a few people of a thread on alleged problems with
> ipfw+bridging, so i think i should say a few things on the subject.
>
> Darren was complaining that net/bridge.c was missing some sanity
> checks on packets before passing them to ip_fw_chk(). I looked at
> his proposed fix on -security archives (i am not subscribed to the
> list, this is why i did not react).
>
> I am not sure which version of FreeBSD Darren refers to -- the
What's in -current.
> missing checks were there when i committed the code to 3.x and 4.x
> -- only thing, they are|were located in /sys/netinet/ip_fw.c,
> function ip_fw_chk() near this section of code:
[...]
Huh ? Can you point to *exact* versions of the relevant files ?
I'm looking at /sys/net/bridge.c (1.23) and I see:
if (ntohs(eh->ether_type) != ETHERTYPE_IP)
goto forward ; /* not an IP packet, ipfw is not appropriate */
/*
* In this section, canfree=1 means m is the same as *m0.
* canfree==0 means m is a copy. We need to make a copy here
* (to be destroyed on exit from the firewall section) because
* the firewall itself might destroy the packet.
* (This is not very smart... i should really change ipfw to
* leave the pkt alive!)
*/
if (canfree == 0 ) {
/*
* Need to make a copy (and for good measure, make sure that
* the header is contiguous). The original is still in *m0
*/
int needed = min(MHLEN, max_protohdr) ;
needed = min(needed, (*m0)->m_len ) ;
m = m_copypacket( (*m0), M_DONTWAIT);
if (m == NULL) {
printf("-- bdg: m_copypacket failed.\n") ;
return ENOBUFS ;
}
if (m->m_len < needed && (m = m_pullup(m, needed)) == NULL) {
printf("-- bdg: pullup failed.\n") ;
return ENOBUFS ;
}
}
Which is *NOT* sufficient.
Version 1.138 of ip_fw.c does not have "ETHERTYPE_IP" anywhere in it.
Version 1.131.2.1 (FreeBSD 4.0) has this in ip_fw.c:
case ETHERTYPE_IP :
if ((*m)->m_len<sizeof(struct ether_header) + sizeof(struct ip))
goto non_ip ;
ip = (struct ip *)(eh + 1 );
if (ip->ip_v != IPVERSION)
goto non_ip ;
hlen = ip->ip_hl << 2;
if (hlen < sizeof(struct ip)) /* minimum header length */
goto non_ip ;
if ((*m)->m_len < 14 + hlen + 14) {
printf("-- m_len %d, need more...\n", (*m)->m_len);
goto non_ip ;
}
offset = (ip->ip_off & IP_OFFMASK);
break ;
default :
non_ip: ip = NULL ;
break ;
}
Which almost does the right thing. It will cause some packets to be
dropped when they shouldn't (what is "14 + hlen + 14" ?). Having looked
at other ipfw code, it would *appear* this is a cheap way of saying
"sizeof(struct ether_header) + hlen + some_magic_bytes", where
some_magic_bytes is hopefully enough to get all the important TCP fields.
Furthermore, ip_len is never checked ... it's unclear if this poses a
threat to icmp_error()/tcp_respond(). Luckily the "state" code for ipfw
has not reached a level of sophistication where it examines the ICMP
payload as does IP Filter.
Darren
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-security" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200008190556.PAA16336>