* [PATCH v2] selinux: do not report error on connect(AF_UNSPEC)
@ 2019-05-10 13:47 Paolo Abeni
2019-05-10 15:32 ` Paul Moore
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2019-05-10 13:47 UTC (permalink / raw)
To: selinux; +Cc: Paul Moore, Tom Deseyn, Richard Haines, Marcelo Ricardo Leitner
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] selinux: do not report error on connect(AF_UNSPEC)
2019-05-10 13:47 [PATCH v2] selinux: do not report error on connect(AF_UNSPEC) Paolo Abeni
@ 2019-05-10 15:32 ` Paul Moore
2019-05-10 15:51 ` Paolo Abeni
0 siblings, 1 reply; 4+ messages in thread
From: Paul Moore @ 2019-05-10 15:32 UTC (permalink / raw)
To: Paolo Abeni; +Cc: selinux, Tom Deseyn, Richard Haines, Marcelo Ricardo Leitner
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
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] selinux: do not report error on connect(AF_UNSPEC)
2019-05-10 15:32 ` Paul Moore
@ 2019-05-10 15:51 ` Paolo Abeni
2019-05-10 16:09 ` Paul Moore
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2019-05-10 15:51 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux, Tom Deseyn, Richard Haines, Marcelo Ricardo Leitner
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] selinux: do not report error on connect(AF_UNSPEC)
2019-05-10 15:51 ` Paolo Abeni
@ 2019-05-10 16:09 ` Paul Moore
0 siblings, 0 replies; 4+ messages in thread
From: Paul Moore @ 2019-05-10 16:09 UTC (permalink / raw)
To: Paolo Abeni; +Cc: selinux, Tom Deseyn, Richard Haines, Marcelo Ricardo Leitner
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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-10 16:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-10 13:47 [PATCH v2] selinux: do not report error on connect(AF_UNSPEC) Paolo Abeni
2019-05-10 15:32 ` Paul Moore
2019-05-10 15:51 ` Paolo Abeni
2019-05-10 16:09 ` Paul Moore
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).