netdev - Re: [PATCH net] pppoe: check sockaddr length in pppoe

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:   Fri, 27 Apr 2018 18:24:24 +0200
From:   Guillaume Nault <g.nault@...halink.fr>
To:     Kevin Easton <kevin@...rana.org>
Cc:     netdev@...r.kernel.org, Michal Ostrowski <mostrows@...thlink.net>
Subject: Re: [PATCH net] pppoe: check sockaddr length in pppoe_connect()

On Fri, Apr 27, 2018 at 11:51:31AM -0400, Kevin Easton wrote:
> On Fri, Apr 27, 2018 at 05:39:06PM +0200, Guillaume Nault wrote:
> > On Fri, Apr 27, 2018 at 08:23:16AM -0400, Kevin Easton wrote:
> ...
> > > There's another bug here - pppoe_connect() should also be validating
> > > sp->sa_family.  My suggested patch was going to be:
> > > 
> > > diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> > > index 1483bc7..90eb3fd 100644
> > > --- a/drivers/net/ppp/pppoe.c
> > > +++ b/drivers/net/ppp/pppoe.c
> > > @@ -620,6 +620,14 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
> > >         lock_sock(sk);
> > >  
> > >         error = -EINVAL;
> > > +       if (sockaddr_len < sizeof(struct sockaddr_pppox))
> > > +               goto end;
> > > +
> > > +       error = -EAFNOSUPPORT;
> > > +       if (sp->sa_family != AF_PPPOX)
> > > +               goto end;
> > > +
> > > +       error = -EINVAL;
> > >         if (sp->sa_protocol != PX_PROTO_OE)
> > >                 goto end;
> > >  
> > > Should I rework this on top of net.git HEAD?
> > > 
> > > (The same applies to pppol2tp_connect()).
> > > 
> > Thanks for the suggestion. But ->sa_family has never been checked.
> > Therefore, it has always been possible to connect a PPPoE or L2TP
> > socket with an invalid .sa_family field. I'd be surprised if there were
> > implementations relying on that, but we never know (for example, an
> > implementation could send this field uninitialised). By being stricter
> > we'd break such programs. And we don't need this field in the
> > connection process, so not checking its value doesn't harm.
> > 
> > I'm all for being strict and validating user-provided data as much as
> > possible, but I'm afraid its too late in this case.
> 
> Doesn't the same apply to supplying a bogus sockaddr_len?
> 
No, because we depend on sockaddr_len for correctly interpreting the
sockaddr structure. The original bug was that 'uservaddr' was smaller
than struct sockaddr_pppox. Therefore, attempts to access some of its
fields resulted in invalid pointer dereferences.


> I did test the rp-pppoe plugin for pppd with this patch - it does
> correctly set both the sa_family and sockaddr_len.  Checking on
> Debian's codesearch also showed that everything in that corpus
> that uses PX_PROTO_OE also sets AF_PPPOX.
> 
Yes, I'm pretty sure all these softwares are fine. But who knows what
other, unpublished, implementations may be doing? Modifying user-facing
behaviour is generally frowned upon because there's no way to know the
exact consequences. That being said if you consider the risk is
sufficiently low, you can always submit the patch to net-next.