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

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

[Guillaume Nault]

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;
 
-- 
2.17.0

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

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Mon, 23 Apr 2018 16:38:27 +0200

> 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>

Applied and queued up for -stable, thank you.

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

[Kevin Easton]

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()).

    - Kevin

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

[Guillaume Nault]

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.

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

[Kevin Easton]

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?

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.

    - Kevin

> 

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

[Guillaume Nault]

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.

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

[Guillaume Nault]

On Fri, Apr 27, 2018 at 06:24:24PM +0200, Guillaume Nault wrote:
> exact consequences. That being said if you consider the risk is
> sufficiently low, you can always submit the patch to net-next.
Humm, forget it. I didn't see David's reply before answering.

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

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Fri, 27 Apr 2018 17:39:06 +0200

> 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.

Agreed, adding the check is too risky.