| Message ID | 1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() | expand |
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. 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; > 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 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 >> >
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 >>>> >>> >> > > >
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 >>>>>> >>>>> >>>> >>> >>> >>> >> > > >
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 >>>>>>> >>>>>> >>>>> >>>> >>>> >>>> >>> >> >> >> >
On Wed, May 9, 2018 at 11:34 AM, Paul Moore <paul@paul-moore.com> wrote: > 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. 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. * 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. 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; + if (snum) { int low, high; @@ -4637,10 +4645,6 @@ static int selinux_socket_bind(struct socket *sock, struc t sockaddr *address, in snum, &sid); if (err) goto out; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->sport = htons(snum); - ad.u.net->family = family; err = avc_has_perm(&selinux_state, sksec->sid, sid, sksec->sclass, @@ -4672,16 +4676,11 @@ static int selinux_socket_bind(struct socket *sock, stru ct sockaddr *address, in break; } - err = sel_netnode_sid(addrp, family, &sid); + err = sel_netnode_sid(addrp, family_sa, &sid); if (err) goto out; - ad.type = LSM_AUDIT_DATA_NET; - ad.u.net = &net; - ad.u.net->sport = htons(snum); - ad.u.net->family = family; - - if (address->sa_family == AF_INET) + if (family_sa == AF_INET) ad.u.net->v4info.saddr = addr4->sin_addr.s_addr; else ad.u.net->v6info.saddr = addr6->sin6_addr; @@ -4694,6 +4693,12 @@ static int selinux_socket_bind(struct socket *sock, struc t sockaddr *address, in } out: return err; +err_af: + /* Note that SCTP services expect -EINVAL, others -EAFNOSUPPORT. */ + if (sksec->sclass == SECCLASS_SCTP_SOCKET) + return -EINVAL; + else + return -EAFNOSUPPORT; } /* This supports connect(2) and SCTP connect services such as sctp_connectx(3)
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(). > > 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? 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()? Thanks, Alexey > if (snum) { > int low, high; > > @@ -4637,10 +4645,6 @@ static int selinux_socket_bind(struct socket *sock, struc > t sockaddr *address, in > snum, &sid); > if (err) > goto out; > - ad.type = LSM_AUDIT_DATA_NET; > - ad.u.net = &net; > - ad.u.net->sport = htons(snum); > - ad.u.net->family = family; > err = avc_has_perm(&selinux_state, > sksec->sid, sid, > sksec->sclass, > @@ -4672,16 +4676,11 @@ static int selinux_socket_bind(struct socket *sock, stru > ct sockaddr *address, in > break; > } > > - err = sel_netnode_sid(addrp, family, &sid); > + err = sel_netnode_sid(addrp, family_sa, &sid); > if (err) > goto out; > > - ad.type = LSM_AUDIT_DATA_NET; > - ad.u.net = &net; > - ad.u.net->sport = htons(snum); > - ad.u.net->family = family; > - > - if (address->sa_family == AF_INET) > + if (family_sa == AF_INET) > ad.u.net->v4info.saddr = addr4->sin_addr.s_addr; > else > ad.u.net->v6info.saddr = addr6->sin6_addr; > @@ -4694,6 +4693,12 @@ static int selinux_socket_bind(struct socket *sock, struc > t sockaddr *address, in > } > out: > return err; > +err_af: > + /* Note that SCTP services expect -EINVAL, others -EAFNOSUPPORT. */ > + if (sksec->sclass == SECCLASS_SCTP_SOCKET) > + return -EINVAL; > + else > + return -EAFNOSUPPORT; > } > > /* This supports connect(2) and SCTP connect services such as sctp_connectx(3) >
On Thu, May 10, 2018 at 5:28 AM, Alexey Kodanev <alexey.kodanev@oracle.com> 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 :)
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,
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(-)