[PATCH] make happy cracking message more verbose

[PATCH] make happy cracking message more verbose

[Maciej Soltysiak]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7303 bytes --]

Hi,

there are places in netfilter where it prints:
ipt_hoop: happy cracking.

Which doesn't explain a thing to non-netfilter hackers.
I decided to make those a bit more verbose to the user.

It writes information what was wrong.
Effects of this patch:
- if the sk_buff->len was to short, print it out
- if header lenght was to short, print it out also
- Write what where the lengths expected.
- Additional debug information as to in which file, which function did
  it occur.
- Add a similar check to ip6 (it was ifdef'd)

I have one question about it.

If we have:

	int len = (*pskb)->nh.iph->ihl * 4;

Is it possible to have a NULL pointer dereference here with (*pskb)->nh
being NULL in:
ip_conntrack_local()
ipt_local_out_hook()
ipt_local_hook()
ip6t_local_out_hook()
ip6t_local_hook()

Should we check for nh, nh.iph first here, or are we okay to rely on
other functions to initialize it correctly, or disallowing for a code path
with these being NULL?


Please comment this patch.

I am also attaching the patch mime'd just in case.

Regards,
Maciej

diff -u linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c linux/net/ipv4/netfilter/ip_conntrack_standalone.c
--- linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 20:36:25.000000000 +0100
+++ linux/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 19:38:48.000000000 +0100
@@ -215,14 +215,35 @@
 				       const struct net_device *out,
 				       int (*okfn)(struct sk_buff *))
 {
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int len = (*pskb)->nh.iph->ihl * 4;
+	int hdrlen = sizeof(struct iphdr);
+	int bad = 0;
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ipt_hook: happy cracking.\n");
-		return NF_ACCEPT;
+			printk("ipt_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
+		bad = 1;
 	}
-	return ip_conntrack_in(hooknum, pskb, in, out, okfn);
+
+	if (len < hdrlen) {
+		if (net_ratelimit())
+			printk("ipt_hook: happy cracking at %s in %s."
+				" IPv4 header too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				len, hdrlen);
+		bad = 1;
+	}
+
+	if (bad == 1)
+		return NF_ACCEPT;
+	else
+		return ip_conntrack_in(hooknum, pskb, in, out, okfn);
 }

 /* Connection tracking may drop packets, but never alters them, so
diff -u linux.orig/net/ipv4/netfilter/iptable_filter.c linux/net/ipv4/netfilter/iptable_filter.c
--- linux.orig/net/ipv4/netfilter/iptable_filter.c	2003-09-08 21:50:01.000000000 +0200
+++ linux/net/ipv4/netfilter/iptable_filter.c	2003-11-06 19:38:57.000000000 +0100
@@ -111,15 +111,35 @@
 		   const struct net_device *out,
 		   int (*okfn)(struct sk_buff *))
 {
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int len = (*pskb)->nh.iph->ihl * 4;
+	int hdrlen = sizeof(struct iphdr);
+	int bad = 0;
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ipt_hook: happy cracking.\n");
-		return NF_ACCEPT;
+			printk("ipt_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
+		bad = 1;
+	}
+
+	if (len < hdrlen) {
+		if (net_ratelimit())
+			printk("ipt_hook: happy cracking at %s in %s."
+				" IPv4 header too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				len, hdrlen);
+		bad = 1;
 	}

-	return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
+	if (bad == 1)
+		return NF_ACCEPT;
+	else
+		return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
 }

 static struct nf_hook_ops ipt_ops[] = {
diff -u linux.orig/net/ipv4/netfilter/iptable_mangle.c linux/net/ipv4/netfilter/iptable_mangle.c
--- linux.orig/net/ipv4/netfilter/iptable_mangle.c	2003-09-08 21:50:22.000000000 +0200
+++ linux/net/ipv4/netfilter/iptable_mangle.c	2003-11-06 19:39:06.000000000 +0100
@@ -149,14 +149,34 @@
 	u_int32_t saddr, daddr;
 	unsigned long nfmark;

+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int len = (*pskb)->nh.iph->ihl * 4;
+	int hdrlen = sizeof(struct iphdr);
+	int bad = 0;
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ipt_hook: happy cracking.\n");
-		return NF_ACCEPT;
+			printk("ipt_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
+		bad = 1;
 	}

+	if (len < hdrlen) {
+		if (net_ratelimit())
+			printk("ipt_hook: happy cracking at %s in %s."
+				" IPv4 header too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				len, hdrlen);
+		bad = 1;
+	}
+
+	if (bad == 1)
+		return NF_ACCEPT;
+
 	/* Save things which could affect route */
 	nfmark = (*pskb)->nfmark;
 	saddr = (*pskb)->nh.iph->saddr;
diff -u linux.orig/net/ipv6/netfilter/ip6table_filter.c linux/net/ipv6/netfilter/ip6table_filter.c
--- linux.orig/net/ipv6/netfilter/ip6table_filter.c	2003-09-08 21:49:52.000000000 +0200
+++ linux/net/ipv6/netfilter/ip6table_filter.c	2003-11-06 19:40:12.000000000 +0100
@@ -111,15 +111,19 @@
 		   const struct net_device *out,
 		   int (*okfn)(struct sk_buff *))
 {
-#if 0
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int hdrlen = sizeof(struct ipv6hdr);
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ip6t_hook: happy cracking.\n");
+			printk("ip6t_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
 		return NF_ACCEPT;
 	}
-#endif

 	return ip6t_do_table(pskb, hook, in, out, &packet_filter, NULL);
 }
diff -u linux.orig/net/ipv6/netfilter/ip6table_mangle.c linux/net/ipv6/netfilter/ip6table_mangle.c
--- linux.orig/net/ipv6/netfilter/ip6table_mangle.c	2003-09-08 21:50:02.000000000 +0200
+++ linux/net/ipv6/netfilter/ip6table_mangle.c	2003-11-06 19:40:17.000000000 +0100
@@ -148,15 +148,19 @@
 	u_int8_t hop_limit;
 	u_int32_t flowlabel;

-#if 0
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int hdrlen = sizeof(struct ipv6hdr);
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ip6t_hook: happy cracking.\n");
+			printk("ip6t_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
 		return NF_ACCEPT;
 	}
-#endif

 	/* FIXME: Push down to extensions --RR */
 	if (skb_is_nonlinear(*pskb) && skb_linearize(*pskb, GFP_ATOMIC) != 0)

[-- Attachment #2: happy_cracking.diff --]
[-- Type: TEXT/plain, Size: 6439 bytes --]

diff -u linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c linux/net/ipv4/netfilter/ip_conntrack_standalone.c
--- linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 20:36:25.000000000 +0100
+++ linux/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 19:38:48.000000000 +0100
@@ -215,14 +215,35 @@
 				       const struct net_device *out,
 				       int (*okfn)(struct sk_buff *))
 {
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int len = (*pskb)->nh.iph->ihl * 4;
+	int hdrlen = sizeof(struct iphdr);
+	int bad = 0;
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ipt_hook: happy cracking.\n");
-		return NF_ACCEPT;
+			printk("ipt_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
+		bad = 1;
 	}
-	return ip_conntrack_in(hooknum, pskb, in, out, okfn);
+
+	if (len < hdrlen) {
+		if (net_ratelimit())
+			printk("ipt_hook: happy cracking at %s in %s."
+				" IPv4 header too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				len, hdrlen);
+		bad = 1;
+	}
+
+	if (bad == 1)
+		return NF_ACCEPT;
+	else
+		return ip_conntrack_in(hooknum, pskb, in, out, okfn);
 }
 
 /* Connection tracking may drop packets, but never alters them, so
diff -u linux.orig/net/ipv4/netfilter/iptable_filter.c linux/net/ipv4/netfilter/iptable_filter.c
--- linux.orig/net/ipv4/netfilter/iptable_filter.c	2003-09-08 21:50:01.000000000 +0200
+++ linux/net/ipv4/netfilter/iptable_filter.c	2003-11-06 19:38:57.000000000 +0100
@@ -111,15 +111,35 @@
 		   const struct net_device *out,
 		   int (*okfn)(struct sk_buff *))
 {
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int len = (*pskb)->nh.iph->ihl * 4;
+	int hdrlen = sizeof(struct iphdr);
+	int bad = 0;
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ipt_hook: happy cracking.\n");
-		return NF_ACCEPT;
+			printk("ipt_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
+		bad = 1;
+	}
+
+	if (len < hdrlen) {
+		if (net_ratelimit())
+			printk("ipt_hook: happy cracking at %s in %s."
+				" IPv4 header too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				len, hdrlen);
+		bad = 1;
 	}
 
-	return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
+	if (bad == 1)
+		return NF_ACCEPT;
+	else
+		return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
 }
 
 static struct nf_hook_ops ipt_ops[] = {
diff -u linux.orig/net/ipv4/netfilter/iptable_mangle.c linux/net/ipv4/netfilter/iptable_mangle.c
--- linux.orig/net/ipv4/netfilter/iptable_mangle.c	2003-09-08 21:50:22.000000000 +0200
+++ linux/net/ipv4/netfilter/iptable_mangle.c	2003-11-06 19:39:06.000000000 +0100
@@ -149,14 +149,34 @@
 	u_int32_t saddr, daddr;
 	unsigned long nfmark;
 
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int len = (*pskb)->nh.iph->ihl * 4;
+	int hdrlen = sizeof(struct iphdr);
+	int bad = 0;
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ipt_hook: happy cracking.\n");
-		return NF_ACCEPT;
+			printk("ipt_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
+		bad = 1;
 	}
 
+	if (len < hdrlen) {
+		if (net_ratelimit())
+			printk("ipt_hook: happy cracking at %s in %s."
+				" IPv4 header too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				len, hdrlen);
+		bad = 1;
+	}
+
+	if (bad == 1)
+		return NF_ACCEPT;
+
 	/* Save things which could affect route */
 	nfmark = (*pskb)->nfmark;
 	saddr = (*pskb)->nh.iph->saddr;
diff -u linux.orig/net/ipv6/netfilter/ip6table_filter.c linux/net/ipv6/netfilter/ip6table_filter.c
--- linux.orig/net/ipv6/netfilter/ip6table_filter.c	2003-09-08 21:49:52.000000000 +0200
+++ linux/net/ipv6/netfilter/ip6table_filter.c	2003-11-06 19:40:12.000000000 +0100
@@ -111,15 +111,19 @@
 		   const struct net_device *out,
 		   int (*okfn)(struct sk_buff *))
 {
-#if 0
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int hdrlen = sizeof(struct ipv6hdr);
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ip6t_hook: happy cracking.\n");
+			printk("ip6t_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
 		return NF_ACCEPT;
 	}
-#endif
 
 	return ip6t_do_table(pskb, hook, in, out, &packet_filter, NULL);
 }
diff -u linux.orig/net/ipv6/netfilter/ip6table_mangle.c linux/net/ipv6/netfilter/ip6table_mangle.c
--- linux.orig/net/ipv6/netfilter/ip6table_mangle.c	2003-09-08 21:50:02.000000000 +0200
+++ linux/net/ipv6/netfilter/ip6table_mangle.c	2003-11-06 19:40:17.000000000 +0100
@@ -148,15 +148,19 @@
 	u_int8_t hop_limit;
 	u_int32_t flowlabel;
 
-#if 0
+	/* More verbosity to happy cracking by Maciej Soltysiak
+	 * <solt@dns.toxicfilms.tv> */
+	int hdrlen = sizeof(struct ipv6hdr);
+
 	/* root is playing with raw sockets. */
-	if ((*pskb)->len < sizeof(struct iphdr)
-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+	if ((*pskb)->len < hdrlen) {
 		if (net_ratelimit())
-			printk("ip6t_hook: happy cracking.\n");
+			printk("ip6t_hook: happy cracking at %s in %s."
+				" sk_buff too short: %u. Expected: %u\n",
+				__FUNCTION__, __FILE__,
+				(*pskb)->len, hdrlen);
 		return NF_ACCEPT;
 	}
-#endif
 
 	/* FIXME: Push down to extensions --RR */
 	if (skb_is_nonlinear(*pskb) && skb_linearize(*pskb, GFP_ATOMIC) != 0)

Re: [PATCH] make happy cracking message more verbose

[Patrick McHardy]

Hi Maciej,
I made a similar change today to give me the name
of the offending process after receiving these messages
for 8 days. It turned out to be an unrelated bug, located
in skb_copy_expand. Luckily this message gave me the
right pointer, but I don't see the point in totally
bloating the code just for some information that is at
best of limited use in debugging the problem. If anything,
we should print the name of the process producing the
packets, or "kernel" if they are produced out of process
context. Still I don't see the point in iptables trying
to perform IDS actions, in my opinion a counter should
be incremented and nothing should be printed at all.

Best regards,
Patrick

Maciej Soltysiak wrote:

>Hi,
>
>there are places in netfilter where it prints:
>ipt_hoop: happy cracking.
>
>Which doesn't explain a thing to non-netfilter hackers.
>I decided to make those a bit more verbose to the user.
>
>It writes information what was wrong.
>Effects of this patch:
>- if the sk_buff->len was to short, print it out
>- if header lenght was to short, print it out also
>- Write what where the lengths expected.
>- Additional debug information as to in which file, which function did
>  it occur.
>- Add a similar check to ip6 (it was ifdef'd)
>
>I have one question about it.
>
>If we have:
>
>	int len = (*pskb)->nh.iph->ihl * 4;
>
>Is it possible to have a NULL pointer dereference here with (*pskb)->nh
>being NULL in:
>ip_conntrack_local()
>ipt_local_out_hook()
>ipt_local_hook()
>ip6t_local_out_hook()
>ip6t_local_hook()
>
>Should we check for nh, nh.iph first here, or are we okay to rely on
>other functions to initialize it correctly, or disallowing for a code path
>with these being NULL?
>
>
>Please comment this patch.
>
>I am also attaching the patch mime'd just in case.
>
>Regards,
>Maciej
>
>diff -u linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c linux/net/ipv4/netfilter/ip_conntrack_standalone.c
>--- linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 20:36:25.000000000 +0100
>+++ linux/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 19:38:48.000000000 +0100
>@@ -215,14 +215,35 @@
> 				       const struct net_device *out,
> 				       int (*okfn)(struct sk_buff *))
> {
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int len = (*pskb)->nh.iph->ihl * 4;
>+	int hdrlen = sizeof(struct iphdr);
>+	int bad = 0;
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ipt_hook: happy cracking.\n");
>-		return NF_ACCEPT;
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
>+		bad = 1;
> 	}
>-	return ip_conntrack_in(hooknum, pskb, in, out, okfn);
>+
>+	if (len < hdrlen) {
>+		if (net_ratelimit())
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" IPv4 header too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				len, hdrlen);
>+		bad = 1;
>+	}
>+
>+	if (bad == 1)
>+		return NF_ACCEPT;
>+	else
>+		return ip_conntrack_in(hooknum, pskb, in, out, okfn);
> }
>
> /* Connection tracking may drop packets, but never alters them, so
>diff -u linux.orig/net/ipv4/netfilter/iptable_filter.c linux/net/ipv4/netfilter/iptable_filter.c
>--- linux.orig/net/ipv4/netfilter/iptable_filter.c	2003-09-08 21:50:01.000000000 +0200
>+++ linux/net/ipv4/netfilter/iptable_filter.c	2003-11-06 19:38:57.000000000 +0100
>@@ -111,15 +111,35 @@
> 		   const struct net_device *out,
> 		   int (*okfn)(struct sk_buff *))
> {
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int len = (*pskb)->nh.iph->ihl * 4;
>+	int hdrlen = sizeof(struct iphdr);
>+	int bad = 0;
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ipt_hook: happy cracking.\n");
>-		return NF_ACCEPT;
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
>+		bad = 1;
>+	}
>+
>+	if (len < hdrlen) {
>+		if (net_ratelimit())
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" IPv4 header too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				len, hdrlen);
>+		bad = 1;
> 	}
>
>-	return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
>+	if (bad == 1)
>+		return NF_ACCEPT;
>+	else
>+		return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
> }
>
> static struct nf_hook_ops ipt_ops[] = {
>diff -u linux.orig/net/ipv4/netfilter/iptable_mangle.c linux/net/ipv4/netfilter/iptable_mangle.c
>--- linux.orig/net/ipv4/netfilter/iptable_mangle.c	2003-09-08 21:50:22.000000000 +0200
>+++ linux/net/ipv4/netfilter/iptable_mangle.c	2003-11-06 19:39:06.000000000 +0100
>@@ -149,14 +149,34 @@
> 	u_int32_t saddr, daddr;
> 	unsigned long nfmark;
>
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int len = (*pskb)->nh.iph->ihl * 4;
>+	int hdrlen = sizeof(struct iphdr);
>+	int bad = 0;
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ipt_hook: happy cracking.\n");
>-		return NF_ACCEPT;
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
>+		bad = 1;
> 	}
>
>+	if (len < hdrlen) {
>+		if (net_ratelimit())
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" IPv4 header too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				len, hdrlen);
>+		bad = 1;
>+	}
>+
>+	if (bad == 1)
>+		return NF_ACCEPT;
>+
> 	/* Save things which could affect route */
> 	nfmark = (*pskb)->nfmark;
> 	saddr = (*pskb)->nh.iph->saddr;
>diff -u linux.orig/net/ipv6/netfilter/ip6table_filter.c linux/net/ipv6/netfilter/ip6table_filter.c
>--- linux.orig/net/ipv6/netfilter/ip6table_filter.c	2003-09-08 21:49:52.000000000 +0200
>+++ linux/net/ipv6/netfilter/ip6table_filter.c	2003-11-06 19:40:12.000000000 +0100
>@@ -111,15 +111,19 @@
> 		   const struct net_device *out,
> 		   int (*okfn)(struct sk_buff *))
> {
>-#if 0
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int hdrlen = sizeof(struct ipv6hdr);
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ip6t_hook: happy cracking.\n");
>+			printk("ip6t_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
> 		return NF_ACCEPT;
> 	}
>-#endif
>
> 	return ip6t_do_table(pskb, hook, in, out, &packet_filter, NULL);
> }
>diff -u linux.orig/net/ipv6/netfilter/ip6table_mangle.c linux/net/ipv6/netfilter/ip6table_mangle.c
>--- linux.orig/net/ipv6/netfilter/ip6table_mangle.c	2003-09-08 21:50:02.000000000 +0200
>+++ linux/net/ipv6/netfilter/ip6table_mangle.c	2003-11-06 19:40:17.000000000 +0100
>@@ -148,15 +148,19 @@
> 	u_int8_t hop_limit;
> 	u_int32_t flowlabel;
>
>-#if 0
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int hdrlen = sizeof(struct ipv6hdr);
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ip6t_hook: happy cracking.\n");
>+			printk("ip6t_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
> 		return NF_ACCEPT;
> 	}
>-#endif
>
> 	/* FIXME: Push down to extensions --RR */
> 	if (skb_is_nonlinear(*pskb) && skb_linearize(*pskb, GFP_ATOMIC) != 0)
>
>------------------------------------------------------------------------
>
>diff -u linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c linux/net/ipv4/netfilter/ip_conntrack_standalone.c
>--- linux.orig/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 20:36:25.000000000 +0100
>+++ linux/net/ipv4/netfilter/ip_conntrack_standalone.c	2003-11-06 19:38:48.000000000 +0100
>@@ -215,14 +215,35 @@
> 				       const struct net_device *out,
> 				       int (*okfn)(struct sk_buff *))
> {
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int len = (*pskb)->nh.iph->ihl * 4;
>+	int hdrlen = sizeof(struct iphdr);
>+	int bad = 0;
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ipt_hook: happy cracking.\n");
>-		return NF_ACCEPT;
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
>+		bad = 1;
> 	}
>-	return ip_conntrack_in(hooknum, pskb, in, out, okfn);
>+
>+	if (len < hdrlen) {
>+		if (net_ratelimit())
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" IPv4 header too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				len, hdrlen);
>+		bad = 1;
>+	}
>+
>+	if (bad == 1)
>+		return NF_ACCEPT;
>+	else
>+		return ip_conntrack_in(hooknum, pskb, in, out, okfn);
> }
> 
> /* Connection tracking may drop packets, but never alters them, so
>diff -u linux.orig/net/ipv4/netfilter/iptable_filter.c linux/net/ipv4/netfilter/iptable_filter.c
>--- linux.orig/net/ipv4/netfilter/iptable_filter.c	2003-09-08 21:50:01.000000000 +0200
>+++ linux/net/ipv4/netfilter/iptable_filter.c	2003-11-06 19:38:57.000000000 +0100
>@@ -111,15 +111,35 @@
> 		   const struct net_device *out,
> 		   int (*okfn)(struct sk_buff *))
> {
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int len = (*pskb)->nh.iph->ihl * 4;
>+	int hdrlen = sizeof(struct iphdr);
>+	int bad = 0;
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ipt_hook: happy cracking.\n");
>-		return NF_ACCEPT;
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
>+		bad = 1;
>+	}
>+
>+	if (len < hdrlen) {
>+		if (net_ratelimit())
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" IPv4 header too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				len, hdrlen);
>+		bad = 1;
> 	}
> 
>-	return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
>+	if (bad == 1)
>+		return NF_ACCEPT;
>+	else
>+		return ipt_do_table(pskb, hook, in, out, &packet_filter, NULL);
> }
> 
> static struct nf_hook_ops ipt_ops[] = {
>diff -u linux.orig/net/ipv4/netfilter/iptable_mangle.c linux/net/ipv4/netfilter/iptable_mangle.c
>--- linux.orig/net/ipv4/netfilter/iptable_mangle.c	2003-09-08 21:50:22.000000000 +0200
>+++ linux/net/ipv4/netfilter/iptable_mangle.c	2003-11-06 19:39:06.000000000 +0100
>@@ -149,14 +149,34 @@
> 	u_int32_t saddr, daddr;
> 	unsigned long nfmark;
> 
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int len = (*pskb)->nh.iph->ihl * 4;
>+	int hdrlen = sizeof(struct iphdr);
>+	int bad = 0;
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ipt_hook: happy cracking.\n");
>-		return NF_ACCEPT;
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
>+		bad = 1;
> 	}
> 
>+	if (len < hdrlen) {
>+		if (net_ratelimit())
>+			printk("ipt_hook: happy cracking at %s in %s."
>+				" IPv4 header too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				len, hdrlen);
>+		bad = 1;
>+	}
>+
>+	if (bad == 1)
>+		return NF_ACCEPT;
>+
> 	/* Save things which could affect route */
> 	nfmark = (*pskb)->nfmark;
> 	saddr = (*pskb)->nh.iph->saddr;
>diff -u linux.orig/net/ipv6/netfilter/ip6table_filter.c linux/net/ipv6/netfilter/ip6table_filter.c
>--- linux.orig/net/ipv6/netfilter/ip6table_filter.c	2003-09-08 21:49:52.000000000 +0200
>+++ linux/net/ipv6/netfilter/ip6table_filter.c	2003-11-06 19:40:12.000000000 +0100
>@@ -111,15 +111,19 @@
> 		   const struct net_device *out,
> 		   int (*okfn)(struct sk_buff *))
> {
>-#if 0
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int hdrlen = sizeof(struct ipv6hdr);
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ip6t_hook: happy cracking.\n");
>+			printk("ip6t_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
> 		return NF_ACCEPT;
> 	}
>-#endif
> 
> 	return ip6t_do_table(pskb, hook, in, out, &packet_filter, NULL);
> }
>diff -u linux.orig/net/ipv6/netfilter/ip6table_mangle.c linux/net/ipv6/netfilter/ip6table_mangle.c
>--- linux.orig/net/ipv6/netfilter/ip6table_mangle.c	2003-09-08 21:50:02.000000000 +0200
>+++ linux/net/ipv6/netfilter/ip6table_mangle.c	2003-11-06 19:40:17.000000000 +0100
>@@ -148,15 +148,19 @@
> 	u_int8_t hop_limit;
> 	u_int32_t flowlabel;
> 
>-#if 0
>+	/* More verbosity to happy cracking by Maciej Soltysiak
>+	 * <solt@dns.toxicfilms.tv> */
>+	int hdrlen = sizeof(struct ipv6hdr);
>+
> 	/* root is playing with raw sockets. */
>-	if ((*pskb)->len < sizeof(struct iphdr)
>-	    || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
>+	if ((*pskb)->len < hdrlen) {
> 		if (net_ratelimit())
>-			printk("ip6t_hook: happy cracking.\n");
>+			printk("ip6t_hook: happy cracking at %s in %s."
>+				" sk_buff too short: %u. Expected: %u\n",
>+				__FUNCTION__, __FILE__,
>+				(*pskb)->len, hdrlen);
> 		return NF_ACCEPT;
> 	}
>-#endif
> 
> 	/* FIXME: Push down to extensions --RR */
> 	if (skb_is_nonlinear(*pskb) && skb_linearize(*pskb, GFP_ATOMIC) != 0)
>  
>

Re: [PATCH] make happy cracking message more verbose

[Maciej Soltysiak]

Hi,

> of the offending process after receiving these messages
> for 8 days.
I have had the same here :) That's why i tried to debug it.

> It turned out to be an unrelated bug, located
> in skb_copy_expand.
Hmm, is there a fix ? I would like to test it, and see if
it still happens on my box.

> If any we should print the name of the process producing
> the packets, or "kernel" if they are produced out of process
> context.
That is of course better than what I was trying. Anyway
I am glad that this area is being looked at.

> Still I don't see the point in iptables trying
> to perform IDS actions, in my opinion a counter should
> be incremented and nothing should be printed at all.
Seems reasonable.

> Patrick
Maciej

Re: [PATCH] make happy cracking message more verbose

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

Maciej Soltysiak wrote:

>>It turned out to be an unrelated bug, located
>>in skb_copy_expand.
>>    
>>
>Hmm, is there a fix ? I would like to test it, and see if
>it still happens on my box.
>  
>

Yes it's attached to this mail. It's also in davem's tree
now, but IIRC the bitkeeper repositories are down until
the weekend.

Best regards,
Patrick


[-- Attachment #2: 2.6-skb_copy_expand.diff --]
[-- Type: text/plain, Size: 1075 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1413  -> 1.1414 
#	   net/core/skbuff.c	1.32    -> 1.33   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/11/06	kaber@trash.net	1.1414
# Fix skb_copy_expand offset calculation
# --------------------------------------------
#
diff -Nru a/net/core/skbuff.c b/net/core/skbuff.c
--- a/net/core/skbuff.c	Thu Nov  6 17:34:55 2003
+++ b/net/core/skbuff.c	Thu Nov  6 17:34:55 2003
@@ -595,10 +595,10 @@
 
 	head_copy_len = skb_headroom(skb);
 	head_copy_off = 0;
-	if (newheadroom < head_copy_len) {
-		head_copy_off = head_copy_len - newheadroom;
+	if (newheadroom <= head_copy_len)
 		head_copy_len = newheadroom;
-	}
+	else
+		head_copy_off = newheadroom - head_copy_len;
 
 	/* Copy the linear header and data. */
 	if (skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,

Re: [PATCH] make happy cracking message more verbose

[Maciej Soltysiak]

> Yes it's attached to this mail. It's also in davem's tree
> now, but IIRC the bitkeeper repositories are down until
> the weekend.
Thanks, it seems that 2.6.0test9-bk12 has this patch included.

Regards,
Maciej

Re: [PATCH] make happy cracking message more verbose

[Henrik Nordstrom]

On Thu, 6 Nov 2003, Maciej Soltysiak wrote:

> I have one question about it.
> 
> If we have:
> 
> 	int len = (*pskb)->nh.iph->ihl * 4;
> 
> Is it possible to have a NULL pointer dereference here with (*pskb)->nh
> being NULL in:
> ip_conntrack_local()
> ipt_local_out_hook()
> ipt_local_hook()
> ip6t_local_out_hook()
> ip6t_local_hook()

Shoul not be possible. iptables is a IP layer thing, and nh.iph is set 
before the IP layer is called..

nh is just a union (not a pointer) of all the network layer protocols
known to the skb structure.


But I do have a question.. shouln't the ipv6 code use nh.ipv6h and struct
ipv6hdr? The ipv6 code deals with IPv6 headers doesn't it? I notice both
the original code and your changes uses nh.iph which looks plain wrong in
any code dealing with IPv6 packets. Until this is corrected you probably 
should leave those "#if 0" shields there to prevent the code from being 
active..

Regards
Henrik

Re: [PATCH] make happy cracking message more verbose

[Maciej Soltysiak]

> But I do have a question.. shouln't the ipv6 code use nh.ipv6h and struct
> ipv6hdr? The ipv6 code deals with IPv6 headers doesn't it? I notice both
> the original code and your changes uses nh.iph which looks plain wrong in
> any code dealing with IPv6 packets. Until this is corrected you probably
> should leave those "#if 0" shields there to prevent the code from being
> active..
I checked and rechecked before sending.
Now I double checked the rechecks, and ... the patch is fine :-)
Look closely, the lines with nh.iph are '-' and I am not doing
a check on nh.ipv6h in IPv6 version because the IPv6 header does not
have any type of length field. I am only comparing the sk_buff len.


Exerpt from the patch:
-#if 0
+       /* More verbosity to happy cracking by Maciej Soltysiak
+        * <solt@dns.toxicfilms.tv> */
+       int hdrlen = sizeof(struct ipv6hdr);
+                                  ^^^^^^^^ <-- this is ok
        /* root is playing with raw sockets. */
-       if ((*pskb)->len < sizeof(struct iphdr)
-           || (*pskb)->nh.iph->ihl * 4 < sizeof(struct iphdr)) {
+       if ((*pskb)->len < hdrlen) {
                if (net_ratelimit())
-                       printk("ip6t_hook: happy cracking.\n");
+                       printk("ip6t_hook: happy cracking at %s in %s."
+                               " sk_buff too short: %u. Expected: %u\n",
+                               __FUNCTION__, __FILE__,
+                               (*pskb)->len, hdrlen);
                return NF_ACCEPT;
        }
-#endif

> Henrik
Maciek