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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <24258ef339a7470b84a28854390f1163@AcuMS.aculab.com>
Date:   Thu, 25 Apr 2019 14:35:46 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Willem de Bruijn' <willemdebruijn.kernel@...il.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

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!
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!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)