On 05/09/2018 11:01 AM, Paul Moore wrote: > On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: >> On 05/08/2018 08:25 PM, Paul Moore wrote: >>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: >>>> On 05/08/2018 01:05 PM, Paul Moore wrote: >>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev >>>>> <alexey.kodanev@xxxxxxxxxx> 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@xxxxxxxxxx> >>>>>> --- >>>>>> 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. It isn't clear to me how to do that without breaking SCTP multiple address binding, which is why Richard had to switch to checking address->sa_family instead of just using the sk->sk_family. Alexey's original fix might be the simplest solution. > > 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 >>>>>> >>>>> >>>> >>> >>> >>> >> > > >