Re: ERRATA Re: [PATCH] fix for netfilter/nat/pppoe crashes (hopefully)

[Rusty Russell <rusty@xxxxxxxxxxxxxxx>]

In message <20010802070445.A11923@xxxxxxxxxxxxx> you write:
> > The bug is elsewhere.  
> 
> Nope. The bug is in netfilter.

Yep...

> > So, we will always have 8 protocol bytes in the inner packet.  This is
> > enough to contain the source and destinations ports (TCP/UDP) or ICMP
> > id, so we're not writing over the end of the packet...
> 
> Wrong, ipv4/netfilter/ip_nat_proto_tcp.c:tcp_manip_pkt() *is* writing 
> over the end of the packet when setting tcp->check in 8 byte inner packets
> without checking length.

Argh... there *is* a check in ip_conntrack_proto_tcp, but it checks
against tcph->doff, not sizeof(struct tcphdr)!

This patch strengthens the check, and for neatness, moves it to
pkt_to_tuple where it belongs.  It also adds my previously minor fix.
If this solves the problem, I will move the iph sanity checks to the
entry to the conntrack & NAT code, so we can set skb->h.raw and not
recalculate it everywhere...

Well done Marc!
Rusty.
--
Premature optmztion is rt of all evl. --DK

diff -urN -I \$.*\$ --exclude TAGS -X 
/home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal 
linux-2.4.7-official/net/ipv4/netfilter/ip_conntrack_core.c 
working-2.4.7-marc/net/ipv4/netfilter/ip_conntrack_core.c
--- linux-2.4.7-official/net/ipv4/netfilter/ip_conntrack_core.c Sat Apr 28 
07:15:01 2001
+++ working-2.4.7-marc/net/ipv4/netfilter/ip_conntrack_core.c   Fri Aug  3 
13:29:48 2001
@@ -356,7 +356,7 @@
        inner = (struct iphdr *)(hdr + 1);
        datalen = skb->len - iph->ihl*4 - sizeof(*hdr);
 
-       if (skb->len < iph->ihl * 4 + sizeof(struct icmphdr)) {
+       if (skb->len < iph->ihl * 4 + sizeof(*hdr) + sizeof(*iph)) {
                DEBUGP("icmp_error_track: too short\n");
                return NULL;
        }
diff -urN -I \$.*\$ --exclude TAGS -X 
/home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal 
linux-2.4.7-official/net/ipv4/netfilter/ip_conntrack_proto_tcp.c 
working-2.4.7-marc/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
--- linux-2.4.7-official/net/ipv4/netfilter/ip_conntrack_proto_tcp.c    Sat Apr 
28 07:15:01 2001
+++ working-2.4.7-marc/net/ipv4/netfilter/ip_conntrack_proto_tcp.c      Fri Aug 
 3 13:28:50 2001
@@ -98,6 +98,10 @@
 {
        const struct tcphdr *hdr = datah;
 
+       /* We know we have 8 bytes, but we need whole TCP header */
+       if (datalen < sizeof(*hdr) || datalen < hdr->doff * 4)
+               return 0;
+
        tuple->src.u.tcp.port = hdr->source;
        tuple->dst.u.tcp.port = hdr->dest;
 
@@ -150,13 +154,6 @@
 {
        enum tcp_conntrack newconntrack, oldtcpstate;
        struct tcphdr *tcph = (struct tcphdr *)((u_int32_t *)iph + iph->ihl);
-
-       /* We're guaranteed to have the base header, but maybe not the
-           options. */
-       if (len < (iph->ihl + tcph->doff) * 4) {
-               DEBUGP("ip_conntrack_tcp: Truncated packet.\n");
-               return -1;
-       }
 
        WRITE_LOCK(&tcp_lock);
        oldtcpstate = conntrack->proto.tcp.state;