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: <62f0a1c528b04e9eaa863179e227165a@AcuMS.aculab.com>
Date:   Tue, 23 Apr 2019 15:53:50 +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: 23 April 2019 16:08
> On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@...lab.com> wrote:
> >
> > From: Willem de Bruijn
> > > Sent: 22 December 2018 21:54
> > > Validate packet socket address length if a length is given. Zero
> > > length is equivalent to not setting an address.
> > >
> > > Fixes: 99137b7888f4 ("packet: validate address length")
> > > Reported-by: Ido Schimmel <idosch@...sch.org>
> > > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > > ---
> > >  net/packet/af_packet.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > >                                               sll_addr)))
> > >                       goto out;
> > >               proto   = saddr->sll_protocol;
> > > -             addr    = saddr->sll_addr;
> > > +             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
> > >               dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
> > >               if (addr && dev && saddr->sll_halen < dev->addr_len)
> > >                       goto out;
> > > @@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> > >               if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
> > >                       goto out;
> > >               proto   = saddr->sll_protocol;
> > > -             addr    = saddr->sll_addr;
> > > +             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
> > >               dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
> > >               if (addr && dev && saddr->sll_halen < dev->addr_len)
> > >                       goto out;
> > > --
> > > 2.20.1.415.g653613c723-goog
> >
> > We've just discovered the combination of this patch and the one it 'fixes'
> > breaks some of our userspace code.
> >
> > Prior to these changes it didn't matter if code using AF_PACKET to
> > send ethernet frames on a specific 'ethertype' failed to set sll_addr.
> > Everything assumed it would be 6 - and the packets were sent.
> >
> > With both changes you get a -EINVAL return from somewhere.
> > I can fix our code, but I doubt it is the only code affected.
> > Other people are likely to have copied the same example.
> 
> Thanks for the report.
> 
> Usage trumps correctness. But this seems to be a case of damned if you
> do, damned if you don't.
> 
> Syzbot found a real use case of reading beyond the end of
> msg->msg_namelen, since that is checked against
> 
>                 if (msg->msg_namelen < (saddr->sll_halen +
> offsetof(struct sockaddr_ll, sll_addr)))
> 
> Just assuming that address length is dev->addr_len allows an
> ns_capable root to build link layer packets with address set to
> uninitialized data.
> 
> Ethernet is not the most problematic link layer. Indeed, since
> ETH_ALEN < sizeof(sll_addr), the previous check
> 
>                 if (msg->msg_namelen < sizeof(struct sockaddr_ll))
> 
> Will be sufficient in this case. The syzbot report was on a device of
> type ip6gre, with addr_len sizeof(struct in6_addr).
> 
> So I can refine to only perform the check on protocols with addr_len
> >= sizeof(sll_addr), excluding Ethernet.

Maybe something like:
		addr    = saddr->sll_addr;
		dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
		if (dev && msg->msg_namelen < (dev->addr_len
					+ offsetof(struct sockaddr_ll,
						sll_addr)))
			/* Don't read address from beyond the end of the buffer */
			goto out;

So it just checks that all of the address is in the buffer passed
from the user.

Although I'd write those tests in a different order:
		if (dev && offsetof(struct sockaddr_ll, sll_addr) + dev->addr_len > msg->msg_namelen)
			goto out;

FWIW the relevant application code has been in our cvs tree for 13 years.
Mark Brown might remember where he stole it from :-)

	David

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