| Message ID | 7301017039d68c920cb9120c035a1a0df3e6b30d.1557322358.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] selinux: do not report error on connect(AF_UNSPEC) | expand |
On Wed, May 08, 2019 at 03:32:51PM +0200, Paolo Abeni wrote: > calling connect(AF_UNSPEC) on an already connected TCP socket is an > established way to disconnect() such socket. After commit 68741a8adab9 > ("selinux: Fix ltp test connect-syscall failure") it no longer works > and, in the above scenario connect() fails with EAFNOSUPPORT. > > Fix the above falling back to the generic/old code when the address family > is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has > specific constraints. > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") > Reported-by: Tom Deseyn <tdeseyn@redhat.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > security/selinux/hooks.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index c61787b15f27..d82b87c16b0a 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4649,7 +4649,7 @@ static int selinux_socket_connect_helper(struct socket *sock, > struct lsm_network_audit net = {0,}; > struct sockaddr_in *addr4 = NULL; > struct sockaddr_in6 *addr6 = NULL; > - unsigned short snum; > + unsigned short snum = 0; > u32 sid, perm; > > /* sctp_connectx(3) calls via selinux_sctp_bind_connect() > @@ -4674,12 +4674,12 @@ static int selinux_socket_connect_helper(struct socket *sock, > break; > default: > /* Note that SCTP services expect -EINVAL, whereas > - * others expect -EAFNOSUPPORT. > + * others must handle this at the protocol level: > + * connect(AF_UNSPEC) on a connected socket is > + * a documented way disconnect the socket. > */ > if (sksec->sclass == SECCLASS_SCTP_SOCKET) > return -EINVAL; > - else > - return -EAFNOSUPPORT; > } > > err = sel_netport_sid(sk->sk_protocol, snum, &sid); > -- > 2.20.1 >
From: Paolo Abeni <pabeni@redhat.com> Date: Wed, 8 May 2019 15:32:51 +0200 > calling connect(AF_UNSPEC) on an already connected TCP socket is an > established way to disconnect() such socket. After commit 68741a8adab9 > ("selinux: Fix ltp test connect-syscall failure") it no longer works > and, in the above scenario connect() fails with EAFNOSUPPORT. > > Fix the above falling back to the generic/old code when the address family > is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has > specific constraints. > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") > Reported-by: Tom Deseyn <tdeseyn@redhat.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> Applied, and queued up for -stable, thanks!
On 5/8/19 9:32 AM, Paolo Abeni wrote: > calling connect(AF_UNSPEC) on an already connected TCP socket is an > established way to disconnect() such socket. After commit 68741a8adab9 > ("selinux: Fix ltp test connect-syscall failure") it no longer works > and, in the above scenario connect() fails with EAFNOSUPPORT. > > Fix the above falling back to the generic/old code when the address family > is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has > specific constraints. > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") > Reported-by: Tom Deseyn <tdeseyn@redhat.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > security/selinux/hooks.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index c61787b15f27..d82b87c16b0a 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4649,7 +4649,7 @@ static int selinux_socket_connect_helper(struct socket *sock, > struct lsm_network_audit net = {0,}; > struct sockaddr_in *addr4 = NULL; > struct sockaddr_in6 *addr6 = NULL; > - unsigned short snum; > + unsigned short snum = 0; > u32 sid, perm; > > /* sctp_connectx(3) calls via selinux_sctp_bind_connect() > @@ -4674,12 +4674,12 @@ static int selinux_socket_connect_helper(struct socket *sock, > break; > default: > /* Note that SCTP services expect -EINVAL, whereas > - * others expect -EAFNOSUPPORT. > + * others must handle this at the protocol level: > + * connect(AF_UNSPEC) on a connected socket is > + * a documented way disconnect the socket. > */ > if (sksec->sclass == SECCLASS_SCTP_SOCKET) > return -EINVAL; > - else > - return -EAFNOSUPPORT; I think we need to return 0 here. Otherwise, we'll fall through with an uninitialized snum, triggering a random/bogus permission check. > } > > err = sel_netport_sid(sk->sk_protocol, snum, &sid); >
On 5/8/19 2:12 PM, Stephen Smalley wrote: > On 5/8/19 9:32 AM, Paolo Abeni wrote: >> calling connect(AF_UNSPEC) on an already connected TCP socket is an >> established way to disconnect() such socket. After commit 68741a8adab9 >> ("selinux: Fix ltp test connect-syscall failure") it no longer works >> and, in the above scenario connect() fails with EAFNOSUPPORT. >> >> Fix the above falling back to the generic/old code when the address >> family >> is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has >> specific constraints. >> >> Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") >> Reported-by: Tom Deseyn <tdeseyn@redhat.com> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >> --- >> security/selinux/hooks.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index c61787b15f27..d82b87c16b0a 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -4649,7 +4649,7 @@ static int selinux_socket_connect_helper(struct >> socket *sock, >> struct lsm_network_audit net = {0,}; >> struct sockaddr_in *addr4 = NULL; >> struct sockaddr_in6 *addr6 = NULL; >> - unsigned short snum; >> + unsigned short snum = 0; >> u32 sid, perm; >> /* sctp_connectx(3) calls via selinux_sctp_bind_connect() >> @@ -4674,12 +4674,12 @@ static int >> selinux_socket_connect_helper(struct socket *sock, >> break; >> default: >> /* Note that SCTP services expect -EINVAL, whereas >> - * others expect -EAFNOSUPPORT. >> + * others must handle this at the protocol level: >> + * connect(AF_UNSPEC) on a connected socket is >> + * a documented way disconnect the socket. >> */ >> if (sksec->sclass == SECCLASS_SCTP_SOCKET) >> return -EINVAL; >> - else >> - return -EAFNOSUPPORT; > > I think we need to return 0 here. Otherwise, we'll fall through with an > uninitialized snum, triggering a random/bogus permission check. Sorry, I see that you initialize snum above. Nonetheless, I think the correct behavior here is to skip the check since this is a disconnect, not a connect. > >> } >> err = sel_netport_sid(sk->sk_protocol, snum, &sid); >> >
On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote: > On 5/8/19 2:12 PM, Stephen Smalley wrote: > > On 5/8/19 9:32 AM, Paolo Abeni wrote: > > > calling connect(AF_UNSPEC) on an already connected TCP socket is an > > > established way to disconnect() such socket. After commit 68741a8adab9 > > > ("selinux: Fix ltp test connect-syscall failure") it no longer works > > > and, in the above scenario connect() fails with EAFNOSUPPORT. > > > > > > Fix the above falling back to the generic/old code when the address > > > family > > > is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has > > > specific constraints. > > > > > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") > > > Reported-by: Tom Deseyn <tdeseyn@redhat.com> > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > --- > > > security/selinux/hooks.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index c61787b15f27..d82b87c16b0a 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -4649,7 +4649,7 @@ static int > > > selinux_socket_connect_helper(struct socket *sock, > > > struct lsm_network_audit net = {0,}; > > > struct sockaddr_in *addr4 = NULL; > > > struct sockaddr_in6 *addr6 = NULL; > > > - unsigned short snum; > > > + unsigned short snum = 0; > > > u32 sid, perm; > > > /* sctp_connectx(3) calls via selinux_sctp_bind_connect() > > > @@ -4674,12 +4674,12 @@ static int > > > selinux_socket_connect_helper(struct socket *sock, > > > break; > > > default: > > > /* Note that SCTP services expect -EINVAL, whereas > > > - * others expect -EAFNOSUPPORT. > > > + * others must handle this at the protocol level: > > > + * connect(AF_UNSPEC) on a connected socket is > > > + * a documented way disconnect the socket. > > > */ > > > if (sksec->sclass == SECCLASS_SCTP_SOCKET) > > > return -EINVAL; > > > - else > > > - return -EAFNOSUPPORT; > > > > I think we need to return 0 here. Otherwise, we'll fall through with an > > uninitialized snum, triggering a random/bogus permission check. > > Sorry, I see that you initialize snum above. Nonetheless, I think the > correct behavior here is to skip the check since this is a disconnect, not a > connect. Skipping the check would make it less controllable. So should it somehow re-use shutdown() stuff? It gets very confusing, and after all, it still is, in essence, a connect() syscall. > > > > > > } > > > err = sel_netport_sid(sk->sk_protocol, snum, &sid); > > > > > >
On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote: > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote: >> On 5/8/19 2:12 PM, Stephen Smalley wrote: >>> On 5/8/19 9:32 AM, Paolo Abeni wrote: >>>> calling connect(AF_UNSPEC) on an already connected TCP socket is an >>>> established way to disconnect() such socket. After commit 68741a8adab9 >>>> ("selinux: Fix ltp test connect-syscall failure") it no longer works >>>> and, in the above scenario connect() fails with EAFNOSUPPORT. >>>> >>>> Fix the above falling back to the generic/old code when the address >>>> family >>>> is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has >>>> specific constraints. >>>> >>>> Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") >>>> Reported-by: Tom Deseyn <tdeseyn@redhat.com> >>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >>>> --- >>>> security/selinux/hooks.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>> index c61787b15f27..d82b87c16b0a 100644 >>>> --- a/security/selinux/hooks.c >>>> +++ b/security/selinux/hooks.c >>>> @@ -4649,7 +4649,7 @@ static int >>>> selinux_socket_connect_helper(struct socket *sock, >>>> struct lsm_network_audit net = {0,}; >>>> struct sockaddr_in *addr4 = NULL; >>>> struct sockaddr_in6 *addr6 = NULL; >>>> - unsigned short snum; >>>> + unsigned short snum = 0; >>>> u32 sid, perm; >>>> /* sctp_connectx(3) calls via selinux_sctp_bind_connect() >>>> @@ -4674,12 +4674,12 @@ static int >>>> selinux_socket_connect_helper(struct socket *sock, >>>> break; >>>> default: >>>> /* Note that SCTP services expect -EINVAL, whereas >>>> - * others expect -EAFNOSUPPORT. >>>> + * others must handle this at the protocol level: >>>> + * connect(AF_UNSPEC) on a connected socket is >>>> + * a documented way disconnect the socket. >>>> */ >>>> if (sksec->sclass == SECCLASS_SCTP_SOCKET) >>>> return -EINVAL; >>>> - else >>>> - return -EAFNOSUPPORT; >>> >>> I think we need to return 0 here. Otherwise, we'll fall through with an >>> uninitialized snum, triggering a random/bogus permission check. >> >> Sorry, I see that you initialize snum above. Nonetheless, I think the >> correct behavior here is to skip the check since this is a disconnect, not a >> connect. > > Skipping the check would make it less controllable. So should it > somehow re-use shutdown() stuff? It gets very confusing, and after > all, it still is, in essence, a connect() syscall. The function checks CONNECT permission on entry, before reaching this point. This logic is only in preparation for a further check (NAME_CONNECT) on the port. In this case, there is no further check to perform and we can just return. > >> >>> >>>> } >>>> err = sel_netport_sid(sk->sk_protocol, snum, &sid); >>>> >>> >>
On Wed, May 8, 2019 at 2:55 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote: > > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote: > >> On 5/8/19 2:12 PM, Stephen Smalley wrote: > >>> On 5/8/19 9:32 AM, Paolo Abeni wrote: > >>>> calling connect(AF_UNSPEC) on an already connected TCP socket is an > >>>> established way to disconnect() such socket. After commit 68741a8adab9 > >>>> ("selinux: Fix ltp test connect-syscall failure") it no longer works > >>>> and, in the above scenario connect() fails with EAFNOSUPPORT. > >>>> > >>>> Fix the above falling back to the generic/old code when the address > >>>> family > >>>> is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has > >>>> specific constraints. > >>>> > >>>> Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") > >>>> Reported-by: Tom Deseyn <tdeseyn@redhat.com> > >>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com> > >>>> --- > >>>> security/selinux/hooks.c | 8 ++++---- > >>>> 1 file changed, 4 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >>>> index c61787b15f27..d82b87c16b0a 100644 > >>>> --- a/security/selinux/hooks.c > >>>> +++ b/security/selinux/hooks.c > >>>> @@ -4649,7 +4649,7 @@ static int > >>>> selinux_socket_connect_helper(struct socket *sock, > >>>> struct lsm_network_audit net = {0,}; > >>>> struct sockaddr_in *addr4 = NULL; > >>>> struct sockaddr_in6 *addr6 = NULL; > >>>> - unsigned short snum; > >>>> + unsigned short snum = 0; > >>>> u32 sid, perm; > >>>> /* sctp_connectx(3) calls via selinux_sctp_bind_connect() > >>>> @@ -4674,12 +4674,12 @@ static int > >>>> selinux_socket_connect_helper(struct socket *sock, > >>>> break; > >>>> default: > >>>> /* Note that SCTP services expect -EINVAL, whereas > >>>> - * others expect -EAFNOSUPPORT. > >>>> + * others must handle this at the protocol level: > >>>> + * connect(AF_UNSPEC) on a connected socket is > >>>> + * a documented way disconnect the socket. > >>>> */ > >>>> if (sksec->sclass == SECCLASS_SCTP_SOCKET) > >>>> return -EINVAL; > >>>> - else > >>>> - return -EAFNOSUPPORT; > >>> > >>> I think we need to return 0 here. Otherwise, we'll fall through with an > >>> uninitialized snum, triggering a random/bogus permission check. > >> > >> Sorry, I see that you initialize snum above. Nonetheless, I think the > >> correct behavior here is to skip the check since this is a disconnect, not a > >> connect. > > > > Skipping the check would make it less controllable. So should it > > somehow re-use shutdown() stuff? It gets very confusing, and after > > all, it still is, in essence, a connect() syscall. > > The function checks CONNECT permission on entry, before reaching this > point. This logic is only in preparation for a further check > (NAME_CONNECT) on the port. In this case, there is no further check to > perform and we can just return. I agree with Stephen, in the connect(AF_UNSPEC) case the right thing to do is to simply return with no error. I would also suggest that since this patch only touches the SELinux code it really should go in via the SELinux tree and not netdev; this will help avoid merge conflicts in the linux-next tree and during the merge window. I think the right thing to do at this point is to create a revert patch (or have DaveM do it, I'm not sure what he prefers in situations like this) for this commit, make the adjustments that Stephen mentioned and submit them for the SELinux tree. Otherwise, thanks for catching this and submitting a fix :)
On Wed, 2019-05-08 at 17:17 -0400, Paul Moore wrote: > On Wed, May 8, 2019 at 2:55 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote: > > > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote: > > > > On 5/8/19 2:12 PM, Stephen Smalley wrote: > > > > > On 5/8/19 9:32 AM, Paolo Abeni wrote: > > > > > > calling connect(AF_UNSPEC) on an already connected TCP socket is an > > > > > > established way to disconnect() such socket. After commit 68741a8adab9 > > > > > > ("selinux: Fix ltp test connect-syscall failure") it no longer works > > > > > > and, in the above scenario connect() fails with EAFNOSUPPORT. > > > > > > > > > > > > Fix the above falling back to the generic/old code when the address > > > > > > family > > > > > > is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has > > > > > > specific constraints. > > > > > > > > > > > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") > > > > > > Reported-by: Tom Deseyn <tdeseyn@redhat.com> > > > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > > > > --- > > > > > > security/selinux/hooks.c | 8 ++++---- > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > > > index c61787b15f27..d82b87c16b0a 100644 > > > > > > --- a/security/selinux/hooks.c > > > > > > +++ b/security/selinux/hooks.c > > > > > > @@ -4649,7 +4649,7 @@ static int > > > > > > selinux_socket_connect_helper(struct socket *sock, > > > > > > struct lsm_network_audit net = {0,}; > > > > > > struct sockaddr_in *addr4 = NULL; > > > > > > struct sockaddr_in6 *addr6 = NULL; > > > > > > - unsigned short snum; > > > > > > + unsigned short snum = 0; > > > > > > u32 sid, perm; > > > > > > /* sctp_connectx(3) calls via selinux_sctp_bind_connect() > > > > > > @@ -4674,12 +4674,12 @@ static int > > > > > > selinux_socket_connect_helper(struct socket *sock, > > > > > > break; > > > > > > default: > > > > > > /* Note that SCTP services expect -EINVAL, whereas > > > > > > - * others expect -EAFNOSUPPORT. > > > > > > + * others must handle this at the protocol level: > > > > > > + * connect(AF_UNSPEC) on a connected socket is > > > > > > + * a documented way disconnect the socket. > > > > > > */ > > > > > > if (sksec->sclass == SECCLASS_SCTP_SOCKET) > > > > > > return -EINVAL; > > > > > > - else > > > > > > - return -EAFNOSUPPORT; > > > > > > > > > > I think we need to return 0 here. Otherwise, we'll fall through with an > > > > > uninitialized snum, triggering a random/bogus permission check. > > > > > > > > Sorry, I see that you initialize snum above. Nonetheless, I think the > > > > correct behavior here is to skip the check since this is a disconnect, not a > > > > connect. > > > > > > Skipping the check would make it less controllable. So should it > > > somehow re-use shutdown() stuff? It gets very confusing, and after > > > all, it still is, in essence, a connect() syscall. > > > > The function checks CONNECT permission on entry, before reaching this > > point. This logic is only in preparation for a further check > > (NAME_CONNECT) on the port. In this case, there is no further check to > > perform and we can just return. > > I agree with Stephen, in the connect(AF_UNSPEC) case the right thing > to do is to simply return with no error. The 'default:' case is catching any address family other than INET{4,6}, but I guess you argument still applies - selinux should not do name check for unknown protocols ?!? > I would also suggest that since this patch only touches the SELinux > code it really should go in via the SELinux tree and not netdev; this > will help avoid merge conflicts in the linux-next tree and during the > merge window. I think the right thing to do at this point is to > create a revert patch (or have DaveM do it, I'm not sure what he > prefers in situations like this) for this commit, make the adjustments > that Stephen mentioned and submit them for the SELinux tree. Sorry, my fault, I sent the email to both MLs for more awareness, I should have used a different subject prefix. @DaveM: if it's ok for you, I'll send a revert for this on netdev and I'll send a v2 via the selinux ML, please let me know! Thank you, Paolo
On Thu, May 9, 2019 at 4:40 AM Paolo Abeni <pabeni@redhat.com> wrote: > On Wed, 2019-05-08 at 17:17 -0400, Paul Moore wrote: > > On Wed, May 8, 2019 at 2:55 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > > On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote: > > > > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote: > > > > > On 5/8/19 2:12 PM, Stephen Smalley wrote: > > > > > > On 5/8/19 9:32 AM, Paolo Abeni wrote: > > > > > > > calling connect(AF_UNSPEC) on an already connected TCP socket is an > > > > > > > established way to disconnect() such socket. After commit 68741a8adab9 > > > > > > > ("selinux: Fix ltp test connect-syscall failure") it no longer works > > > > > > > and, in the above scenario connect() fails with EAFNOSUPPORT. > > > > > > > > > > > > > > Fix the above falling back to the generic/old code when the address > > > > > > > family > > > > > > > is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has > > > > > > > specific constraints. > > > > > > > > > > > > > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") > > > > > > > Reported-by: Tom Deseyn <tdeseyn@redhat.com> > > > > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > > > > > --- > > > > > > > security/selinux/hooks.c | 8 ++++---- > > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > > > > index c61787b15f27..d82b87c16b0a 100644 > > > > > > > --- a/security/selinux/hooks.c > > > > > > > +++ b/security/selinux/hooks.c > > > > > > > @@ -4649,7 +4649,7 @@ static int > > > > > > > selinux_socket_connect_helper(struct socket *sock, > > > > > > > struct lsm_network_audit net = {0,}; > > > > > > > struct sockaddr_in *addr4 = NULL; > > > > > > > struct sockaddr_in6 *addr6 = NULL; > > > > > > > - unsigned short snum; > > > > > > > + unsigned short snum = 0; > > > > > > > u32 sid, perm; > > > > > > > /* sctp_connectx(3) calls via selinux_sctp_bind_connect() > > > > > > > @@ -4674,12 +4674,12 @@ static int > > > > > > > selinux_socket_connect_helper(struct socket *sock, > > > > > > > break; > > > > > > > default: > > > > > > > /* Note that SCTP services expect -EINVAL, whereas > > > > > > > - * others expect -EAFNOSUPPORT. > > > > > > > + * others must handle this at the protocol level: > > > > > > > + * connect(AF_UNSPEC) on a connected socket is > > > > > > > + * a documented way disconnect the socket. > > > > > > > */ > > > > > > > if (sksec->sclass == SECCLASS_SCTP_SOCKET) > > > > > > > return -EINVAL; > > > > > > > - else > > > > > > > - return -EAFNOSUPPORT; > > > > > > > > > > > > I think we need to return 0 here. Otherwise, we'll fall through with an > > > > > > uninitialized snum, triggering a random/bogus permission check. > > > > > > > > > > Sorry, I see that you initialize snum above. Nonetheless, I think the > > > > > correct behavior here is to skip the check since this is a disconnect, not a > > > > > connect. > > > > > > > > Skipping the check would make it less controllable. So should it > > > > somehow re-use shutdown() stuff? It gets very confusing, and after > > > > all, it still is, in essence, a connect() syscall. > > > > > > The function checks CONNECT permission on entry, before reaching this > > > point. This logic is only in preparation for a further check > > > (NAME_CONNECT) on the port. In this case, there is no further check to > > > perform and we can just return. > > > > I agree with Stephen, in the connect(AF_UNSPEC) case the right thing > > to do is to simply return with no error. > > The 'default:' case is catching any address family other than > INET{4,6}, but I guess you argument still applies - selinux should not > do name check for unknown protocols ?!? If the code doesn't understand how to parse the port/"name" info it can't really do a useful name_connect check, this is why we return -EAFNOSUPPORT in the default case (or -EINVAL in the case of SCTP). However, the connect/AF_UNSPEC case is a bit of a special case and as such I probably needs special handling. My initial thinking is that we should do the AF_UNSPEC check immediately after the sock_has_perm() check in selinux_socket_connect_helper(): err = sock_has_perm(sk, SOCKET__CONNECT); if (err) return err; if (addrlen < offsetofend(struct sockaddr, sa_family)) return -EINVAL; if (address->sa_family == AF_UNSPEC) return 0; ... we can then remove the addrlen check from inside the TCP/DCCP/SCTP if-true block later in the function. There is the downside the we are now adding some additional code that executes for each connect() call (as opposed to just TCP/DCCP/SCTP), but this seems much cleaner from a conceptual point of view and I expect the overhead to be in the "unmeasurable" range. > > I would also suggest that since this patch only touches the SELinux > > code it really should go in via the SELinux tree and not netdev; this > > will help avoid merge conflicts in the linux-next tree and during the > > merge window. I think the right thing to do at this point is to > > create a revert patch (or have DaveM do it, I'm not sure what he > > prefers in situations like this) for this commit, make the adjustments > > that Stephen mentioned and submit them for the SELinux tree. > > Sorry, my fault, I sent the email to both MLs for more awareness, I > should have used a different subject prefix. It's not a big deal for patches this small, but since you're going to respin this patch anyway I figured it would be worth mentioning. Also, I have no object to posting to multiple MLs when appropriate (it seems appropriate here); I think the problem here was the "[PATCH net]" which caused DaveM to pull it into his tree. > @DaveM: if it's ok for you, I'll send a revert for this on netdev and > I'll send a v2 via the selinux ML, please let me know!
From: Paolo Abeni <pabeni@redhat.com> Date: Thu, 09 May 2019 10:40:40 +0200 > @DaveM: if it's ok for you, I'll send a revert for this on netdev and > I'll send a v2 via the selinux ML, please let me know! Sure.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index c61787b15f27..d82b87c16b0a 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4649,7 +4649,7 @@ static int selinux_socket_connect_helper(struct socket *sock, struct lsm_network_audit net = {0,}; struct sockaddr_in *addr4 = NULL; struct sockaddr_in6 *addr6 = NULL; - unsigned short snum; + unsigned short snum = 0; u32 sid, perm; /* sctp_connectx(3) calls via selinux_sctp_bind_connect() @@ -4674,12 +4674,12 @@ static int selinux_socket_connect_helper(struct socket *sock, break; default: /* Note that SCTP services expect -EINVAL, whereas - * others expect -EAFNOSUPPORT. + * others must handle this at the protocol level: + * connect(AF_UNSPEC) on a connected socket is + * a documented way disconnect the socket. */ if (sksec->sclass == SECCLASS_SCTP_SOCKET) return -EINVAL; - else - return -EAFNOSUPPORT; } err = sel_netport_sid(sk->sk_protocol, snum, &sid);
calling connect(AF_UNSPEC) on an already connected TCP socket is an established way to disconnect() such socket. After commit 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") it no longer works and, in the above scenario connect() fails with EAFNOSUPPORT. Fix the above falling back to the generic/old code when the address family is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has specific constraints. Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure") Reported-by: Tom Deseyn <tdeseyn@redhat.com> Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- security/selinux/hooks.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)