| Message ID | 34870696b95f9cf48b5436df46e27dddd054858c.1557492319.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] selinux: do not report error on connect(AF_UNSPEC) | expand |
On Fri, May 10, 2019 at 9:49 AM Paolo Abeni <pabeni@redhat.com> 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 skipping the checks when the address family is not > AF_INET{4,6} - we don't have any port to validate, 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> > --- > v1 -> v2: > - avoid validation for AF_UNSPEC > --- > security/selinux/hooks.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) What was wrong with explicitly checking for AF_UNSPEC as I mentioned in my last email? > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index c61787b15f27..bccc4b3e6f57 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4674,12 +4674,13 @@ 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; > + return 0; > } > > err = sel_netport_sid(sk->sk_protocol, snum, &sid); > -- > 2.20.1
On Fri, 2019-05-10 at 11:32 -0400, Paul Moore wrote: > On Fri, May 10, 2019 at 9:49 AM Paolo Abeni <pabeni@redhat.com> 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 skipping the checks when the address family is not > > AF_INET{4,6} - we don't have any port to validate, 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> > > --- > > v1 -> v2: > > - avoid validation for AF_UNSPEC > > --- > > security/selinux/hooks.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > What was wrong with explicitly checking for AF_UNSPEC as I mentioned > in my last email? Whoops sorry, I missed a relevant part of that email. Reading it now. To me, the 2 options look quite similar, and I have a slighter preference for this one, being a smaller change possibly more suited to a stable fix. But if you have strong different opinions I can post the code you suggested. I don't see any performance related issue with that. Cheers, Paolo
On Fri, May 10, 2019 at 11:52 AM Paolo Abeni <pabeni@redhat.com> wrote: > On Fri, 2019-05-10 at 11:32 -0400, Paul Moore wrote: > > On Fri, May 10, 2019 at 9:49 AM Paolo Abeni <pabeni@redhat.com> 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 skipping the checks when the address family is not > > > AF_INET{4,6} - we don't have any port to validate, 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> > > > --- > > > v1 -> v2: > > > - avoid validation for AF_UNSPEC > > > --- > > > security/selinux/hooks.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > What was wrong with explicitly checking for AF_UNSPEC as I mentioned > > in my last email? > > Whoops sorry, I missed a relevant part of that email. > > Reading it now. > > To me, the 2 options look quite similar, and I have a slighter > preference for this one, being a smaller change possibly more suited to > a stable fix. > > But if you have strong different opinions I can post the code you > suggested. I don't see any performance related issue with that. My thinking is that this patch affects address families other than AF_UNSPEC, whereas the fix I suggested only affects AF_UNSPEC. Given the compatibility problems we have had with this code recently, I would prefer what I suggested since it has the least impact to userspace. It also has the benefit of solving AF_UNSPEC regardless of the SELinux socket class and moving the addrlen check higher up the function; these things alone aren't really a big deal, but they are nice side effects.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index c61787b15f27..bccc4b3e6f57 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4674,12 +4674,13 @@ 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; + return 0; } 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 skipping the checks when the address family is not AF_INET{4,6} - we don't have any port to validate, 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> --- v1 -> v2: - avoid validation for AF_UNSPEC --- security/selinux/hooks.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)