ipip6 - integer underrun when handlince icmpv4 unreachable messages

ipip6 - integer underrun when handlince icmpv4 unreachable messages

[Alexander Wetzel]

Hello netdev,

the current code to translate icmpv4 "destination unreachable" packets
to icmpv6 is later generating an integer underrun when calling
icmpv6_send, by later calling skb_network_header_len and subtracting a
bigger number from a lower one.

The issue is not visible for vanilla kernels and works correctly from
a user perspective, but a kernel with the PAX patches and enabled
"size overflow protection" will panic immediately when it's getting an
icmpv4 destination unreachable packet back for an encapsulated ipv6
packet. (Remote tunnel endpoint not reachable.)

I think I've tracked the issue down and can show you the problem with
the code... as I understand it as non-programmer greping the sources
and googeling functions. I was even able to find a fix which passes
the functionality test, but I'm unqualified to rate the correctness of
it and so are reaching out to you for that.

What happens (output of printk's) with transport_header and
network_header around "skb_reset_network_header" is described below
the function.

Near the end of the mail there are two links to the gentoo bug tracker
and pax forum, suggesting to put this forward to the lkml/netdev for
review, also including more details on the panics.

So here the function "ipip6_err_gen_icmpv6_unreach" from
net/ipv6/sit.c with some remarks and one new line which seems to fix
the problem:

************************************************************************
static int ipip6_err_gen_icmpv6_unreach(struct sk_buff *skb)
{
        int ihl = ((const struct iphdr *)skb->data)->ihl*4;
        struct rt6_info *rt;
        struct sk_buff *skb2;

        if (!pskb_may_pull(skb, ihl + sizeof(struct ipv6hdr) + 8))
                return 1;

// we clone the ipv4 skb in skb2 to prepare the icmpv6 packet
        skb2 = skb_clone(skb, GFP_ATOMIC);

        if (!skb2)
                return 1;

// we clean up the cloned skb2
        skb_dst_drop(skb2);
        skb_pull(skb2, ihl);
// The network header is reset
        skb_reset_network_header(skb2);

//THE PROPOSED FIX: The following line is NOT in the current code
        skb_reset_transport_header(skb2);

        rt = rt6_lookup(dev_net(skb->dev), &ipv6_hdr(skb2)->saddr,
NULL, 0, 0);

        if (rt && rt->dst.dev)
                skb2->dev = rt->dst.dev;

        icmpv6_send(skb2, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, 0);

        if (rt)
                ip6_rt_put(rt);

        kfree_skb(skb2);

        return 0;

************************************************************************

With debug printk's prior to "skb_reset_network_header(skb2);" I get
the following values when the code is used:
	sk2b->transport_header: 62
	skb2->network_header  : 4e

After "skb_reset_network_header(skb2);" it reads:
	sk2b->transport_header: 62
	skb2->network_header  : 7e

That are the same values which are later causing the integer underrun
(and with PAX a kernel panic) in "skb_network_header_len"

Adding "skb_reset_network_header(skb2);" prevents that, the transport
header size is extended:
	sk2b->transport_header: 7e
	skb2->network_header  : 7e


Some more details on the error, a clean diff for the proposed patch
(without the comments) and  the full debugging can be fond here:

https://forums.grsecurity.net/viewtopic.php?t=4083
https://bugs.gentoo.org/show_bug.cgi?id=529352

Can you please verify if this is the correct way to fix it and include
the correct fix - if any - in future kernel releases?
Or do you think that the PAX patch is wrong and an integer underrun is
acceptable here?

I'll check the mailing list archives from time to time for replies,
but I'm not subscribed and if you need me for more information or
tests please add me on CC.

Cheers,

Alexander Wetzel

Re: ipip6 - integer underrun when handlince icmpv4 unreachable messages

[Steffen Klassert]

On Mon, Nov 17, 2014 at 10:52:04PM +0100, Alexander Wetzel wrote:
> Hello netdev,
> 
> the current code to translate icmpv4 "destination unreachable" packets
> to icmpv6 is later generating an integer underrun when calling
> icmpv6_send, by later calling skb_network_header_len and subtracting a
> bigger number from a lower one.

I'd guess the call to skb_network_header_len() in _decode_session6()
is the problematic one, right?

> 
> The issue is not visible for vanilla kernels and works correctly from
> a user perspective, but a kernel with the PAX patches and enabled
> "size overflow protection" will panic immediately when it's getting an
> icmpv4 destination unreachable packet back for an encapsulated ipv6
> packet. (Remote tunnel endpoint not reachable.)
> 
> I think I've tracked the issue down and can show you the problem with
> the code... as I understand it as non-programmer greping the sources
> and googeling functions. I was even able to find a fix which passes
> the functionality test, but I'm unqualified to rate the correctness of
> it and so are reaching out to you for that.
> 
> What happens (output of printk's) with transport_header and
> network_header around "skb_reset_network_header" is described below
> the function.
> 
> Near the end of the mail there are two links to the gentoo bug tracker
> and pax forum, suggesting to put this forward to the lkml/netdev for
> review, also including more details on the panics.
> 
> So here the function "ipip6_err_gen_icmpv6_unreach" from
> net/ipv6/sit.c with some remarks and one new line which seems to fix
> the problem:
> 
> ************************************************************************
> static int ipip6_err_gen_icmpv6_unreach(struct sk_buff *skb)
> {
>         int ihl = ((const struct iphdr *)skb->data)->ihl*4;
>         struct rt6_info *rt;
>         struct sk_buff *skb2;
> 
>         if (!pskb_may_pull(skb, ihl + sizeof(struct ipv6hdr) + 8))
>                 return 1;
> 
> // we clone the ipv4 skb in skb2 to prepare the icmpv6 packet
>         skb2 = skb_clone(skb, GFP_ATOMIC);
> 
>         if (!skb2)
>                 return 1;
> 
> // we clean up the cloned skb2
>         skb_dst_drop(skb2);
>         skb_pull(skb2, ihl);
> // The network header is reset
>         skb_reset_network_header(skb2);
> 
> //THE PROPOSED FIX: The following line is NOT in the current code
>         skb_reset_transport_header(skb2);

This will make your panic with PAX go away, but the offset to
the transport header is still wrong. Also, it is not just the
sit tunnel that has this problem. All ipv6 tunnels are affected.

I think the easiest is to fix it in _decode_session6().
Could you please try the patch below?

Subject: [PATCH] xfrm6: Fix transport header offset in _decode_session6.

skb->transport_header might not be valid when we do a reverse
decode because the ipv6 tunnel error handlers don't update it
to the inner transport header. This leads to a wrong offset
calculation and to wrong layer 4 informations. We fix this
by using the size of the ipv6 header as the first offset.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv6/xfrm6_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index ac49f84..576fc42 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -130,8 +130,8 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
 {
 	struct flowi6 *fl6 = &fl->u.ip6;
 	int onlyproto = 0;
-	u16 offset = skb_network_header_len(skb);
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
+	u16 offset = sizeof(*hdr);
 	struct ipv6_opt_hdr *exthdr;
 	const unsigned char *nh = skb_network_header(skb);
 	u8 nexthdr = nh[IP6CB(skb)->nhoff];
-- 
1.9.1

Re: ipip6 - integer underrun when handlince icmpv4 unreachable messages

[Alexander Wetzel]

Hello,

The patch works for me, thank you very much.

Rejecting the ipv4 tunel packets with icmp unreachable is producing the
expected "Destination unreachable: Address unreachable" messages for the
inside ipv6 client and everything works as expected. Without the call in
sit.c to skb_reset_transport_header.

I'll continue to use the patch on my ipv6 tunnel router but I'm sure it
works as intended.


On 04.12.2014 09:56, Steffen Klassert wrote:
> On Mon, Nov 17, 2014 at 10:52:04PM +0100, Alexander Wetzel wrote:
>> Hello netdev,
>>
>> the current code to translate icmpv4 "destination unreachable" packets
>> to icmpv6 is later generating an integer underrun when calling
>> icmpv6_send, by later calling skb_network_header_len and subtracting a
>> bigger number from a lower one.
> 
> I'd guess the call to skb_network_header_len() in _decode_session6()
> is the problematic one, right?
> 
>>
>> The issue is not visible for vanilla kernels and works correctly from
>> a user perspective, but a kernel with the PAX patches and enabled
>> "size overflow protection" will panic immediately when it's getting an
>> icmpv4 destination unreachable packet back for an encapsulated ipv6
>> packet. (Remote tunnel endpoint not reachable.)
>>
>> I think I've tracked the issue down and can show you the problem with
>> the code... as I understand it as non-programmer greping the sources
>> and googeling functions. I was even able to find a fix which passes
>> the functionality test, but I'm unqualified to rate the correctness of
>> it and so are reaching out to you for that.
>>
>> What happens (output of printk's) with transport_header and
>> network_header around "skb_reset_network_header" is described below
>> the function.
>>
>> Near the end of the mail there are two links to the gentoo bug tracker
>> and pax forum, suggesting to put this forward to the lkml/netdev for
>> review, also including more details on the panics.
>>
>> So here the function "ipip6_err_gen_icmpv6_unreach" from
>> net/ipv6/sit.c with some remarks and one new line which seems to fix
>> the problem:
>>
>> ************************************************************************
>> static int ipip6_err_gen_icmpv6_unreach(struct sk_buff *skb)
>> {
>>         int ihl = ((const struct iphdr *)skb->data)->ihl*4;
>>         struct rt6_info *rt;
>>         struct sk_buff *skb2;
>>
>>         if (!pskb_may_pull(skb, ihl + sizeof(struct ipv6hdr) + 8))
>>                 return 1;
>>
>> // we clone the ipv4 skb in skb2 to prepare the icmpv6 packet
>>         skb2 = skb_clone(skb, GFP_ATOMIC);
>>
>>         if (!skb2)
>>                 return 1;
>>
>> // we clean up the cloned skb2
>>         skb_dst_drop(skb2);
>>         skb_pull(skb2, ihl);
>> // The network header is reset
>>         skb_reset_network_header(skb2);
>>
>> //THE PROPOSED FIX: The following line is NOT in the current code
>>         skb_reset_transport_header(skb2);
> 
> This will make your panic with PAX go away, but the offset to
> the transport header is still wrong. Also, it is not just the
> sit tunnel that has this problem. All ipv6 tunnels are affected.
> 
> I think the easiest is to fix it in _decode_session6().
> Could you please try the patch below?
> 
> Subject: [PATCH] xfrm6: Fix transport header offset in _decode_session6.
> 
> skb->transport_header might not be valid when we do a reverse
> decode because the ipv6 tunnel error handlers don't update it
> to the inner transport header. This leads to a wrong offset
> calculation and to wrong layer 4 informations. We fix this
> by using the size of the ipv6 header as the first offset.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/ipv6/xfrm6_policy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
> index ac49f84..576fc42 100644
> --- a/net/ipv6/xfrm6_policy.c
> +++ b/net/ipv6/xfrm6_policy.c
> @@ -130,8 +130,8 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
>  {
>  	struct flowi6 *fl6 = &fl->u.ip6;
>  	int onlyproto = 0;
> -	u16 offset = skb_network_header_len(skb);
>  	const struct ipv6hdr *hdr = ipv6_hdr(skb);
> +	u16 offset = sizeof(*hdr);
>  	struct ipv6_opt_hdr *exthdr;
>  	const unsigned char *nh = skb_network_header(skb);
>  	u8 nexthdr = nh[IP6CB(skb)->nhoff];
> 

Re: ipip6 - integer underrun when handlince icmpv4 unreachable messages

[Steffen Klassert]

On Thu, Dec 04, 2014 at 05:22:52PM +0100, Alexander Wetzel wrote:
> Hello,
> 
> The patch works for me, thank you very much.
> 
> Rejecting the ipv4 tunel packets with icmp unreachable is producing the
> expected "Destination unreachable: Address unreachable" messages for the
> inside ipv6 client and everything works as expected. Without the call in
> sit.c to skb_reset_transport_header.
> 
> I'll continue to use the patch on my ipv6 tunnel router but I'm sure it
> works as intended.
> 
> 
> On 04.12.2014 09:56, Steffen Klassert wrote:
> > 
> > I think the easiest is to fix it in _decode_session6().
> > Could you please try the patch below?
> > 
> > Subject: [PATCH] xfrm6: Fix transport header offset in _decode_session6.
> > 
> > skb->transport_header might not be valid when we do a reverse
> > decode because the ipv6 tunnel error handlers don't update it
> > to the inner transport header. This leads to a wrong offset
> > calculation and to wrong layer 4 informations. We fix this
> > by using the size of the ipv6 header as the first offset.
> > 
> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Thanks for testing, I've applied this to the ipsec tree now.