Re: Iptables ipt_unclean bug?

Rusty Russell
2001-06-19T09:29:11Z

In message <Pine.LNX.4.21.0106190104020.414-100000@firewall.sch.bme.hu> you wri
te:
>
> Hi all!
>
> I think it's possible to hang the kernel useing isic 0.05
> (www.packetfactory.net/Projects/ISIC/), when there's a unclean match in
> iptables rules.

Thanks for the bug report. I've just done an audit of the unclean
code: patch against 2.4.5 is below. There were some bad thinkos
there, two fatal.

It'd be nice if someone else audits it, as I wrote it in the first
place...

Untested, please report back.

Cheers,
Rusty.
--
Premature optmztion is rt of all evl. --DK

--- linux-2.4.5-official/net/ipv4/netfilter/ipt_unclean.c Fri Apr 28 08:43:15 2000
+++ working-2.4.5-hotplug/net/ipv4/netfilter/ipt_unclean.c Tue Jun 19 19:30:32 2001
@@ -76,7 +76,7 @@
= { 12, 12, ICMP_NOT_ERROR, 0, 0 } };

/* Can't do anything if it's a fragment. */
- if (!offset)
+ if (offset)
return 1;

/* Must cover type and code. */
@@ -87,7 +87,7 @@

/* If not embedded. */
if (!embedded) {
- /* Bad checksum? Don't print, just drop. */
+ /* Bad checksum? Don't print, just ignore. */
if (!more_frags
&& ip_compute_csum((unsigned char *) icmph, datalen) != 0)
return 0;
@@ -108,6 +108,8 @@
length of iph + 8 bytes. */
struct iphdr *inner = (void *)icmph + 8;

+ /* datalen > 8 since all ICMP_IS_ERROR types
+ have min length > 8 */
if (datalen - 8 < sizeof(struct iphdr)) {
limpk("ICMP error internal way too short\n");
return 0;
@@ -155,6 +157,8 @@
u_int32_t arg = ntohl(icmph->un.gateway);

if (icmph->code == 0) {
+ /* Code 0 means that upper 8 bits is pointer
+ to problem. */
if ((arg >> 24) >= iph->ihl*4) {
limpk("ICMP PARAMETERPROB ptr = %u\n",
ntohl(icmph->un.gateway) >> 24);
@@ -196,7 +200,7 @@
int embedded)
{
/* Can't do anything if it's a fragment. */
- if (!offset)
+ if (offset)
return 1;

/* CHECK: Must cover UDP header. */
@@ -205,7 +209,7 @@
return 0;
}

- /* Bad checksum? Don't print, just drop. */
+ /* Bad checksum? Don't print, just say it's unclean. */
/* FIXME: SRC ROUTE packets won't match checksum --RR */
if (!more_frags && !embedded
&& csum_tcpudp_magic(iph->saddr, iph->daddr, datalen, IPPROTO_UDP,
@@ -272,7 +276,7 @@
/* In fact, this is caught below (offset < 516). */

/* Can't do anything if it's a fragment. */
- if (!offset)
+ if (offset)
return 1;

/* CHECK: Smaller than minimal TCP hdr. */
@@ -281,7 +285,8 @@
limpk("Packet length %u < TCP header.\n", datalen);
return 0;
}
- /* Must have ports available (datalen >= 8). */
+ /* Must have ports available (datalen >= 8), from
+ check_icmp which set embedded = 1 */
/* CHECK: TCP ports inside ICMP error */
if (!tcph->source || !tcph->dest) {
limpk("Zero TCP ports %u/%u.\n",
@@ -301,7 +306,7 @@
return 1;
}

- /* Bad checksum? Don't print, just drop. */
+ /* Bad checksum? Don't print, just say it's unclean. */
/* FIXME: SRC ROUTE packets won't match checksum --RR */
if (!more_frags && !embedded
&& csum_tcpudp_magic(iph->saddr, iph->daddr, datalen, IPPROTO_TCP,
@@ -373,6 +378,8 @@
(unsigned int) opt[i], i);
return 0;
}
+ /* Move to next option */
+ i += opt[i+1];
}
}

@@ -442,6 +449,8 @@
opt[i], i);
return 0;
}
+ /* Move to next option */
+ i += opt[i+1];
}
}

@@ -495,10 +504,10 @@
return 0;
}

- /* CHECK: Min offset of frag = 128 - 60 (max IP hdr len). */
- if (offset && offset * 8 < MIN_LIKELY_MTU - 60) {
+ /* CHECK: Min offset of frag = 128 - IP hdr len. */
+ if (offset && offset * 8 < MIN_LIKELY_MTU - iph->ihl * 4) {
limpk("Fragment starts at %u < %u\n", offset * 8,
- MIN_LIKELY_MTU-60);
+ MIN_LIKELY_MTU - iph->ihl * 4);
return 0;
}