Re: [PATCH] remove overzealous checks in REJECT target]

Re: [PATCH] remove overzealous checks in REJECT target]

[Harald Welte]

[-- Attachment #1.1: Type: text/plain, Size: 560 bytes --]

Hi Patrick!

I agree with Yasuyuki's proposed changes, do you already have this patch
in your pending queue?

I'm just asking because there was no follow-up on the list...

Thanks!
-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #1.2: Type: message/rfc822, Size: 7358 bytes --]

[-- Attachment #1.2.1.1: Type: Text/Plain, Size: 1649 bytes --]


Hi,

I think so. This check isn't needed.

And I found strange codes after this check: directly access to the data
in skb.h and overwriting ICMP header with same data.

I think we should use skb_header_pointer() to get UDP/ICMP header.

Signed-off-by: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>

-----------------------------------------------------------------
Yasuyuki KOZAKAI @ USAGI Project <yasuyuki.kozakai@toshiba.co.jp>

From: Carl-Daniel Hailfinger <c-d.hailfinger.kernel.2004@gmx.net>
Date: Wed, 01 Dec 2004 07:41:39 +0100

> Hi,
> 
> after wondering why the REJECT target didn't work as expected
> when scanned with nmap -sO, I found a check in ipt_REJECT.c
> for 8 or more bytes of proto header which caused all packets
> gernated by nmap to be dropped although they were sent to the
> REJECT target. Since I could not see any use for the proto
> header length check, I replaced it with a warning.
> Now the REJECT target works as expected for all packets I
> could thow at it.
> 
> Regards,
> Carl-Daniel
> -- 
> http://www.hailfinger.org/
> 
> Signed-off-by Carl-Daniel Hailfinger <c-d.hailfinger.kernel.2004@gmx.net>
> --- linux-2.6.9/net/ipv4/netfilter/ipt_REJECT.c~	Wed Dec  1 06:38:06 2004
> +++ linux-2.6.9/net/ipv4/netfilter/ipt_REJECT.c	Wed Dec  1 06:41:04 2004
> @@ -255,7 +255,7 @@ static void send_unreach(struct sk_buff
> 
>  	/* Ensure we have at least 8 bytes of proto header. */
>  	if (skb_in->len < skb_in->nh.iph->ihl*4 + 8)
> -		return;
> +		printk("REJECT: we have less than 8 bytes of proto header.\n");
> 
>  	/* if UDP checksum is set, verify it's correct */
>  	if (iph->protocol == IPPROTO_UDP
> 
> 
> 

[-- Attachment #1.2.1.2: ipt_reject.patch --]
[-- Type: Text/Plain, Size: 2520 bytes --]

--- linux-2.6.10-rc2-rc2-bk13/net/ipv4/netfilter/ipt_REJECT.c	2004-12-02 11:45:28.000000000 +0900
+++ linux-2.6.10-rc2-rc2-bk13-reject/net/ipv4/netfilter/ipt_REJECT.c	2004-12-02 14:47:17.245615416 +0900
@@ -223,8 +223,8 @@
 static void send_unreach(struct sk_buff *skb_in, int code)
 {
 	struct iphdr *iph;
-	struct udphdr *udph;
-	struct icmphdr *icmph;
+	struct udphdr *udph, _udph;
+	struct icmphdr *icmph, _icmph;
 	struct sk_buff *nskb;
 	u32 saddr;
 	u8 tos;
@@ -253,40 +253,37 @@
 	if (iph->frag_off&htons(IP_OFFSET))
 		return;
 
-	/* Ensure we have at least 8 bytes of proto header. */
-	if (skb_in->len < skb_in->nh.iph->ihl*4 + 8)
-		return;
-
 	/* if UDP checksum is set, verify it's correct */
-	if (iph->protocol == IPPROTO_UDP
-	    && skb_in->tail-(u8*)iph >= sizeof(struct udphdr)) {
-		int datalen = skb_in->len - (iph->ihl<<2);
-		udph = (struct udphdr *)((char *)iph + (iph->ihl<<2));
-		if (udph->check
+	if (iph->protocol == IPPROTO_UDP) {
+		int udphoff = ((u8*)iph - skb_in->data) + (iph->ihl << 2);
+		int datalen = skb_in->len - udphoff;
+
+		udph = skb_header_pointer(skb_in, udphoff,
+					  sizeof(struct udphdr), &_udph);
+
+		if (udph && udph->check
 		    && csum_tcpudp_magic(iph->saddr, iph->daddr,
 		                         datalen, IPPROTO_UDP,
-		                         csum_partial((char *)udph, datalen,
-		                                      0)) != 0)
+					 skb_checksum(skb_in, udphoff, datalen,
+						      0)) != 0)
 			return;
 	}
 
 	/* If we send an ICMP error to an ICMP error a mess would result.. */
-	if (iph->protocol == IPPROTO_ICMP
-	    && skb_in->tail-(u8*)iph >= sizeof(struct icmphdr)) {
-		icmph = (struct icmphdr *)((char *)iph + (iph->ihl<<2));
-
-		if (skb_copy_bits(skb_in, skb_in->nh.iph->ihl*4,
-				  icmph, sizeof(*icmph)) < 0)
-			return;
+	if (iph->protocol == IPPROTO_ICMP) {
+		icmph = skb_header_pointer(skb_in, iph->ihl<<2,
+					   sizeof(struct icmphdr),
+					   &_icmph);
 
 		/* Between echo-reply (0) and timestamp (13),
 		   everything except echo-request (8) is an error.
 		   Also, anything greater than NR_ICMP_TYPES is
 		   unknown, and hence should be treated as an error... */
-		if ((icmph->type < ICMP_TIMESTAMP
-		     && icmph->type != ICMP_ECHOREPLY
-		     && icmph->type != ICMP_ECHO)
-		    || icmph->type > NR_ICMP_TYPES)
+		if (icmph &&
+		    ((icmph->type < ICMP_TIMESTAMP
+		      && icmph->type != ICMP_ECHOREPLY
+		      && icmph->type != ICMP_ECHO)
+		     || icmph->type > NR_ICMP_TYPES))
 			return;
 	}
 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] remove overzealous checks in REJECT target]

[Patrick McHardy]

Harald Welte wrote:

>Hi Patrick!
>
>I agree with Yasuyuki's proposed changes, do you already have this patch
>in your pending queue?
>
>I'm just asking because there was no follow-up on the list...
>
I missed it, but the patch is wrong. We must return at least 8 byte of
protocol header, so the check can't be removed. The skb_header_pointer
part looks fine, I'm going to apply it after getting some sleep.

RFC1122: §3.2.2:
Every ICMP error message includes the Internet header and at
least the first 8 data octets of the datagram that triggered
the error; more than 8 octets MAY be sent; this header and data
MUST be unchanged from the received datagram.

Regards
Patrick

Re: [PATCH] remove overzealous checks in REJECT target]

[Harald Welte]

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

On Fri, Dec 17, 2004 at 06:43:39AM +0100, Patrick McHardy wrote:
> Harald Welte wrote:
> 
> >Hi Patrick!
> >
> >I agree with Yasuyuki's proposed changes, do you already have this patch
> >in your pending queue?
>
> I missed it, but the patch is wrong. We must return at least 8 byte of
> protocol header, so the check can't be removed. The skb_header_pointer
> part looks fine, I'm going to apply it after getting some sleep.

> RFC1122: §3.2.2:

Thanks for pointing this out.  I certainly read it a number of times
before, but it slipped my mind temporarily.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] remove overzealous checks in REJECT target]

[Carl-Daniel Hailfinger]

Harald Welte schrieb:
> On Fri, Dec 17, 2004 at 06:43:39AM +0100, Patrick McHardy wrote:
> 
>>Harald Welte wrote:
>>
>>
>>>Hi Patrick!
>>>
>>>I agree with Yasuyuki's proposed changes, do you already have this patch
>>>in your pending queue?
>>
>>I missed it, but the patch is wrong. We must return at least 8 byte of
>>protocol header, so the check can't be removed. The skb_header_pointer
>>part looks fine, I'm going to apply it after getting some sleep.
> 
> 
>>RFC1122: §3.2.2:
> 
> 
> Thanks for pointing this out.  I certainly read it a number of times
> before, but it slipped my mind temporarily.

Well, the kernel for sure doesn't care if netfilter isn't loaded. My
patch (and by consequence, Yasuyuki's patch) only tried to behave the
same as a kernel without netfilter enabled.

Hint: Try nmap "protocol scan" on a host without netfilter loaded. It
will happiliy reject packets which are too short. Then enable REJECT
for all IP protocols you don't want to support. And you'll see that
the too short packets will suddenly stay unanswered.

So we either break the standard or we don't, but breaking it only if
netfilter is not loaded doesn't sound like a sensible default to me.


Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/

Re: [PATCH] remove overzealous checks in REJECT target]

[Patrick McHardy]

Carl-Daniel Hailfinger wrote:

>Well, the kernel for sure doesn't care if netfilter isn't loaded. My
>patch (and by consequence, Yasuyuki's patch) only tried to behave the
>same as a kernel without netfilter enabled.
>
>Hint: Try nmap "protocol scan" on a host without netfilter loaded. It
>will happiliy reject packets which are too short. Then enable REJECT
>for all IP protocols you don't want to support. And you'll see that
>the too short packets will suddenly stay unanswered.
>
You're right. I was misguided by this comment in icmp.c:

 *      RFC 1122: 3.2.2 MUST send at least the IP header and 8 bytes of 
header.

but icmp.c doesn't enforce this like ipt_REJECT. If no header is present, it
seems we are not required to return it :)

>
>So we either break the standard or we don't, but breaking it only if
>netfilter is not loaded doesn't sound like a sensible default to me.
>
Agreed, I'm going to apply the entire patch.

Regards
Patrick