On Thu, Aug 02, 2001 at 04:00:24PM +1000, Rusty Russell wrote:
> In message <200107312226.CAA00407@xxxxxxxxxxxxxx> you write:
> > Very interesting... Paul, did he really not miss the place where it was
> > really checked?
>
> The bug is elsewhere.
Nope. The bug is in netfilter.
> Looks like Marc forgot that we don't do NAT if
> the conntrack code hasn't already set skb->nfct. 8)
Perhaps; some of the checks added by my patch may be superfluous.
I wanted to be on the safe side while hunting down the bug..
> If the packet gets here, it is an ICMP packet which the connection
> tracking code has marked RELATED.
>
> The only way ICMP packets get marked RELATED is in icmp_error_track.
> To do this, it has to get past ip_conntrack_core.c:357-359:
>
> datalen = skb->len - iph->ihl*4 - sizeof(*hdr);
>
> if (skb->len < iph->ihl * 4 + sizeof(struct icmphdr)) {
> DEBUGP("icmp_error_track: too short\n");
> return NULL;
> }
>
> In my first audit, I noticed that this should strictly be ... +
> sizeof(struct iphdr), but we only read from it.
>
> Anyway, so datalen is always >= 0.
>
> And this is the killer: line 385 (it's redundant: we check this inside
> get_tuple anyway):
>
> /* Are they talking about one of our connections? */
> if (inner->ihl * 4 + 8 > datalen
> || !get_tuple(inner, datalen, &origtuple, innerproto)) {
>
> 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.
Several users have reported on the netfilter mailing list that the crashes have
disappeared after the patch was applied.
> Now, if Marc is seeing this, and noone can find a flaw in my logic,
> then I'd say that someone else is mangling a packet without doing (see
> ipip.c):
>
> nf_conntrack_put(skb->nfct);
> skb->nfct = NULL;
>
> Please find them and hit them hard...
I'll let you take care of that! But please, don't be too harsch on yourself. 8)
Marc
>
> Hope that helps,
> Rusty.
> --
> Premature optmztion is rt of all evl. --DK
>
|