SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
@ 2019-05-08 13:32 Paolo Abeni
  2019-05-08 13:50 ` Marcelo Ricardo Leitner
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Paolo Abeni @ 2019-05-08 13:32 UTC (permalink / raw)
  To: Paul Moore
  Cc: selinux, netdev, Tom Deseyn, Marcelo Ricardo Leitner, Richard Haines

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 13:32 [PATCH net] selinux: do not report error on connect(AF_UNSPEC) Paolo Abeni
@ 2019-05-08 13:50 ` Marcelo Ricardo Leitner
  2019-05-08 16:46 ` David Miller
  2019-05-08 18:12 ` Stephen Smalley
  2 siblings, 0 replies; 11+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-05-08 13:50 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: Paul Moore, selinux, netdev, Tom Deseyn, Richard Haines

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 13:32 [PATCH net] selinux: do not report error on connect(AF_UNSPEC) Paolo Abeni
  2019-05-08 13:50 ` Marcelo Ricardo Leitner
@ 2019-05-08 16:46 ` David Miller
  2019-05-08 18:12 ` Stephen Smalley
  2 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2019-05-08 16:46 UTC (permalink / raw)
  To: pabeni; +Cc: paul, selinux, netdev, tdeseyn, marcelo.leitner, richard_c_haines

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!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 13:32 [PATCH net] selinux: do not report error on connect(AF_UNSPEC) Paolo Abeni
  2019-05-08 13:50 ` Marcelo Ricardo Leitner
  2019-05-08 16:46 ` David Miller
@ 2019-05-08 18:12 ` Stephen Smalley
  2019-05-08 18:13   ` Stephen Smalley
  2 siblings, 1 reply; 11+ messages in thread
From: Stephen Smalley @ 2019-05-08 18:12 UTC (permalink / raw)
  To: Paolo Abeni, Paul Moore
  Cc: selinux, netdev, Tom Deseyn, Marcelo Ricardo Leitner, Richard Haines

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 18:12 ` Stephen Smalley
@ 2019-05-08 18:13   ` Stephen Smalley
  2019-05-08 18:27     ` Marcelo Ricardo Leitner
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Smalley @ 2019-05-08 18:13 UTC (permalink / raw)
  To: Paolo Abeni, Paul Moore
  Cc: selinux, netdev, Tom Deseyn, Marcelo Ricardo Leitner, Richard Haines

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 18:13   ` Stephen Smalley
@ 2019-05-08 18:27     ` Marcelo Ricardo Leitner
  2019-05-08 18:51       ` Stephen Smalley
  0 siblings, 1 reply; 11+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-05-08 18:27 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Paolo Abeni, Paul Moore, selinux, netdev, Tom Deseyn, Richard Haines

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 18:27     ` Marcelo Ricardo Leitner
@ 2019-05-08 18:51       ` Stephen Smalley
  2019-05-08 21:17         ` Paul Moore
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Smalley @ 2019-05-08 18:51 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: Paolo Abeni, Paul Moore, selinux, netdev, Tom Deseyn, Richard Haines

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 18:51       ` Stephen Smalley
@ 2019-05-08 21:17         ` Paul Moore
  2019-05-09  8:40           ` Paolo Abeni
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Moore @ 2019-05-08 21:17 UTC (permalink / raw)
  To: Stephen Smalley, Marcelo Ricardo Leitner, Paolo Abeni
  Cc: selinux, netdev, Tom Deseyn, Richard Haines

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-08 21:17         ` Paul Moore
@ 2019-05-09  8:40           ` Paolo Abeni
  2019-05-09 13:17             ` Paul Moore
  2019-05-09 16:33             ` David Miller
  0 siblings, 2 replies; 11+ messages in thread
From: Paolo Abeni @ 2019-05-09  8:40 UTC (permalink / raw)
  To: Paul Moore, davem
  Cc: selinux, netdev, Tom Deseyn, Richard Haines, Stephen Smalley,
	Marcelo Ricardo Leitner

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-09  8:40           ` Paolo Abeni
@ 2019-05-09 13:17             ` Paul Moore
  2019-05-09 16:33             ` David Miller
  1 sibling, 0 replies; 11+ messages in thread
From: Paul Moore @ 2019-05-09 13:17 UTC (permalink / raw)
  To: Paolo Abeni
  Cc: davem, selinux, netdev, Tom Deseyn, Richard Haines,
	Stephen Smalley, Marcelo Ricardo Leitner

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
  2019-05-09  8:40           ` Paolo Abeni
  2019-05-09 13:17             ` Paul Moore
@ 2019-05-09 16:33             ` David Miller
  1 sibling, 0 replies; 11+ messages in thread
From: David Miller @ 2019-05-09 16:33 UTC (permalink / raw)
  To: pabeni
  Cc: paul, selinux, netdev, tdeseyn, richard_c_haines, sds, marcelo.leitner

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-08 13:32 [PATCH net] selinux: do not report error on connect(AF_UNSPEC) Paolo Abeni
2019-05-08 13:50 ` Marcelo Ricardo Leitner
2019-05-08 16:46 ` David Miller
2019-05-08 18:12 ` Stephen Smalley
2019-05-08 18:13   ` Stephen Smalley
2019-05-08 18:27     ` Marcelo Ricardo Leitner
2019-05-08 18:51       ` Stephen Smalley
2019-05-08 21:17         ` Paul Moore
2019-05-09  8:40           ` Paolo Abeni
2019-05-09 13:17             ` Paul Moore
2019-05-09 16:33             ` David Miller

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox