| Message ID | CAHC9VhTACGPt2dSkUN9Efxs-HQVSAsxoiwPxHcujd++O-mMafg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/08/2018 01:05 PM, Paul Moore wrote: > On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev > <alexey.kodanev@oracle.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@oracle.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. > > 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 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <sds@tycho.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@oracle.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@oracle.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? >> 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 >>> >> >
On 05/08/2018 08:25 PM, Paul Moore wrote: > On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <sds@tycho.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@oracle.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@oracle.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? > >>> 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 >>>> >>> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <sds@tycho.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@tycho.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@oracle.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@oracle.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. 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 >>>>> >>>> >>> >> >> >> >
On 05/09/2018 11:01 AM, Paul Moore wrote: > On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <sds@tycho.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@tycho.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@oracle.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@oracle.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 >>>>>> >>>>> >>>> >>> >>> >>> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 9, 2018 at 11:11 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 05/09/2018 11:01 AM, Paul Moore wrote: >> On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <sds@tycho.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@tycho.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@oracle.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@oracle.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. I'm going to have to apologize, I'm traveling at the moment and more distracted than usual as a result. Let me take a closer look later today. It may be that Alexey's original fix the only practical solution, but I really would like to avoid having to duplicate checks like that in the SELinux code. >> 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 >>>>>>> >>>>>> >>>>> >>>> >>>> >>>> >>> >> >> >> >
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)) { 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;