netdev - Re: [PATCH net] selinux: do not report error on connect(AF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 May 2019 17:17:34 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Stephen Smalley <sds@...ho.nsa.gov>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     selinux@...r.kernel.org, netdev@...r.kernel.org,
        Tom Deseyn <tdeseyn@...hat.com>,
        Richard Haines <richard_c_haines@...nternet.com>
Subject: Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)

On Wed, May 8, 2019 at 2:55 PM Stephen Smalley <sds@...ho.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@...hat.com>
> >>>> Signed-off-by: Paolo Abeni <pabeni@...hat.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