netdev - Re: [PATCH net] packet: validate address length if non-zero

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Apr 2019 11:42:02 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "idosch@...sch.org" <idosch@...sch.org>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] packet: validate address length if non-zero

On Thu, Apr 25, 2019 at 10:35 AM David Laight <David.Laight@...lab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 25 April 2019 14:57
> ...
> > > I've just done a bit of software archaeology.
> > >
> > > Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code.
> > > So it is not surprising that old application code leaves it as zero.
> > >
> > > The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll).
> > > The receive code now sets:
> > >   msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
> > > For ethernet this changes the msg_namelen from 20 to 18.
> > > A side effect (no one has noticed for years) is that you can't send a reply
> > > by passing back the received address buffer.
> >
> > Great find, thanks. I hadn't thought of going back that far, but
> > clearly should in these legacy caller questions..
>
> Fortunately I didn't have to find the pre-git sources :-)
>
> > > Looking at it all again how about:
> > >         char *addr = NULL;
> > >         ...
> > >                         err = -EINVAL;
> > >                         if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
> > >                                 goto out;
> > >                         proto = saddr->sll_protocol;
> > >                         dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
> > >                         if (dev && sock->type == SOCK_DGRAM) {
> > >                                 if (msg->msg_namelen < dev->addr_len + offsetof(struct sockaddr_ll, sll_addr))
> > >                                         goto out_unlock;
> > >                                 addr = saddr->sll_addr;
> > >                         }
> >
> > Yes, given the above, this looks great to me.
> >
> > In general I'm hesitant to loosen interface constraints. As you can
> > never tighten them again.
>
> Indeed.
>
> > But given the change on recv, it seems
> > correct here. That is technically a separate issue, so worth a
> > separate patch, I think. If that's not too pedantic. Else at least an
> > extra Fixes tag.
>
> Or leave the above using 'sizeof' and change the receive code to pad the
> address to sizeof struct sockaddr_ll.
> Which is definitely a completely different fix.
> The rx code seems to be:
>
>                 if (sock->type == SOCK_PACKET) {
>                         __sockaddr_check_size(sizeof(struct sockaddr_pkt));
>                         msg->msg_namelen = sizeof(struct sockaddr_pkt);
>                 } else {
>                         struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;
>
>                         msg->msg_namelen = sll->sll_halen +
>                                 offsetof(struct sockaddr_ll, sll_addr);
>                 }
>                 memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa,
>                        msg->msg_namelen);
>
> Hopefully the buffer is always big enough!

The kernel buffer is always sockaddr_storage with _K_SS_MAXSIZE or
128B. Before copying into skb->cb[] there is the
sock_skb_cb_check_size BUILD_BUG_ON based on the assumption that every
lladdr len <= MAX_ADDR_LEN (32B). As far as I can telll, that does
seem to hold (INFINIBAND_ALEN being the largest at 32). So while there
is no explicit check on the memcpy from skb->cb into msg_name, this is
safe.

> Assuming that the code that sets up the address zaps the last two bytes
> the SOCK_DGRAM side just needs a max(, sizeof (struct sockaddr_ll)).
> Zeroing the 8 byte field before the mac address is put into it is cheap
> (one 64bit write on 64bit systems).
>
> I guess this would have 'Fixes' tag for the 2.6.14 git tag!

The uaddr_len change is commit 0fb375fb9b93 ("[AF_PACKET]: Allow for >
8 byte hardware addresses.")?

That both introduced the recv change to return a length based on
dev->addr_len even the total is if below sizeof(struct sockaddr_ll)

-       else
-               msg->msg_namelen = sizeof(struct sockaddr_ll);
[..]
+       else
+               msg->msg_namelen = sll->sll_halen + offsetof(struct
sockaddr_ll, sll_addr);


and the new sendmsg check

                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;
+               if (msg->msg_namelen < (saddr->sll_halen +
offsetof(struct sockaddr_ll, sll_addr)))
+                       goto out;

but without removing the preexisting check above it, thus causing the
recvfrom () followed by sendto() to fail.