[PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()

Paul Moore paul at paul-moore.com
Wed May 9 15:01:09 UTC 2018


On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <sds at tycho.nsa.gov> wrote:
> On 05/08/2018 08:25 PM, Paul Moore wrote:
>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <sds at tycho.nsa.gov> wrote:
>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>> <alexey.kodanev at oracle.com> wrote:
>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>> It was found with LTP/asapi_01 test.
>>>>>
>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>
>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>
>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>> Signed-off-by: Alexey Kodanev <alexey.kodanev at oracle.com>
>>>>> ---
>>>>>  security/selinux/hooks.c | 12 +++++++++---
>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> Thanks for finding and reporting this regression.
>>>>
>>>> I think I would prefer to avoid having to duplicate the
>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>> it is a small bit of code and unlikely to change.  I'm wondering if it
>>>> would be better to check both the socket and sockaddr address family
>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>> think?  Another option would be to go back to just checking the
>>>> soackaddr address family; we moved away from that for a reason which
>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>> mistake.
>>>
>>> We've always used the sk->sk_family there; it was only the recent code from Richard that started
>>> using the socket address family.
>>
>> Yes I know, I thought I was the one that suggested it at some point
>> (I'll take the blame) ... although now that I'm looking at the git
>> log, maybe I'm confusing it with something else.
>>
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 4cafe6a19167..a3789b167667 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock, struc>
>>>> {
>>>>        struct sock *sk = sock->sk;
>>>>        u16 family;
>>>> +       u16 family_sa;
>>>>        int err;
>>>>
>>>>        err = sock_has_perm(sk, SOCKET__BIND);
>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock, struc>
>>>>
>>>>        /* If PF_INET or PF_INET6, check name_bind permission for the port. */
>>>>        family = sk->sk_family;
>>>> -       if (family == PF_INET || family == PF_INET6) {
>>>> +       family_sa = address->sa_family;
>>>> +       if ((family == PF_INET || family == PF_INET6) &&
>>>> +           (family_sa == PF_INET || family_sa == PF_INET6)) {
>>>
>>> Wouldn't this allow bypassing the name_bind permission check by passing in AF_UNSPEC?
>>
>> I believe these name_bind permission checkis skipped for AF_UNSPEC
>> already, isn't it?  The only way the name_bind check would be
>> triggered is if the source port, snum, was non-zero and I didn't think
>> that was really legal for AF_UNSPEC/INADDR_ANY, is it?
>
> 1) What in inet_bind() prevents that from occurring?
> 2) Regardless, what about the node_bind check?

Fair enough.  As mentioned above, perhaps the right fix is to move the
address family checking back to how it was pre-SCTP.

Alexey, is this something you want to do, or should we take care of that?

>>>>                char *addrp;
>>>>                struct sk_security_struct *sksec = sk->sk_security;
>>>>                struct common_audit_data ad;
>>>> @@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock, struc>
>>>>                 * need to check address->sa_family as it is possible to have
>>>>                 * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>>>>                 */
>>>> -               switch (address->sa_family) {
>>>> +               switch (family_sa) {
>>>>                case AF_INET:
>>>>                        if (addrlen < sizeof(struct sockaddr_in))
>>>>                                return -EINVAL;
>>>>
>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>> index 4cafe6a..649a3be 100644
>>>>> --- a/security/selinux/hooks.c
>>>>> +++ b/security/selinux/hooks.c
>>>>> @@ -4602,10 +4602,16 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>>>>>                  * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>>>>>                  */
>>>>>                 switch (address->sa_family) {
>>>>> +               case AF_UNSPEC:
>>>>>                 case AF_INET:
>>>>>                         if (addrlen < sizeof(struct sockaddr_in))
>>>>>                                 return -EINVAL;
>>>>>                         addr4 = (struct sockaddr_in *)address;
>>>>> +
>>>>> +                       if (address->sa_family == AF_UNSPEC &&
>>>>> +                           addr4->sin_addr.s_addr != htonl(INADDR_ANY))
>>>>> +                               return -EAFNOSUPPORT;
>>>>> +
>>>>>                         snum = ntohs(addr4->sin_port);
>>>>>                         addrp = (char *)&addr4->sin_addr.s_addr;
>>>>>                         break;
>>>>> @@ -4681,10 +4687,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>>>>>                 ad.u.net->sport = htons(snum);
>>>>>                 ad.u.net->family = family;
>>>>>
>>>>> -               if (address->sa_family == AF_INET)
>>>>> -                       ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
>>>>> -               else
>>>>> +               if (address->sa_family == AF_INET6)
>>>>>                         ad.u.net->v6info.saddr = addr6->sin6_addr;
>>>>> +               else
>>>>> +                       ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
>>>>>
>>>>>                 err = avc_has_perm(&selinux_state,
>>>>>                                    sksec->sid, sid,
>>>>> --
>>>>> 1.8.3.1
>>>>>
>>>>
>>>
>>
>>
>>
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



More information about the Linux-security-module-archive mailing list