On Thu, May 10, 2018 at 5:28 AM, Alexey Kodanev
<alexey.kodanev@xxxxxxxxxx> wrote:
> On 10.05.2018 01:02, Paul Moore wrote:
> ...
>> I just had a better look at this and I believe that Alexey and Stephen
>> are right: this is the best option. My apologies for the noise
>> earlier. However, while looking at the code I think there are some
>> additional necessary changes:
>>
>> * In the case of an SCTP socket, we should return -EINVAL, just as we
>> do with other address families.
>
> Right.
>
>> * While not strictly related to AF_UNSPEC, we really should be passing
>> the address family of the sockaddr, and not the socket, to functions
>> that need to interpret the bind address/port.
>
> That looks like a correct solution. I guess we need the same fix for
> sctp_connectx(), in selinux_socket_connect_helper().
Yes. I think we also need a small change to
selinux_sctp_bind_connect() to both not error out on AF_UNSPEC, and to
return EINVAL on a bad address family (some of the callers pass on the
return value, some don't).
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5f30045b2053..a8bac9b37ee7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5277,6 +5277,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int>
while (walk_size < addrlen) {
addr = addr_buf;
switch (addr->sa_family) {
+ case AF_UNSPEC:
case AF_INET:
len = sizeof(struct sockaddr_in);
break;
@@ -5284,7 +5285,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int>
len = sizeof(struct sockaddr_in6);
break;
default:
- return -EAFNOSUPPORT;
+ return -EINVAL;
}
err = -EINVAL;
>> I'm waiting for my kernel to compile so I haven't given this any
>> sanity testing, but the patch below is what I think we need ...
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 4cafe6a19167..5f30045b2053 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4576,6 +4576,7 @@ static int selinux_socket_post_create(struct socket *sock,
>> int family,
>> static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, i
>> nt addrlen)
>> {
>> struct sock *sk = sock->sk;
>> + struct sk_security_struct *sksec = sk->sk_security;
>> u16 family;
>> int err;
>>
>> @@ -4587,13 +4588,13 @@ static int selinux_socket_bind(struct socket *sock, stru
>> ct sockaddr *address, in
>> family = sk->sk_family;
>> if (family == PF_INET || family == PF_INET6) {
>> char *addrp;
>> - struct sk_security_struct *sksec = sk->sk_security;
>> struct common_audit_data ad;
>> struct lsm_network_audit net = {0,};
>> struct sockaddr_in *addr4 = NULL;
>> struct sockaddr_in6 *addr6 = NULL;
>> unsigned short snum;
>> u32 sid, node_perm;
>> + u16 family_sa = address->sa_family;
>>
>> /*
>> * sctp_bindx(3) calls via selinux_sctp_bind_connect()
>> @@ -4601,11 +4602,19 @@ static int selinux_socket_bind(struct socket *sock, stru
>> ct sockaddr *address, in
>> * 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_UNSPEC:
>> case AF_INET:
>> if (addrlen < sizeof(struct sockaddr_in))
>> return -EINVAL;
>> addr4 = (struct sockaddr_in *)address;
>> + if (family_sa == AF_UNSPEC) {
>> + /* see "__inet_bind()", we only want to allow
>> + * AF_UNSPEC if the address is INADDR_ANY */
>> + if (addr4->sin_addr.s_addr != htonl(INADDR_ANY))
>> + goto err_af;
>> + family_sa = AF_INET;
>> + }
>> snum = ntohs(addr4->sin_port);
>> addrp = (char *)&addr4->sin_addr.s_addr;
>> break;
>> @@ -4617,15 +4626,14 @@ static int selinux_socket_bind(struct socket *sock, stru
>> ct sockaddr *address, in
>> addrp = (char *)&addr6->sin6_addr.s6_addr;
>> break;
>> default:
>> - /* Note that SCTP services expect -EINVAL, whereas
>> - * others expect -EAFNOSUPPORT.
>> - */
>> - if (sksec->sclass == SECCLASS_SCTP_SOCKET)
>> - return -EINVAL;
>> - else
>> - return -EAFNOSUPPORT;
>> + goto err_af;
>> }
>>
>> + ad.type = LSM_AUDIT_DATA_NET;
>> + ad.u.net = &net;
>> + ad.u.net->sport = htons(snum);
>> + ad.u.net->family = family_sa;
>> +
>
> May be we could move setting ad.u.net->v{4|6}info.saddr here as well?
I looked at that too, the problem is that if we set the IP address
here it will be reported in the audit record for a name_bind failure,
which is a change from the current behavior. One could argue this is
the correct thing to do, but I would like to limit the number of
changes for patches that are destined for the -rcX stream.
Let's leave them separate for now.
> Will send a v2 of this patch so that SCTP socket returns EINVAL with
> AF_UNSPEC. Should I prepare a patch with correcting 'ad.u.net->family'
> and sel_netnode_sid()?
Please, that would be helpful. I think all of the issues we have
identified in this thread should be fixed during the v4.17-rcX
releases, so if you don't do it I'll need to do it :)
--
paul moore
www.paul-moore.com
