From: Paul Moore <paul@paul-moore.com> To: Alexey Kodanev <alexey.kodanev@oracle.com>, Richard Haines <richard_c_haines@btinternet.com> Cc: selinux@tycho.nsa.gov, Stephen Smalley <sds@tycho.nsa.gov>, Eric Paris <eparis@parisplace.org>, linux-security-module@vger.kernel.org, netdev <netdev@vger.kernel.org> Subject: Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() Date: Tue, 8 May 2018 13:05:48 -0400 [thread overview] Message-ID: <CAHC9VhTACGPt2dSkUN9Efxs-HQVSAsxoiwPxHcujd++O-mMafg@mail.gmail.com> (raw) In-Reply-To: <1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com> 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 > -- paul moore www.paul-moore.com
WARNING: multiple messages have this Message-ID (diff)
From: paul@paul-moore.com (Paul Moore) To: linux-security-module@vger.kernel.org Subject: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() Date: Tue, 8 May 2018 13:05:48 -0400 [thread overview] Message-ID: <CAHC9VhTACGPt2dSkUN9Efxs-HQVSAsxoiwPxHcujd++O-mMafg@mail.gmail.com> (raw) In-Reply-To: <1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com> 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 > -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-05-08 17:05 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-08 14:05 [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() Alexey Kodanev 2018-05-08 14:05 ` Alexey Kodanev 2018-05-08 17:05 ` Paul Moore [this message] 2018-05-08 17:05 ` Paul Moore 2018-05-08 18:40 ` Stephen Smalley 2018-05-08 18:40 ` Stephen Smalley 2018-05-09 0:25 ` Paul Moore 2018-05-09 0:25 ` Paul Moore [not found] ` <CAHC9VhT1+-ch1Ncv5YCNgu7tPnUj1Qx8S=a=q=Fn=Dwx4SnTKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-05-09 12:37 ` Stephen Smalley 2018-05-09 12:37 ` Stephen Smalley 2018-05-09 12:37 ` Stephen Smalley 2018-05-09 15:01 ` Paul Moore 2018-05-09 15:01 ` Paul Moore 2018-05-09 15:11 ` Stephen Smalley 2018-05-09 15:11 ` Stephen Smalley 2018-05-09 15:34 ` Paul Moore 2018-05-09 15:34 ` Paul Moore 2018-05-09 22:02 ` Paul Moore 2018-05-09 22:02 ` Paul Moore 2018-05-10 9:28 ` Alexey Kodanev 2018-05-10 9:28 ` Alexey Kodanev 2018-05-10 16:05 ` Paul Moore 2018-05-10 16:05 ` Paul Moore
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAHC9VhTACGPt2dSkUN9Efxs-HQVSAsxoiwPxHcujd++O-mMafg@mail.gmail.com \ --to=paul@paul-moore.com \ --cc=alexey.kodanev@oracle.com \ --cc=eparis@parisplace.org \ --cc=linux-security-module@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=richard_c_haines@btinternet.com \ --cc=sds@tycho.nsa.gov \ --cc=selinux@tycho.nsa.gov \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.