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