Re: [PATCH net v5] ipv6: validate extension header length before copying to cmsg

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Qi Tang wrote:
> ip6_datagram_recv_specific_ctl() builds IPV6_{HOPOPTS,DSTOPTS,RTHDR}
> cmsgs (and their IPV6_2292* legacy counterparts) by trusting the
> on-wire hdrlen byte (ptr[1]) when computing the put_cmsg() length.
> The length was validated only at parse time (ipv6_parse_hopopts(),
> etc.).  An nftables payload-write expression can rewrite hdrlen after
> parsing and before the skb reaches recvmsg; the write itself is
> in-bounds but put_cmsg() then reads up to ((hdrlen+1) << 3) = 2040
> bytes from an 8-byte header.  nftables is reachable from an
> unprivileged user namespace, so this is an unprivileged
> slab-out-of-bounds read:
> 
>   BUG: KASAN: slab-out-of-bounds in put_cmsg+0x3ac/0x540
>    put_cmsg+0x3ac/0x540
>    udpv6_recvmsg+0xca0/0x1250
>    sock_recvmsg+0xdf/0x190
>    ____sys_recvmsg+0x1b1/0x620
> 
> Add ipv6_get_exthdr_len() which validates that at least two bytes
> are accessible before reading the hdrlen field, then checks the
> computed length against skb_tail_pointer(skb), returning 0 on
> failure.  Extension headers are kept in the linear skb area by
> pskb_may_pull() during input, so skb_tail_pointer() is the correct
> bound.
> 
> Use ipv6_get_exthdr_len() at all non-AH call sites: the five
> standalone cmsg blocks (HbH, 2292HbH, 2292DSTOPTS x2, 2292RTHDR)
> and the three standard cases in the extension-header walk loop
> (DSTOPTS, ROUTING, default).  AH retains an inline bounds check
> because its length formula differs ((ptr[1]+2)<<2).
> 
> The walk loop also gets a pre-read bounds check at the top to
> validate ptr before any case accesses ptr[0] or ptr[1].
> 
> When the walk loop detects a corrupted header, return from the
> function instead of continuing to process later socket options.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Qi Tang <tpluszz77@gmail.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
> Changes v4 -> v5 (Jakub Kicinski):
>   - Switch (ptr + len <= tail) to (len <= tail - ptr) form in
>     ipv6_get_exthdr_len() to avoid pointer arithmetic concerns.

Please do send the net-next patch replacing the open constants with
offsetof and such.


> @@ -664,26 +679,37 @@ void ip6_datagram_recv_specific_ctl(struct sock *sk, struct msghdr *msg,
>  			unsigned int len;
>  			u8 *ptr = nh + off;
>  
> +			if (ptr + 2 > skb_tail_pointer(skb))
> +				return;
> +
>  			switch (nexthdr) {
>  			case IPPROTO_DSTOPTS:
>  				nexthdr = ptr[0];
> -				len = (ptr[1] + 1) << 3;
> +				len = ipv6_get_exthdr_len(skb, ptr);
> +				if (!len)
> +					return;
>  				if (np->rxopt.bits.dstopts)
>  					put_cmsg(msg, SOL_IPV6, IPV6_DSTOPTS, len, ptr);
>  				break;
>  			case IPPROTO_ROUTING:
>  				nexthdr = ptr[0];
> -				len = (ptr[1] + 1) << 3;
> +				len = ipv6_get_exthdr_len(skb, ptr);
> +				if (!len)
> +					return;

Optional: instead of return, jump out of the while loop and continue
processing other cmsg not based on exthdrs.

>  				if (np->rxopt.bits.srcrt)
>  					put_cmsg(msg, SOL_IPV6, IPV6_RTHDR, len, ptr);
>  				break;
>  			case IPPROTO_AH:
>  				nexthdr = ptr[0];
>  				len = (ptr[1] + 2) << 2;
> +				if (ptr + len > skb_tail_pointer(skb))
> +					return;
>  				break;
>  			default:
>  				nexthdr = ptr[0];
> -				len = (ptr[1] + 1) << 3;
> +				len = ipv6_get_exthdr_len(skb, ptr);
> +				if (!len)
> +					return;
>  				break;
>  			}
>