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.
>
> 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?
> 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
>>
>
