Re: [PATCH net] pppoe: check sockaddr length in pppoe_connect()

[Guillaume Nault <g.nault@alphalink.fr>]

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.