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]
Date:   Mon, 29 Apr 2019 08:53:06 -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 msg_namelen in send directly

On Mon, Apr 29, 2019 at 5:00 AM David Laight <David.Laight@...lab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 26 April 2019 20:28
> > Packet sockets in datagram mode take a destination address. Verify its
> > length before passing to dev_hard_header.
> >
> > Prior to 2.6.14-rc3, the send code ignored sll_halen. This is
> > established behavior. Directly compare msg_namelen to dev->addr_len.
> >
> > Fixes: 6b8d95f1795c4 ("packet: validate address length if non-zero")
> > Suggested-by: David Laight <David.Laight@...lab.com>
> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > ---
> >  net/packet/af_packet.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 9419c5cf4de5e..13301e36b4a28 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -2624,10 +2624,13 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >                                               sll_addr)))
> >                       goto out;
> >               proto   = saddr->sll_protocol;
> > -             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_put;
> > +             if (po->sk.sk_socket->type == SOCK_DGRAM) {
> > +                     addr = saddr->sll_addr;
> > +                     if (dev && msg->msg_namelen < dev->addr_len +
> > +                                     offsetof(struct sockaddr_ll, sll_addr))
> > +                             goto out_put;
> > +             }
>
> IIRC you need to initialise 'addr - NULL' at the top of the functions.
> I'm surprised the compiler doesn't complain.

It did complain when I moved it below the if (dev && ..) branch. But
inside a branch with exactly the same condition as the one where used,
the compiler did figure it out. Admittedly that is fragile. Then it
might be simplest to restore the unconditional assignment

                proto   = saddr->sll_protocol;
+               addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);

Powered by blists - more mailing lists