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.duma...@gmail.com> wrote:
>
>
>
> On 5/30/19 8:04 PM, Yang Xiao wrote:
> > On Fri, May 31, 2019 at 1:17 AM Eric Dumazet <eric.duma...@gmail.com> 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 <92siuy...@gmail.com>
> >>> ---
> >>> 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
-----------------------------------------------------------
