Re: [PATCH] ipv6: Prevent overrun when parsing v6 header options

[Yang Xiao]

Sorry, I don't get your point. Why is xfrm6_transport_output() buggy?
The point is that there would be out-of-bound access in
mip6_destopt_offset() and mip6_destopt_offset(), since there is no
sanity check for offset.

There is chance that offset + sizeof(struct ipv6_opt_hdr) > packet_len.

As described in CVE-2017-9074:  "The IPv6 fragmentation implementation
in the Linux kernel through 4.11.1 does not consider that the nexthdr
field may be associated with an invalid option, which allows local
users to cause a denial of service (out-of-bounds read and BUG)".

At the same time, there are bugs in  mip6_destopt_offset() and
mip6_destopt_offset(), which is similar to CVE-2017-7542.

On Sat, Jun 1, 2019 at 1:35 AM Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
>
>
>
> On 5/30/19 8:04 PM, Yang Xiao wrote:
> > On Fri, May 31, 2019 at 1:17 AM Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 5/30/19 8:28 AM, Young Xiao wrote:
> >>> The fragmentation code tries to parse the header options in order
> >>> to figure out where to insert the fragment option.  Since nexthdr points
> >>> to an invalid option, the calculation of the size of the network header
> >>> can made to be much larger than the linear section of the skb and data
> >>> is read outside of it.
> >>>
> >>> This vulnerability is similar to CVE-2017-9074.
> >>>
> >>> Signed-off-by: Young Xiao <92siuyang@xxxxxxxxx>
> >>> ---
> >>>  net/ipv6/mip6.c | 24 ++++++++++++++----------
> >>>  1 file changed, 14 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/net/ipv6/mip6.c b/net/ipv6/mip6.c
> >>> index 64f0f7b..30ed1c5 100644
> >>> --- a/net/ipv6/mip6.c
> >>> +++ b/net/ipv6/mip6.c
> >>> @@ -263,8 +263,6 @@ static int mip6_destopt_offset(struct xfrm_state *x, struct sk_buff *skb,
> >>>                              u8 **nexthdr)
> >>>  {
> >>>       u16 offset = sizeof(struct ipv6hdr);
> >>> -     struct ipv6_opt_hdr *exthdr =
> >>> -                                (struct ipv6_opt_hdr *)(ipv6_hdr(skb) + 1);
> >>>       const unsigned char *nh = skb_network_header(skb);
> >>>       unsigned int packet_len = skb_tail_pointer(skb) -
> >>>               skb_network_header(skb);
> >>> @@ -272,7 +270,8 @@ static int mip6_destopt_offset(struct xfrm_state *x, struct sk_buff *skb,
> >>>
> >>>       *nexthdr = &ipv6_hdr(skb)->nexthdr;
> >>>
> >>> -     while (offset + 1 <= packet_len) {
> >>> +     while (offset <= packet_len) {
> >>> +             struct ipv6_opt_hdr *exthdr;
> >>>
> >>>               switch (**nexthdr) {
> >>>               case NEXTHDR_HOP:
> >>> @@ -299,12 +298,15 @@ static int mip6_destopt_offset(struct xfrm_state *x, struct sk_buff *skb,
> >>>                       return offset;
> >>>               }
> >>>
> >>> +             if (offset + sizeof(struct ipv6_opt_hdr) > packet_len)
> >>> +                     return -EINVAL;
> >>> +
> >>> +             exthdr = (struct ipv6_opt_hdr *)(nh + offset);
> >>>               offset += ipv6_optlen(exthdr);
> >>>               *nexthdr = &exthdr->nexthdr;
> >>> -             exthdr = (struct ipv6_opt_hdr *)(nh + offset);
> >>>       }
> >>>
> >>> -     return offset;
> >>> +     return -EINVAL;
> >>>  }
> >>>
> >>
> >>
> >> Ok, but have you checked that callers have been fixed ?
> >
> > I've checked the callers. There are two callers:
> > xfrm6_transport_output() and xfrm6_ro_output(). There are checks in
> > both function.
> >
> > ------------------------------------------------------------------------------
> >         hdr_len = x->type->hdr_offset(x, skb, &prevhdr);
> >         if (hdr_len < 0)
> >                 return hdr_len;
> > ------------------------------------------------------------------------------
> >>
> >> xfrm6_transport_output() seems buggy as well,
> >> unless the skbs are linearized before entering these functions ?
> > I can not understand what you mean about this comment.
> > Could you explain it in more detail.
>
>
> If we had a problem, then the memmove(ipv6_hdr(skb), iph, hdr_len);
>  in xfrm6_transport_output() would be buggy, since iph could also point to freed memory.
>
>
>


-- 
Best regards!

Young
-----------------------------------------------------------