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 17:39:06 +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 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@...kaller.appspotmail.com
> > Signed-off-by: Guillaume Nault <g.nault@...halink.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.