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

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

On Fri, Apr 27, 2018 at 08:23:16AM -0400, Kevin Easton wrote:
> On Mon, Apr 23, 2018 at 04:38:27PM +0200, Guillaume Nault wrote:
> > We must validate sockaddr_len, otherwise userspace can pass fewer data
> > than we expect and we end up accessing invalid data.
> > 
> > Fixes: 224cf5ad14c0 ("ppp: Move the PPP drivers")
> > Reported-by: syzbot+4f03bdf92fdf9ef5ddab@syzkaller.appspotmail.com
> > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
> > ---
> >  drivers/net/ppp/pppoe.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> > index 1483bc7b01e1..7df07337d69c 100644
> > --- a/drivers/net/ppp/pppoe.c
> > +++ b/drivers/net/ppp/pppoe.c
> > @@ -620,6 +620,10 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
> >  	lock_sock(sk);
> >  
> >  	error = -EINVAL;
> > +
> > +	if (sockaddr_len != sizeof(struct sockaddr_pppox))
> > +		goto end;
> > +
> >  	if (sp->sa_protocol != PX_PROTO_OE)
> >  		goto end;
> 
> 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.