[PATCH v2 net] ipv6: Fix MLD Query message check

[PATCH v2 net] ipv6: Fix MLD Query message check

[Hangbin Liu]

Based on RFC3810 6.2, we also need to check the hop limit and router alert
option besides source address.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/mcast.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 08b367c..fe27a77 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1301,8 +1301,18 @@ int igmp6_event_query(struct sk_buff *skb)
 	len = ntohs(ipv6_hdr(skb)->payload_len) + sizeof(struct ipv6hdr);
 	len -= skb_network_header_len(skb);
 
-	/* Drop queries with not link local source */
-	if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL))
+	/* RFC3810 6.2
+	 * Upon reception of an MLD message that contains a Query, the node
+	 * checks if the source address of the message is a valid link-local
+	 * address, if the Hop Limit is set to 1, and if the Router Alert
+	 * option is present in the Hop-By-Hop Options header of the IPv6
+	 * packet.  If any of these checks fails, the packet is dropped.
+	 */
+	if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) ||
+	    ipv6_hdr(skb)->hop_limit != 1 ||
+	    ipv6_hdr(skb)->nexthdr != NEXTHDR_HOP ||
+	    !(IP6CB(skb)->flags & IP6SKB_ROUTERALERT) ||
+	    IP6CB(skb)->ra != htons(IPV6_OPT_ROUTERALERT_MLD))
 		return -EINVAL;
 
 	idev = __in6_dev_get(skb->dev);
-- 
1.8.1.4

Re: [PATCH v2 net] ipv6: Fix MLD Query message check

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Wed, 25 Jun 2014 09:31:07 +0800

> @@ -1301,8 +1301,18 @@ int igmp6_event_query(struct sk_buff *skb)
>  	len = ntohs(ipv6_hdr(skb)->payload_len) + sizeof(struct ipv6hdr);
>  	len -= skb_network_header_len(skb);
>  
> -	/* Drop queries with not link local source */
> -	if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL))
> +	/* RFC3810 6.2
> +	 * Upon reception of an MLD message that contains a Query, the node
> +	 * checks if the source address of the message is a valid link-local
> +	 * address, if the Hop Limit is set to 1, and if the Router Alert
> +	 * option is present in the Hop-By-Hop Options header of the IPv6
> +	 * packet.  If any of these checks fails, the packet is dropped.
> +	 */
> +	if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) ||
> +	    ipv6_hdr(skb)->hop_limit != 1 ||
> +	    ipv6_hdr(skb)->nexthdr != NEXTHDR_HOP ||
> +	    !(IP6CB(skb)->flags & IP6SKB_ROUTERALERT) ||
> +	    IP6CB(skb)->ra != htons(IPV6_OPT_ROUTERALERT_MLD))
>  		return -EINVAL;
>  

Does the NEXTHDR_HOP have the be exactly the next extension header
present?  I think you need to allow for more flexibility here.

In any event, the only way the IP6SKB_ROUTERALERT bit can be set
is if there were hop-by-hop options present, so testing only the
bit itself is sufficient.

The forwarding path simplifies the check in this way too.

So please adjust this to only check the bit and remove the NEXTHDR_HOP
test.

Thanks.

Re: [PATCH v2 net] ipv6: Fix MLD Query message check

[Hangbin Liu]

2014-06-27 8:33 GMT+08:00 David Miller <davem@davemloft.net>:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Wed, 25 Jun 2014 09:31:07 +0800
>
>> @@ -1301,8 +1301,18 @@ int igmp6_event_query(struct sk_buff *skb)
>>       len = ntohs(ipv6_hdr(skb)->payload_len) + sizeof(struct ipv6hdr);
>>       len -= skb_network_header_len(skb);
>>
>> -     /* Drop queries with not link local source */
>> -     if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL))
>> +     /* RFC3810 6.2
>> +      * Upon reception of an MLD message that contains a Query, the node
>> +      * checks if the source address of the message is a valid link-local
>> +      * address, if the Hop Limit is set to 1, and if the Router Alert
>> +      * option is present in the Hop-By-Hop Options header of the IPv6
>> +      * packet.  If any of these checks fails, the packet is dropped.
>> +      */
>> +     if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) ||
>> +         ipv6_hdr(skb)->hop_limit != 1 ||
>> +         ipv6_hdr(skb)->nexthdr != NEXTHDR_HOP ||
>> +         !(IP6CB(skb)->flags & IP6SKB_ROUTERALERT) ||
>> +         IP6CB(skb)->ra != htons(IPV6_OPT_ROUTERALERT_MLD))
>>               return -EINVAL;
>>
>
> Does the NEXTHDR_HOP have the be exactly the next extension header
> present?  I think you need to allow for more flexibility here.
>
> In any event, the only way the IP6SKB_ROUTERALERT bit can be set
> is if there were hop-by-hop options present, so testing only the
> bit itself is sufficient.
>
> The forwarding path simplifies the check in this way too.
>
> So please adjust this to only check the bit and remove the NEXTHDR_HOP
> test.
>
Thanks for this advise, will send a v3 patch

Best regards
Hangbin Liu