From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Moore Subject: Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() Date: Wed, 9 May 2018 11:01:09 -0400 Message-ID: References: <1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com> <1eb10913-8802-e2dd-68f0-9483435cd949@tycho.nsa.gov> <7fdbaf13-fea2-4a2c-213d-fa291db67081@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Alexey Kodanev , Richard Haines , selinux@tycho.nsa.gov, Eric Paris , linux-security-module@vger.kernel.org, netdev To: Stephen Smalley Return-path: Received: from mail-lf0-f47.google.com ([209.85.215.47]:36308 "EHLO mail-lf0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935204AbeEIPBM (ORCPT ); Wed, 9 May 2018 11:01:12 -0400 Received: by mail-lf0-f47.google.com with SMTP id t129-v6so12124997lff.3 for ; Wed, 09 May 2018 08:01:11 -0700 (PDT) In-Reply-To: <7fdbaf13-fea2-4a2c-213d-fa291db67081@tycho.nsa.gov> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley wrote: > On 05/08/2018 08:25 PM, Paul Moore wrote: >> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley wrote: >>> On 05/08/2018 01:05 PM, Paul Moore wrote: >>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev >>>> 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 >>>>> --- >>>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: paul@paul-moore.com (Paul Moore) Date: Wed, 9 May 2018 11:01:09 -0400 Subject: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() In-Reply-To: <7fdbaf13-fea2-4a2c-213d-fa291db67081@tycho.nsa.gov> References: <1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com> <1eb10913-8802-e2dd-68f0-9483435cd949@tycho.nsa.gov> <7fdbaf13-fea2-4a2c-213d-fa291db67081@tycho.nsa.gov> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley wrote: > On 05/08/2018 08:25 PM, Paul Moore wrote: >> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley wrote: >>> On 05/08/2018 01:05 PM, Paul Moore wrote: >>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev >>>> 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 >>>>> --- >>>>> 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