| Message ID | 387ca48810af36f2626049008a795d1adc375cb8.1524494257.git.g.nault@alphalink.fr |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] pppoe: check sockaddr length in pppoe_connect() | expand |
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.
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
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.
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 >
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.
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.
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.
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;
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(+)