| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d637da77-2789-e08b-c78b-b48821cbf0fe@tycho.nsa.gov>
Date: Wed, 9 May 2018 11:11:09 -0400
From: Stephen Smalley <sds@...ho.nsa.gov>
To: Paul Moore <paul@...l-moore.com>
Cc: Alexey Kodanev <alexey.kodanev@...cle.com>,
Richard Haines <richard_c_haines@...nternet.com>,
selinux@...ho.nsa.gov, Eric Paris <eparis@...isplace.org>,
linux-security-module@...r.kernel.org,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to
selinux_socket_bind()
On 05/09/2018 11:01 AM, Paul Moore wrote:
> On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <sds@...ho.nsa.gov> wrote:
>> On 05/08/2018 08:25 PM, Paul Moore wrote:
>>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <sds@...ho.nsa.gov> wrote:
>>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>>> <alexey.kodanev@...cle.com> 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@...cle.com>
>>>>>> ---
>>>>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>
>
>
Powered by blists - more mailing lists