[PATCH] ipv4/ipv6: check hop limit field on input

[PATCH] ipv4/ipv6: check hop limit field on input

[Nicolas Dichtel]

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

Hi,

when network stack receives a packet, it didn't check value of ttl/hop limit
field. RFC indicates that a router must drop the packet if this field is 0.


Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>


[-- Attachment #2: x.diff --]
[-- Type: text/x-diff, Size: 2119 bytes --]

diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 1a58a6f..9253b34 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -376,6 +376,7 @@ drop:
 int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev)
 {
 	struct iphdr *iph;
+	struct in_device *in_dev;
 	u32 len;
 
 	/* When the interface is in promisc. mode, drop all the crap
@@ -415,6 +416,28 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
 
 	iph = ip_hdr(skb);
 
+	/*
+	 * RFC792 Page 6
+	 *    If the gateway processing a datagram finds the time to live field
+	 *    is zero it must discard the datagram.  The gateway may also notify
+	 *    the source host via the time exceeded message.
+	 */
+	if ((in_dev = in_dev_get(dev)) != NULL) {
+		if (iph->ttl == 0 && IPV4_DEVCONF(in_dev->cnf, FORWARDING)) {
+			if (likely(skb->dst == NULL)) {
+				int err = ip_route_input(skb, iph->daddr, iph->saddr, iph->tos,
+							 skb->dev);
+				if (unlikely(err))
+					goto inhdr_error;
+			}
+			icmp_send(skb, ICMP_TIME_EXCEEDED, ICMP_EXC_TTL, 0);
+			goto inhdr_error;
+		}
+	} else {
+		IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INDISCARDS);
+		goto drop;
+	}
+
 	if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
 		goto inhdr_error;
 
diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index f171e8d..fee40f9 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -103,6 +103,19 @@ int ipv6_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt
 		goto err;
 
 	/*
+	 * RFC4443 3.3
+	 *    If a router receives a packet with a Hop Limit of zero, or if a
+	 *    router decrements a packet's Hop Limit to zero, it MUST discard the
+	 *    packet and originate an ICMPv6 Time Exceeded message with Code 0 to
+	 *    the source of the packet.
+	 */
+	if (hdr->hop_limit == 0 && idev->cnf.forwarding) {
+		icmpv6_send(skb, ICMPV6_TIME_EXCEED, ICMPV6_EXC_HOPLIMIT,
+			    0, skb->dev);
+		goto err;
+	}
+
+	/*
 	 * RFC4291 2.5.3
 	 * A packet received on an interface with a destination address
 	 * of loopback must be dropped.

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Florian Westphal]

Nicolas Dichtel <nicolas.dichtel@dev.6wind.com> wrote:
> when network stack receives a packet, it didn't check value of ttl/hop 
> limit
> field. RFC indicates that a router must drop the packet if this field is 0.

Whats wrong with the checks in ip(6)_forward?

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Nicolas Dichtel]

Le 01.06.2009 18:19, Florian Westphal a écrit :
> Nicolas Dichtel <nicolas.dichtel@dev.6wind.com> wrote:
>> when network stack receives a packet, it didn't check value of ttl/hop 
>> limit
>> field. RFC indicates that a router must drop the packet if this field is 0.
> 
> Whats wrong with the checks in ip(6)_forward?
It's on forward, not on input. Router must not process it.
For example, if you try to ping (with ttl set to 0) the router, you will receive 
a reply.

Nicolas

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Florian Westphal]

Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>> Whats wrong with the checks in ip(6)_forward?
> It's on forward, not on input. Router must not process it.
> For example, if you try to ping (with ttl set to 0) the router, you will 
> receive a reply.

Ah.  That makes more sense.
However, I'd argue that this is sane behaviour.

The datagram did reach its intended destination and the TTL did not
"exceed in transit" (if it had, the datagram would not have been
received).  Why discard an otherwise perfectly legal packet?

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Eric Dumazet]

Nicolas Dichtel a écrit :
> Le 01.06.2009 18:19, Florian Westphal a écrit :
>> Nicolas Dichtel <nicolas.dichtel@dev.6wind.com> wrote:
>>> when network stack receives a packet, it didn't check value of
>>> ttl/hop limit
>>> field. RFC indicates that a router must drop the packet if this field
>>> is 0.
>>
>> Whats wrong with the checks in ip(6)_forward?
> It's on forward, not on input. Router must not process it.
> For example, if you try to ping (with ttl set to 0) the router, you will
> receive a reply.
> 

You seem to mix requirements for routers and hosts. ttl processing
is relevant for a gateway only, not for a host.

(terminology : gateway / host in rfc 792)

I would say : who sent this ttl=0 packet at first ?

ping -t 0 host
ping: can't set unicast time-to-live: Invalid argument

So Linux is not able to do that, unless using tricks of course, or patching IP_TTL

BTW, sending ttl=0 packets to my cisco host (also a router but not relevant)
is ok, it replies to this packets...

I wonder why Linux forbids sending ttl=0 packets, time to read again all these RFCs :)

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Brian Haley]

Eric Dumazet wrote:
> Nicolas Dichtel a écrit :
>> Le 01.06.2009 18:19, Florian Westphal a écrit :
>>> Nicolas Dichtel <nicolas.dichtel@dev.6wind.com> wrote:
>>>> when network stack receives a packet, it didn't check value of
>>>> ttl/hop limit
>>>> field. RFC indicates that a router must drop the packet if this field
>>>> is 0.
>>> Whats wrong with the checks in ip(6)_forward?
>> It's on forward, not on input. Router must not process it.
>> For example, if you try to ping (with ttl set to 0) the router, you will
>> receive a reply.
>>
> 
> You seem to mix requirements for routers and hosts. ttl processing
> is relevant for a gateway only, not for a host.
> 
> (terminology : gateway / host in rfc 792)
> 
> I would say : who sent this ttl=0 packet at first ?
> 
> ping -t 0 host
> ping: can't set unicast time-to-live: Invalid argument
> 
> So Linux is not able to do that, unless using tricks of course, or patching IP_TTL

'ping6 -t 0 host' does work however.  The problem I see is that if you ping a system,
if it's a host it will respond, if it's a router it won't - the RFCs don't
explicitly state the host should drop the packet.  I don't know if that difference
in behavior is desired.  Do we know how any other OSes behave?

-Brian

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[John Dykstra]

On Mon, 2009-06-01 at 14:55 -0400, Brian Haley wrote: 
> 'ping6 -t 0 host' does work however.  The problem I see is that if you ping a system,
> if it's a host it will respond, if it's a router it won't - the RFCs don't
> explicitly state the host should drop the packet.  

There are two cases--an echo request to an address assigned to a
router's interface, and to an address _beyond_ the router on another
link.

Any given interface on a router can have forwarding dynamically enabled
or disabled.  I don't remember prescribed echo request or hop limit
behavior changing depending on the forwarding enable, so it seems that
if you ping an address assigned to a router's interface, the router is
expected to follow the (apparently unwritten) host rules.  

Echo requests forwarded by a router should obviously have the hop limit
decremented and checked.

> I don't know if that difference
> in behavior is desired.  Do we know how any other OSes behave?

FWIW, the random BSD flavors I have on hand all check hop limit when
forwarding, but not when processing local ingress traffic.

Also FWIW, as I remember, the TAHI tests only check hop limit behavior
on forwarded traffic.

Nicolas, what's driving your patch?  Are you trying to align slow path
behavior with one of the 6WIND fast path implementations?

  --  John

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[David Miller]

From: John Dykstra <john.dykstra1@gmail.com>
Date: Mon, 01 Jun 2009 20:54:06 -0500

> On Mon, 2009-06-01 at 14:55 -0400, Brian Haley wrote: 
>> I don't know if that difference
>> in behavior is desired.  Do we know how any other OSes behave?
> 
> FWIW, the random BSD flavors I have on hand all check hop limit when
> forwarding, but not when processing local ingress traffic.
> 
> Also FWIW, as I remember, the TAHI tests only check hop limit behavior
> on forwarded traffic.

And this is the behavior that makes the most sense to me.

The local system is "accounted for" in the hop limit by the previous
hop system.  No other behavior makes any sense.

And I even remember there are applications that use multicast and
a hop limit of zero explicitly to keep application traffic only on
the local subnet.  So any change like that proposed could break
things.

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@dev.6wind.com>
Date: Mon, 01 Jun 2009 17:13:43 +0200

> RFC indicates that a router must drop the packet if this field is 0.

It only must do this when executing the forwarding function.  It's an
egress check, not an ingress one.

I'm not applying this patch, it can even break some applications
out there that use a TTL of zero intentionally to keep traffic
only on a local subnet.

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Eric Dumazet]

David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@dev.6wind.com>
> Date: Mon, 01 Jun 2009 17:13:43 +0200
> 
>> RFC indicates that a router must drop the packet if this field is 0.
> 
> It only must do this when executing the forwarding function.  It's an
> egress check, not an ingress one.
> 
> I'm not applying this patch, it can even break some applications
> out there that use a TTL of zero intentionally to keep traffic
> only on a local subnet.

I wonder if we then should allow setting ttl to zero. I had to patch
my kernel to allow ping to do this...

I'll check RFC when time permits.

diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index e2d1f87..efe2797 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -558,7 +558,7 @@ static int do_ip_setsockopt(struct sock *sk, int level,
 	case IP_TTL:
 		if (optlen<1)
 			goto e_inval;
-		if (val != -1 && (val < 1 || val>255))
+		if (val != -1 && (val < 0 || val>255))
 			goto e_inval;
 		inet->uc_ttl = val;
 		break;

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Tue, 02 Jun 2009 07:31:12 +0200

> David Miller a écrit :
>> From: Nicolas Dichtel <nicolas.dichtel@dev.6wind.com>
>> Date: Mon, 01 Jun 2009 17:13:43 +0200
>> 
>>> RFC indicates that a router must drop the packet if this field is 0.
>> 
>> It only must do this when executing the forwarding function.  It's an
>> egress check, not an ingress one.
>> 
>> I'm not applying this patch, it can even break some applications
>> out there that use a TTL of zero intentionally to keep traffic
>> only on a local subnet.
> 
> I wonder if we then should allow setting ttl to zero. I had to patch
> my kernel to allow ping to do this...
> 
> I'll check RFC when time permits.

Eric, notice how I mentioned in my other reply to this thread
"multicast" applications, which use mc_ttl which we allow to be set to
zero.

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[John Dykstra]

On Mon, 2009-06-01 at 19:02 -0700, David Miller wrote:
> And I even remember there are applications that use multicast and
> a hop limit of zero explicitly to keep application traffic only on
> the local subnet.  So any change like that proposed could break
> things.

Are you thinking of multicast apps that explicitly set a hop limit, or Multicast
Listener Discovery?  The hop limit specified for MLDv2 messages is one,
not zero.

  --  John

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Nicolas Dichtel]

Florian Westphal wrote:
> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>>> Whats wrong with the checks in ip(6)_forward?
>> It's on forward, not on input. Router must not process it.
>> For example, if you try to ping (with ttl set to 0) the router, you will 
>> receive a reply.
> 
> Ah.  That makes more sense.
> However, I'd argue that this is sane behaviour.
> 
> The datagram did reach its intended destination and the TTL did not
> "exceed in transit" (if it had, the datagram would not have been
> received).  Why discard an otherwise perfectly legal packet?
Because RFC requires this:

RFC792 Page 6:
   If the gateway processing a datagram finds the time to live field
   is zero it must discard the datagram.  The gateway may also notify
   the source host via the time exceeded message.

RFC4443 Section 3.3:
    If a router receives a packet with a Hop Limit of zero, or if a
    router decrements a packet's Hop Limit to zero, it MUST discard the
    packet and originate an ICMPv6 Time Exceeded message with Code 0 to
    the source of the packet.

Nicolas

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Nicolas Dichtel]

Eric Dumazet wrote:
> Nicolas Dichtel a écrit :
>> Le 01.06.2009 18:19, Florian Westphal a écrit :
>>> Nicolas Dichtel <nicolas.dichtel@dev.6wind.com> wrote:
>>>> when network stack receives a packet, it didn't check value of
>>>> ttl/hop limit
>>>> field. RFC indicates that a router must drop the packet if this field
>>>> is 0.
>>> Whats wrong with the checks in ip(6)_forward?
>> It's on forward, not on input. Router must not process it.
>> For example, if you try to ping (with ttl set to 0) the router, you will
>> receive a reply.
>>
> 
> You seem to mix requirements for routers and hosts. ttl processing
> is relevant for a gateway only, not for a host.
It's why I test the forwarding value in my patch. If forwarding is 
enable, it's a router.

> 
> (terminology : gateway / host in rfc 792)
> 
> I would say : who sent this ttl=0 packet at first ?
I prefere to ask: is this packet a "legal" packet?

> 
> ping -t 0 host
> ping: can't set unicast time-to-live: Invalid argument
> 
> So Linux is not able to do that, unless using tricks of course, or patching IP_TTL
> 
> BTW, sending ttl=0 packets to my cisco host (also a router but not relevant)
> is ok, it replies to this packets...
> 
> I wonder why Linux forbids sending ttl=0 packets, time to read again all these RFCs :)
I find a statement which tell to drop the packet but maybe I've missing 
something.


Nicolas

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Nicolas Dichtel]

Brian Haley wrote:
> Eric Dumazet wrote:
>> Nicolas Dichtel a écrit :
>>> Le 01.06.2009 18:19, Florian Westphal a écrit :
>>>> Nicolas Dichtel <nicolas.dichtel@dev.6wind.com> wrote:
>>>>> when network stack receives a packet, it didn't check value of
>>>>> ttl/hop limit
>>>>> field. RFC indicates that a router must drop the packet if this field
>>>>> is 0.
>>>> Whats wrong with the checks in ip(6)_forward?
>>> It's on forward, not on input. Router must not process it.
>>> For example, if you try to ping (with ttl set to 0) the router, you will
>>> receive a reply.
>>>
>> You seem to mix requirements for routers and hosts. ttl processing
>> is relevant for a gateway only, not for a host.
>>
>> (terminology : gateway / host in rfc 792)
>>
>> I would say : who sent this ttl=0 packet at first ?
>>
>> ping -t 0 host
>> ping: can't set unicast time-to-live: Invalid argument
>>
>> So Linux is not able to do that, unless using tricks of course, or patching IP_TTL
> 
> 'ping6 -t 0 host' does work however.  The problem I see is that if you ping a system,
> if it's a host it will respond, if it's a router it won't - the RFCs don't
> explicitly state the host should drop the packet.  I don't know if that difference
> in behavior is desired.  Do we know how any other OSes behave?
I've ask the IETF mailing list about host case. Response was:
"process as normal."

Nicolas

> 
> -Brian
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[David Miller]

From: John Dykstra <john.dykstra1@gmail.com>
Date: Tue, 02 Jun 2009 09:22:10 +0000

> On Mon, 2009-06-01 at 19:02 -0700, David Miller wrote:
>> And I even remember there are applications that use multicast and
>> a hop limit of zero explicitly to keep application traffic only on
>> the local subnet.  So any change like that proposed could break
>> things.
> 
> Are you thinking of multicast apps that explicitly set a hop limit, or Multicast
> Listener Discovery?  The hop limit specified for MLDv2 messages is one,
> not zero.

I am very much thinking of multicast apps that explicitly set a hop
limit.

The proof is in the pudding.  As mentioned in other parts of this
thread, we explicitly DO NOT allow a zero normal socket TTL option
but we DO allow a zero multicast TTL socket option to be set.

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Nicolas Dichtel]

John Dykstra wrote:
> On Mon, 2009-06-01 at 14:55 -0400, Brian Haley wrote: 
>> 'ping6 -t 0 host' does work however.  The problem I see is that if you ping a system,
>> if it's a host it will respond, if it's a router it won't - the RFCs don't
>> explicitly state the host should drop the packet.  
> 
> There are two cases--an echo request to an address assigned to a
> router's interface, and to an address _beyond_ the router on another
> link.
> 
> Any given interface on a router can have forwarding dynamically enabled
> or disabled.  I don't remember prescribed echo request or hop limit
> behavior changing depending on the forwarding enable, so it seems that
> if you ping an address assigned to a router's interface, the router is
> expected to follow the (apparently unwritten) host rules.  
Good point.

> 
> Echo requests forwarded by a router should obviously have the hop limit
> decremented and checked.
> 
>> I don't know if that difference
>> in behavior is desired.  Do we know how any other OSes behave?
> 
> FWIW, the random BSD flavors I have on hand all check hop limit when
> forwarding, but not when processing local ingress traffic.
> 
> Also FWIW, as I remember, the TAHI tests only check hop limit behavior
> on forwarded traffic.
Right.

> 
> Nicolas, what's driving your patch?  Are you trying to align slow path
> behavior with one of the 6WIND fast path implementations?
No. I'm just checking RFC conformance ;-)


Nicolas

> 
>   --  John
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[Nicolas Dichtel]

David Miller wrote:
> From: Nicolas Dichtel <nicolas.dichtel@dev.6wind.com>
> Date: Mon, 01 Jun 2009 17:13:43 +0200
> 
>> RFC indicates that a router must drop the packet if this field is 0.
> 
> It only must do this when executing the forwarding function.  It's an
> egress check, not an ingress one.
In my understanding, it can be on input to:
RFC4443 Section 3.3:
    If a router receives a packet with a Hop Limit of zero, or if a
    router decrements a packet's Hop Limit to zero, it MUST discard the
    packet and originate an ICMPv6 Time Exceeded message with Code 0 to
    the source of the packet.

> 
> I'm not applying this patch, it can even break some applications
> out there that use a TTL of zero intentionally to keep traffic
> only on a local subnet.
OK ok. John sends good arguments ;-)


Nicolas

Re: [PATCH] ipv4/ipv6: check hop limit field on input

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 02 Jun 2009 11:36:29 +0200

> David Miller wrote:
>> From: Nicolas Dichtel <nicolas.dichtel@dev.6wind.com>
>> Date: Mon, 01 Jun 2009 17:13:43 +0200
>> 
>>> RFC indicates that a router must drop the packet if this field is 0.
>> It only must do this when executing the forwarding function.  It's an
>> egress check, not an ingress one.
> In my understanding, it can be on input to:
> RFC4443 Section 3.3:
>    If a router receives a packet with a Hop Limit of zero, or if a
>    router decrements a packet's Hop Limit to zero, it MUST discard the
>    packet and originate an ICMPv6 Time Exceeded message with Code 0 to
>    the source of the packet.

I think this was unintentional.

Usage of zero TTL by multicast applications is very well established.

Running such an application on a router should work as well.