[PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Paolo Abeni]

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;
 		}
 
 		err = sel_netport_sid(sk->sk_protocol, snum, &sid);
-- 
2.20.1

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Marcelo Ricardo Leitner]

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
> 

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[David Miller]

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!

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Stephen Smalley]

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

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Stephen Smalley]

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

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Marcelo Ricardo Leitner]

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

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Stephen Smalley]

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

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Paul Moore]

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

-- 
paul moore
www.paul-moore.com

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Paolo Abeni]

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

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[Paul Moore]

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!

-- 
paul moore
www.paul-moore.com

Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

[David Miller]

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.